Closed Bug 333917 Opened 15 years ago Closed 15 years ago
The non-x86 code in desblapi
.c seems to violate ANSI C strict aliasing rules
83.71 KB, text/html
286.49 KB, text/plain
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.
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)
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?
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).
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 and vec) to access 'vec' and the COPY8B macro may use a BYTE * or SHORT * pointer to access 'vec'.
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+
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: 22.214.171.124; previous revision: 126.96.36.199 done
Attachment #218345 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.