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. 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. 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. 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. 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. ======================================================================== 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. -- Stefan Teleman Sun Microsystems, Inc. Stefan.Teleman at Sun.COM
