Closed
Bug 333917
Opened 18 years ago
Closed 18 years ago
The non-x86 code in desblapi.c seems to violate ANSI C strict aliasing rules
Categories
(NSS :: Libraries, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.2
People
(Reporter: wtc, Assigned: wtc)
Details
Attachments
(4 files, 1 obsolete file)
83.71 KB,
text/html
|
Details | |
286.49 KB,
text/plain
|
Details | |
1.47 KB,
patch
|
Details | Diff | Splinter Review | |
844 bytes,
patch
|
Details | Diff | Splinter Review |
The 64-bit Linux x86_64 optimized build on NSS trunk (3.12) and NSS_3_11_BRANCH (3.11.1) fails the "DES CBC Encrypt" and "DES3 CBC Encrypt" tests in the "Cipher Tests" section (cipher.sh) of all.sh. The debug build doesn't have these test failures. The 32-bit Linux x86 debug and optimized builds don't have these test failures. By trial and error, I found that if I use GCC's -fno-strict-aliasing option on just the lib/freebl/desblapi.c file, the 64-bit Linux x86_64 optimized build passes all the tests. The problematic code is likely to be the default (non-x86) definition of the COPY8B macro in lib/freebl/desblapi.c, which casts a HALF pointer to a SHORT or BYTE pointer.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Comment 2•18 years ago
|
||
This patch disallows the compiler (GCC) to assume the strictest aliasing rules when compiling desblapi.c on Linux non-x86. I recommend this workaround on the NSS_3_11_BRANCH to save time and to avoid re-doing the FIPS Triple DES algorithm testing. On the trunk we can try to fix the code.
Attachment #218345 -
Flags: review?(nelson)
Comment 3•18 years ago
|
||
Wan-Teh, help me understand this bug. It fails the TESTS? Are there any compiler warnings? What is "wrong" with the optimized code? What optimization is it doing that is causing wrong (er, undesired) results?
Assignee | ||
Comment 4•18 years ago
|
||
The 64-bit (USE_64=1) optimized (BUILD_OPT=1) build done on Linux x86_64 fails two tests in cipher.sh. There are no compiler warnings, even if I compile desblapi.c with the -fstrict-aliasing -Wstrict-aliasing flags. I did not inspect the optimized code (I can't debug at that level, unlike you).
Assignee | ||
Comment 5•18 years ago
|
||
This patch is only intended to point out the location of the aliasing bugs that make us fail those two tests in cipher.sh. This patch is not a bug fix, and it does not try to point out all the aliasing bugs in that file. So do not check this in. The mere act of declaring 'vec' as a union is enough to fix the test failures. The aliasing rules state that pointers to different types (e.g., HALF * and BYTE *) should not point to the same object. These two functions break the aliasing rules because the functions use a HALF * pointer (vec[0] and vec[1]) to access 'vec' and the COPY8B macro may use a BYTE * or SHORT * pointer to access 'vec'.
Comment 6•18 years ago
|
||
Comment on attachment 218345 [details] [diff] [review] Proposed workaround First, thanks for explaining this to me. I believe this patch is correct, but may I suggest adding -fno-strict-aliasing to CFLAGS in all our gcc builds, and not only for this particular OS/CPU combination?
Attachment #218345 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 7•18 years ago
|
||
I checked in the GCC -fno-strict-aliasing workaround on the NSS trunk (3.12) and NSS_3_11_BRANCH (3.11.1). I put the -fno-strict-aliasing flag after $(CFLAGS) just to be safe, in case the ordering of conflicting optimization flags is important. Checking in Makefile; /cvsroot/mozilla/security/nss/lib/freebl/Makefile,v <-- Makefile new revision: 1.79; previous revision: 1.78 done Checking in Makefile; /cvsroot/mozilla/security/nss/lib/freebl/Makefile,v <-- Makefile new revision: 1.70.2.6; previous revision: 1.70.2.5 done
Attachment #218345 -
Attachment is obsolete: true
Comment 8•18 years ago
|
||
I'm in the process of marking this bug fixed. But first I'm reassigning it to Wan-Teh, who fixed it.
Assignee: nobody → wtchang
Component: Test → Libraries
Priority: -- → P2
Target Milestone: --- → 3.11.2
Updated•18 years ago
|
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•