388824 - Misaligned structures in pkcs11 result in crash on 64-bit Windows

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Ian Marshall]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: 3.11.4


I am working with NSS on Windows 2003 Server (64 bit). With a minimal program during NSS_Shutdown, I would I would consistently get assertions in sftk_DBShutdown while checking keyHandle->ref.  

Examining the assembly code generated showed that there was a mismatch between the structure memory representation & the offset the code was reading the ref variable from.

The problem was tracked down to the inclusion of the pkcs11p.h file at the top of softtoken/pkcs11.c. This file changes the structure alignment for all the following include files, ONLY for the pkcs11.c module. 

Fix was simply to remove this include file from the pkcs11.c file.

It's actually pretty amazing that this does not cause failures in 32bit windows as well.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[timeless]

Attachment: per reporter...

nice bug (btw, cvs diff -up patches are appreciated, otherwise bugs might take a while to get where they need to go),
for my reference, this is how it's supposed to be used:
http://bonsai.mozilla.org/cvsblame.cgi?file=%2Fmozilla%2Fsecurity%2Fnss%2Flib%2Fsoftoken%2Fpkcs11t.h&rev=1.15&mark=112-113,1744-1745

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

32-bit x86 CPUs NEVER have alignment failures.  They literally CANNOT.
I thought this was also true for x86_64 CPUs, but maybe not.  
However, the PKCS11 headers ought not to depend on x86_32 CPU behavior.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272999 [details] [diff] [review]
per reporter...

removing this file is DEFINITELY not the right thing to do.

Any CPU that runs X86_32 code MUST NOT generate alignment exceptions.  
I know, from having done a LOT of work on AMD64 CPUs, that they do not generate those exceptions when running 32-bit code.

IFF X86_64 cannot handle non-aligned memory accesses in 64-bit code, then the ifdefs in pkcs11p.h need to be improved to not include the pragma on that platform (that is, when building x86_64 code). 
 
On the other hand, If non-aligned accesses do not work on X86_64 CPUs in 64-bit mode, the the compiler should be smart enough to not honor that pragma when building code for that mode.  As long as no pointers are being cast to other types, it should not be possible to get the compiler to produce code that has alignment exceptions.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

P.S.  I don't think Ian was suggesting that we remove that #include.
I think he was only observing that the pack pragma was related to 
the cause of the issue.  It still seems like a compiler bug to me.

Ian, what compiler and version are you running?  
Are you using any command line options in compiling or linking that 
tell the system to require/force alignment? 
I ask because I did AMD64 work with NSS for over a year and NEVER 
saw an alignment exception.  

Comment 5

[Ian Marshall]

To be clear, I am not getting an alignment exception (I agree you don't get them on x86 cpus).  I am working with the AMD64 patches.

The problem is that the same structures are compiled with different alignment directives (#pragma pack on MSVC) in different C files. When these structures are accessed by the different C objects, the code may not match the in memory representation of the struct.

The specific case I hit was the NSSLOWKEYDBHandleStr struct defined in keydbi.h. This is included in pcks11.c after pcks11p.h (which forces 1 byte struct alignment). Other modules do change the default aligned hence, a mismatch occurs. 

I saw this on Windows 2003 Server (64 bit) compiling with VS2005 (64 bit) - all latest service packs are applied.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

> The problem is that the same structures are compiled with different 
> alignment directives (#pragma pack on MSVC) in different C files.

Oh! :( That's Evil!  
Seems to me that pkcs11p.h should be included only from within other .h 
files, and not from within .c files.  

Hmm. I will retract that r-, and ask Bob to review.

Comment 7

[Robert Relyea]

Comment on attachment 272999 [details] [diff] [review]
per reporter...

r=rrelyea.

Agreed. this is definately evil and wrong and the correct fix to a bug I introduced 2 years ago.

good catch!

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Bob, is this change going to introduce any binary compatibility issues?
Will fixed code be compatible with DBs made by unfixed code?

Comment 9

[Robert Relyea]

No it shouldn't cause compatibility problems. The input interfaces are already packed by the pkcs11.h and pkcs11t.h header files.

DB's are written out by pcertdb.c. We never just write out a datastructure in anycase (that would make databases non-portable between different platforms).

The only affect are those datastructures defined after the include in pkcs11.c, which would be a mismatch between internal components of softoken. My guess is we are already compiling compact on windows 32 (otherwise we would have seen this problem already).

bob

Comment 10

[timeless]

bob: i don't have commit permission, could you please commit this?

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Before this can be committed, we need to decide where (on what branches)
it needs to be committed.

Comment 12

[Robert Relyea]

With one review, it certainly can be committed on the trunk.

Nelson, do you know of ports of NSS 3.11 (or other branches) to Windows 64-bit? The bug (fortunately) only affects Win 64.

My guess is if there were only ports of older versions of NSS to Win64, we would have heard about it by now.... so I would think that the 3_11 branch is the only other candidate.

bob

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 272999 [details] [diff] [review]
per reporter...

Yes, I think it makes sense to commit this on both the trunk and the 3.11 branch.  I'll try to do that today.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

On trunk:
Remove erroneous include of "pkcs11p.h".  Bug 388824.
Patch by Ian Marshall <[e-mail]>  R=rrelyea,nelson

Checking in pkcs11.c; new revision: 1.142; previous revision: 1.141

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Checking in pkcs11.c; new revision: 1.112.2.23; previous revision: 1.112.2.22