Dan Anderson wrote:
> Hai-May,
>
> I have one question concerning
> http://www.opensolaris.org/os/project/crypto/inprogress/fips/admin-policy-design/
> >From section 3, I assume that if there's a hardware provider (such as
> >ncp/0), that it is left enabled and only a warning message is printed.
> Is this true?
>
>
Dan,
Yes, it'll be left enabled with a warning message.
Thanks for your review. I updated the webrev with
all your comments.
Thanks,
Hai-May
> Comments:
>
> DEA-1 adm_kef.c lines 1392-1399
> mech_table should be a local variable inside fips_sync_ulp() around line 1408,
> as only fips_sync_ulp() uses the variable.
> Static variables inside a function are still static and are not on the stack.
>
> DEA-2 adm_kef.c lines 1464-1468
> Same comment as DEA-1.
> prov_table should be inside fips_sync_ksp() around line 1476.
>
> DEA-3 adm_kef.c lines 1431 and 1434
> You allocate memory and store it in argv[3] at line 1431, yet on line 1434
> you overwrite the pointer with a pointer to a string constant ("mechanism=").
>
> As a result, the strcat() on line 1437 is overwriting memory that may be used
> for something else.
>
> To fix this, replace the pointer assignment on line 1434 with strlcpy() or
> similar string copy function.
>
> DEA-4 adm_kef.c lines 1498 and 1504.
> Same problem as DEA-3.
> You are overwriting a pointer allocated with calloc() on line 1498 at line
> 1504. Replace the pointer assignment on line 1505 with a strlcpy() or
> similar string copy function.
>
> DEA-5 adm_kef.c lines 1535-1537
> You should have a I18N TRANSLATION_NOTE preceeding the error message.
> Something like:
>
> /*
> * TRANSLATION_NOTE
> * %1$s is "enable" or "disable" and is not to be translated.
> * %2$s is "global" and is not to be translated.
> */
>
> DEA-6 adm_kef_util.c line 307
> Nit: you need a space before and after the + operators:
> 307 if ((rc = parse_fips(buf+strlen(token1)+1, pent)) != SUCCESS) {
>
> DEA-7 (all files)
> Nit: you need to update the copyright to 2009.
>
> The rest looks good to me.
>
> - Dan
>
