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

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

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.

Comment 1

[Wan-Teh Chang]

Attachment: results.html

Comment 2

[Wan-Teh Chang]

Attachment: Proposed workaround

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.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 4

[Wan-Teh Chang]

Attachment: Subset of output.log generated by cipher.sh

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

Comment 5

[Wan-Teh Chang]

Attachment: Patch to point out the aliasing bugs (do not check it in)

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

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 7

[Wan-Teh Chang]

Attachment: Proposed workaround (as checked in)

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

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I'm in the process of marking this bug fixed.  But first I'm reassigning it
to Wan-Teh, who fixed it.