pk11mode crashes on Win64

RESOLVED FIXED in 3.11.8

Status

NSS
Tools
P2
normal
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

trunk
3.11.8
x86
Windows Server 2003

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
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

11 years ago
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)
(Assignee)

Comment 2

11 years ago
Created attachment 286123 [details] [diff] [review]
Fix 2 - fix structure packing in the second declaration
Attachment #286123 - Flags: review?(glen.beasley)

Updated

11 years ago
Attachment #286123 - Flags: review?(glen.beasley) → review+

Comment 3

11 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

11 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
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.  
(Assignee)

Comment 7

11 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

11 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

10 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

10 years ago
Attachment #286123 - Flags: superreview?(nelson)
Attachment #286123 - Flags: superreview?(nelson) → superreview+
(Assignee)

Comment 10

10 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
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED

Comment 11

10 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.