Closed
Bug 401071
Opened 17 years ago
Closed 17 years ago
pk11mode crashes on Win64
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.8
People
(Reporter: julien.pierre, Assigned: julien.pierre)
Details
Attachments
(2 files)
2.78 KB,
patch
|
glenbeasley
:
review-
|
Details | Diff | Splinter Review |
662 bytes,
patch
|
glenbeasley
:
review+
nelson
:
superreview+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•17 years ago
|
||
I favor this fix - I think it's never good to have 2 copies of the same thing.
Attachment #286122 -
Flags: review?(glen.beasley)
Assignee | ||
Comment 2•17 years ago
|
||
Attachment #286123 -
Flags: review?(glen.beasley)
Updated•17 years ago
|
Attachment #286123 -
Flags: review?(glen.beasley) → review+
Comment 3•17 years ago
|
||
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-
Assignee | ||
Comment 4•17 years ago
|
||
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
Closed: 17 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 3.12
Comment 5•17 years ago
|
||
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?
Comment 6•17 years ago
|
||
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.
Assignee | ||
Comment 7•17 years ago
|
||
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.
Assignee | ||
Comment 8•17 years ago
|
||
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.
Assignee | ||
Comment 9•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Attachment #286123 -
Flags: superreview?(nelson)
Updated•17 years ago
|
Attachment #286123 -
Flags: superreview?(nelson) → superreview+
Assignee | ||
Comment 10•17 years ago
|
||
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: 1.1.2.15; previous revision: 1.1.2.14
done
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 11•17 years ago
|
||
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.
Description
•