[kmf-discuss] kmf code review (part I.)

[Wyllys Ingersoll]

Jan Pechanec wrote:
> 	
>
> usr/src/cmd/cmd-crypto/pktool/common.c
>
> 1114         if (ekus != NULL && ekus->eku_count > 0) {
>
> 	- just asking - is it possible that ekus might point to a 
> dynamically allocated memory and eku_count still be 0?
>   

I don't think so, but this is just to be safe.

>
> 1133         if (ekuliststr == NULL || !strlen(ekuliststr))
>
> 	- I think we should not use strlen() output as boolean value. It's 
> not so we should check for != 0
>   

I don't see why that is dangerous or confusing, its just more of a 
personal preference, plus it
helps avoid cstyle errors by running over the 80 column limit in some cases.

>
> 1143                 if (!strncasecmp(ekuliststr, "critical:",
> 1144                     strlen("critical:"))) {
>
> 	- even here, I would suggest to use == 0. It's just easier to read.
>   
OK.

>
> 1141         /* If no tokens found, then maybe its just a single EKU value 
> */
> 	- 'its' -> "it's"
>
> 1117                         kmf_free_data(&ekus->ekulist[i]);
>   
> 	- hmm, I think this is wrong. You are trying to free a piece of 
> memory by pieces. On line 1104 you assign the value so I think that line 
> 1117 should not be there at all since the following line takes care of 
> everything:
>
> 1119                 free(ekus->ekulist);
>   
No, that takes care of freeing the list of OIDs, but not the actual data 
within each OID.
The individual kmf_free_data call (above, line 1117) is needed to free 
the "Data" member
of the OIDs.

> 	- and memory for OIDs is freed here:
>
> 1153                         free(newoid);
>   

That frees the OID structure itself, but not the Data member inside the 
OID, so this free is still needed.


> lines 1143-1156:
>
> 	- could we put those lines into a helper function and use it right 
> below? It's completely identical code aside from p/ekuliststr and one 
> break... In the future, it's just easy to change/fix/whatever one place but 
> forget about the other.
>   

Yes, good idea - done.

>
>
> usr/src/cmd/cmd-crypto/pktool/common.h
>
>   86 }EKU_LIST;
>
> 	- missing space after '}' ?
>   

cstyle didn't seem to mind, but I will fix it anyway.
>
>
> usr/src/cmd/cmd-crypto/pktool/gencert.c
>
>  176                             &ekulist->ekulist[i],
>  177                             ekulist->critlist[i]), "Extended Key 
> Usage");
>
> 	- it might be easier to read if "ekulist->critlist[i])," was put to 
>   

OK

> 176
>
>  232         free_eku_list(ekulist);
>
> 	- I don't like too much that we put a dynamically allocated memory 
> to a function and expect it to free it then. It's just, hmm, not used very 
> often. Well, it's internal function but it's the only parameter that is 
> alloated outside and freed inside, other are not - subname, for example.
>   

I agree, but the problem with an unbounded list is that you never know 
how much to
allocate.  If I pre-allocate a list, it will undoubtedly get broken some 
day by a user
who puts 1 too many on the list. 

> 172-180
> 399-407
> 599-606
>
> 	- could we have a helper function for that?
>   
I don't think its necessary in that case, its is just a simple loop 
around a single macro.  If there
were additional logic involved, I would consider it, but as it stands it 
is pretty straightforward
to read and follow.

>  983                     &serial, kubits, kucrit, &tokencred,
>  984                     ekulist);
>
> 	- can be put on 1 line
>   
OK

Thanks for the comments so far.
I've updated the webrev to reflect the changes I made so far.

http://cr.opensolaris.org/~wyllys/kmf


-Wyllys