Closed Bug 333917 Opened 15 years ago Closed 14 years ago

The non-x86 code in desblapi.c seems to violate ANSI C strict aliasing rules

Categories

(NSS :: Libraries, defect, P2)

3.11
All
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.2

People

(Reporter: wtc, Assigned: wtc)

Details

Attachments

(4 files, 1 obsolete file)

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.
Attached file results.html
Attached patch Proposed workaround (obsolete) — Splinter Review
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[0] and vec[1]) 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: 1.70.2.6; previous revision: 1.70.2.5
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: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.