In my NSS port to windows 64-bit, pk11mode crashes in C_Initialize. The reason is that pk11mode.c defines the structure CK_C_INITIALIZE_ARGS_NSS . It appears to be an identical copy of the one in pkcs11t.h, but actually it is not, because it has different structure packing. This is the reason for the crash. There are two possible fixes here : 1) delete this extraneous copy of the structure, and just use the one from pkcs11.h 2) include pkcs11p.h and pkcs11u.h before and after the definition of the structure
Created attachment 286122 [details] [diff] [review] Fix 1 - remove duplicate declaration I favor this fix - I think it's never good to have 2 copies of the same thing.
Attachment #286122 - Flags: review?(glen.beasley)
Created attachment 286123 [details] [diff] [review] Fix 2 - fix structure packing in the second declaration
Attachment #286123 - Flags: review?(glen.beasley)
Comment on attachment 286122 [details] [diff] [review] Fix 1 - remove duplicate declaration I prefer fix 2. The benefit of having CK_C_INITIALIZE_ARGS_NSS is that on windows one can choose to build with RSA Security pkcs11.h file that can be download from http://www.rsa.com/rsalabs/node.asp?id=2133 and libsoftokn3.dll. This way one can ensure that they are not pulling extra NSS api.
Attachment #286122 - Flags: review?(glen.beasley) → review-
Thanks for the review, Glen. I checked this in to the trunk. Checking in pk11mode.c; /cvsroot/mozilla/security/nss/cmd/pk11mode/pk11mode.c,v <-- pk11mode.c new revision: 1.17; previous revision: 1.16 done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.12
Comment on attachment 286123 [details] [diff] [review] Fix 2 - fix structure packing in the second declaration We recently fixed a bug in another NSS source file caused by the improper use of pkcs11p.h. That was bug 388824. Does this patch introduce that same bug in a different source file?
To expand on that last comment, the improper use was that the packing of the struct was being done inconsistently, in some .c files but not in others. If a struct is going to be defined as packed in any .c file that uses it, then it must be consistently defined in all that do. It's probably best to never include pkcs11p.h in any .c file and only do so in .h files.
Nelson, Bug 388824 was about a different structure being affected by packing. In this case, the pkcs11t.h from NSS had the proper definition for CK_C_INITIALIZE_ARGS, with the proper packing. The problem is caused by the fact that pk11mode.c isn't using the definition from our header file. It declares a second one called CK_C_INITIALIZE_ARGS_NSS , which Glen did so that pk11mode can be compiled against RSA's own pkcs11 headers that don't include the LibraryParameters. When Glen copied, pasted and renamed the CK_C_INITIALIZE_ARGS structure definition from our version of pkcs11t.h to pk11mode.c, he forgot about the packing. So, on Windows AMD64, pk11mode was filling the structure a certain way, and softoken was getting a different view when reading it, and that caused a runtime crash. My preference was for not having 2 copies (see fix 1), but fix 2 allows pk11mode to continue to build pk11mode from the RSA PKCS#11 headers and run it successfully.
Note also that pkcs11p.h is OK to use in a C file if it is followed by the use of pkcs11u.h, which restores the packing to the previous state, as I did in fix 2.
This fix is also needed on NSS_3_11_BRANCH since we are also going to release Win64 from it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.12 → 3.11.9
Attachment #286123 - Flags: superreview?(nelson) → superreview+
Thanks, Nelson. I checked this in to the branch. Checking in pk11mode.c; /cvsroot/mozilla/security/nss/cmd/pk11mode/pk11mode.c,v <-- pk11mode.c new revision: 126.96.36.199; previous revision: 188.8.131.52 done
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago → 11 years ago
Resolution: --- → FIXED
Actually, this commit was made before the RC2 for NSS 3.11.8. Changing the Target Milestone to 3.11.8 instead of 3.11.9.
Target Milestone: 3.11.9 → 3.11.8
You need to log in before you can comment on or make changes to this bug.