[kmf-discuss] KMF code review (part II.)

[Wyllys Ingersoll]

Jan Pechanec wrote:
> 	hi Wyllys,
>
> usr/src/cmd/cmd-crypto/pktool/gencsr.c
>
>  182         free_eku_list(ekulist);
>  346         free_eku_list(ekulist);
>  496         free_eku_list(ekulist);
>
> 	- similar note as with gencert.c, I would suggest to free ekulist in 
> pk_gencsr() for consistency reasons
>
>   
OK

> usr/src/cmd/cmd-crypto/pktool/pktool.c
>
> 	- nit, missing space between "\
>   
OK
>  351 #define GENCSR_SUMM gettext("creates a PKCS#10 certificate signing "\
>
> 	- should there be two tabelators or one?
>   
>  401         "[ outfile = outfile ]\n\t\t")
>   
One.

> 	- extra space before )
>
>  445         "[ altname=subjectAltName ] (add subjectAltName )\n\t\t" \
>  447         "[ eku=[critical:]EKU Name,...] (add Extended Key Usage 
> )\n\t\t" \
>
>
>   
OK


> usr/src/lib/libkmf/ber_der/common/decode.c
>
> 	- could we check for != NULL?
>
>  683                         if (*ss)
>   
OK

> usr/src/lib/libkmf/include/kmftypes.h
>
>  973 #define OID_KRB5_PKINIT_KPCLIENTAUTH_LENGTH OID_KRB5_PKINIT_LENGTH + 1
>  976 #define OID_KRB5_PKINIT_KPKDC_LENGTH    OID_KRB5_PKINIT_LENGTH + 1
>  994 #define OID_MS_KP_SC_LOGON_LENGTH       OID_MS_LENGTH + 3
>  997 #define OID_MS_KP_SC_LOGON_UPN_LENGTH   OID_MS_LENGTH + 3
>
> 	- missing parentheses
>   
OK

> 1172 KMFOID_PKINIT_san,
> 1173 KMFOID_PKINIT_ClientAuth,
> 1174 KMFOID_PKINIT_Kdc,
> 1175 KMFOID_MS_KP_SCLogon,
> 1176 KMFOID_MS_KP_SCLogon_UPN;
>
> 	- could these be indented with 4 spaces since those are continuation 
> lines?
>   

They could be, but the rest of the file is not like that and cstyle 
doesnt seem to care
so I'm going to leave them alone.

> usr/src/lib/libkmf/libkmf/common/certgetsetop.c
>
> 	- ok
>
> usr/src/lib/libkmf/libkmf/common/certop.c
>
>  510         /* if no OID and No AlgID, use the signer cert */
>
> 	- "No" --> "no"
>
>   
OK

>  521         if (x509_cert != NULL) {
>  522                 kmf_free_signed_cert(x509_cert);
>  523                 free(x509_cert);
>  524         }
>
> 	- similar to gencert(), these should be rather deallocated in the 
> same function where allocated
>   
No, in this case x509_cert is allocated in that same routine, not passed 
in from another one.

>  771         /* We only need the algorithm index if we dont have a signer 
> cert. */
>
> 	- "dont" --> "don't"
>
>
> usr/src/lib/libkmf/libkmf/common/csrcrlop.c
>
>  281                     foundextn->BERvalue.Length,      &olddata, 
>
> 	- extra tab or spaces
>   
OK

>
> usr/src/lib/libkmf/libkmf/common/kmfoids.c
>
>  394 KMFOID_PKINIT_san = {OID_KRB5_SAN_LENGTH, OID_pkinit_san },
>  395 KMFOID_PKINIT_ClientAuth = {OID_KRB5_PKINIT_KPCLIENTAUTH_LENGTH,
>  396     OID_pkinit_kp_clientauth},
>  397 KMFOID_PKINIT_Kdc = {OID_KRB5_PKINIT_KPKDC_LENGTH,
>  398     OID_pkinit_kp_kdc},
>  399 KMFOID_MS_KP_SCLogon = {OID_MS_KP_SC_LOGON_LENGTH,
>  400     OID_pkinit_kp_sc_logon},
>  401 KMFOID_MS_KP_SCLogon_UPN = {OID_MS_KP_SC_LOGON_UPN_LENGTH,
>  402     OID_pkinit_san_upn};
>
> 	- missing indentation of all lines since they are continuation 
> lines for 393
>   

Yes, but it is consistent with the rest of that file and it is not 
causing any cstyle
errors at this time, so I'll leave it alone for now.


> usr/src/lib/libkmf/libkmf/common/pem_encode.c
>
>  476         while ((i <= inlen) && (i <= buflen) && (in[i] != '\n')) {
>
> 	- I think sharp inequality should be there in both cases
>   

OK

>
>  507                 i = get_line(in + total, inlen - total, buf, sizeof 
> (buf));
>  508                 if (i <= 0) {
>  509                         kmf_rv = KMF_ERR_ENCODING;
>  510                         goto err;
>
> 	- I'm not sure I understand that. "i" can't be less that zero. Is
> it supposed to be a test to find out whether it's an empty line?
>   

Correct, it can only be 0 or greater.   0 if the line was empty.

>  549                 if (i <= 0) break;
>
> 	- same as above
>   
OK


Thanks for the comments so far.   The webrev will be updated shortly.

-Wyllys