Hi, Stefan, Thank you for your feedback. Please see my comments interspersed below.
Stefan Teleman wrote: > Huie-Ying Lee wrote: > >> Hi all, >> >> Please help review code for the pam_pkcs11 project which is to be >> integrated into SFWNV consolication. >> >> The webrev is located at: http://cr.opensolaris.org/~hylee/pam_pkcs11/ >> > > ------------------ > > Hi. > > Please find my comments below. > > --Stefan > > ----------------- > > http://cr.opensolaris.org/~hylee/pam_pkcs11/usr/src/lib/pam_pkcs11/pam_pkcs11.patch.patch: > > 1. > > static int > memcmp_pad_max(void *d1, unsigned int d1_len, void *d2, unsigned int d2_len, > unsigned int max_sz) > { > unsigned int len, extra_len; > > /* ... */ > > /* Have a match in the shortest length of data? */ > if (memcmp(d1, d2, len) != 0) > /* CONSTCOND */ > return (1); > } > > 1.1 Since this function calls memcmp(3C), the function arguments > d1_len and d2_len, and the variables len and extra_len are technically > of incorrect type: the prototype for memcmp(3C) provides for the > third argument n to be of type size_t, and not type unsigned int. > > In 32-bit, sizeof(unsigned int) == sizeof(size_t). > In 64-bit, sizeof(unsigned int) < sizeof(size_t). > > The function arguments and the len, extra_len variables should be > changed to size_t. > > OK. I will change their data types to be size_t. > 2. > > int > find_slot_by_slotlabel(pkcs11_handle_t *h, const char *wanted_slot_label, > unsigned int *slot_num) > { > /* ... */ > int index; > int len; > > /* ... */ > > for (index = 0; index < h->slot_count; index++) { > if (h->slots[index].token_present) { > *slot_num = index; > return (0); > } > } > > /* ... */ > > } else { > /* Look up the slot by it's slotDescription */ > len = strlen(wanted_slot_label); > > /* ... */ > } > > 2.1 The argument slot_num is of type pointer-to-unsigned-int. > The variable index is of type signed int. Technically speaking, the > signedness of the local variable should match the signedness of the > argument. > > OK. After checking all related places, I think I should change the type of the variable index to be CK_ULONG. > 2.2. The struct member variable h->slot_count is declared as: > > struct pkcs11_handle_str { > /* ... */ > CK_ULONG slot_count; > }; > > CK_ULONG is typedef'ed as: > > typedef unsigned long int CK_ULONG; > > in $(top_srcdir)/src/common/rsaref/pkcs11t.h > > which means that in 64-bit sizeof(CK_ULONG) > sizeof(int). > > (unsigned long would have been sufficient, without the extra 'int' > for CK_ULONG, but that's a different story). > > The type of the variable index should be changed at least to > an unsigned long, if not to the typedef it is being assigned to. > > OK. I will change the type of the function argument "slot_num" to be CK_ULONG_PTR and change the type of the variable index to be CK_ULONG. > 2.3. The return of strlen(3C) is of type size_t, and not > (signed) int. The type size_t is an unsigned integral type, and its > sizes are: > > sizeof(size_t) == sizeof(uint_t) == 4 bytes in 32-bit > sizeof(size_t) == sizeof(ulong_t) == 8 bytes in 64-bit > > The type of the variable len should be changed from int to size_t. > > OK. Will change the type of the variable len to be size_t. > 3. > > int > find_slot_by_slotlabel_and_tokenlabel(pkcs11_handle_t *h, > const char *wanted_slot_label, const char *wanted_token_label, > unsigned int *slot_num) > { > int i; > > /* ... */ > > if (h->slots[i].token_present && > strcmp(wanted_token_label, h->slots[i].label) == 0) { > *slot_num = i; > return (0); > } > > /* ... */ > > if (h->slots[i].token_present && > strcmp(wanted_token_label, h->slots[i].label) == 0 && > strcmp(wanted_slot_label, h->slots[i].slotDescription) > == 0) { > *slot_num = i; > return (0); > } > } > > 3.1. Same comments as in [ 2.1 ], this time about variable i. > > OK. Same as my reply to 2.2, will change "slot_num" to be of the CK_ULONG_PTR type and "i" to be of the CK_ULONG type. > ======================================================================== > > http://cr.opensolaris.org/~hylee/pam_pkcs11/usr/src/lib/pam_pkcs11/Makefile.sfw.patch: > > ./configure --with-pcsclite=no --with-included-gettext=yes > > It would be better if the ./configure invocation would not rely on the > installation directory location default (which is /usr/local), and would > specify the actual directory installation location: > > PREFIX=/usr > > ./configure --prefix=$(PREFIX) \ > --libdir=$(PREFIX)/lib \ > --bindir=$(PREFIX)/bin \ > --localstatedir=/var > > for 32-bit, and > > ./configure --prefix=$(PREFIX) \ > --libdir=$(PREFIX)/lib/$(MACH64) \ > --bindir=$(PREFIX)/bin/$(MACH64) \ > --localstatedir=/var > > for 64-bit. > > By default, ./configure will set ${localstatedir} to ${prefix}/var. > If pam_pkcs11 needs to create temporary files in ${localstatedir}/tmp, > this directory location will be set to /usr/var/tmp. > > This location cannot exist in Solaris; the correct location is, in fact, > /var/tmp. > > > OK. I will update the arguments to the configure invocation, according to your suggestions. I will update the source and rebuild my gate. Will post an updated webrev once the testing is successfully run again. Thanks, Huie-Ying
