265369 - compiler warning in unix_rand

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P4)

Description

[Julien Pierre]

"unix_rand.c", line 831: warning: assignment type mismatch:
	pointer to const pointer to const char "=" pointer to pointer to char

The fix is to declare environ of the same type as cp .

Comment 1

[Julien Pierre]

Attachment: fix environ type

Comment 2

[Saul Edwards]

Comment on attachment 162793 [details] [diff] [review]
fix environ type

Looks OK.  I know it matches the Solaris type, but should we test on other
Unices?

Comment 3

[Wan-Teh Chang]

Julien,

environ should be declared as extern char **.
See http://www.opengroup.org/onlinepubs/009695399/functions/environ.html.

I've known about this compiler warning for a long
time, and never understood it.

Brendan, do you know why the compiler warns about
the assignment below?

    const char * const *cp;
    extern char **environ;

    cp = environ;

The warning is
        assignment type mismatch:
	pointer to const pointer to const char "=" pointer to pointer to char

Comment 4

[Julien Pierre]

Saul, Wan-Teh,

I don't believe there is any real problem with declaring environ the way I did.
At the C ABI level, const isn't differentiated on any platform that I know of,
so it shouldn't be an issue either at compile time or runtime.

The main issue is that we want the type on both sides of the assignment to
match. If we don't want to change the environ declaration (and it seems
reasonable to keep it that way, given Wan-Teh's response), then perhaps we
should change the cp declaration.

However, I agree the compiler warning is puzzling. One should be able to assign
a non-const variable to a const variable, just not the other way around, but we
are doing the former. Perhaps the compiler is being overzealous. I think the
problem is with pointers - it strictly differentiates two pointers by the type
to which they point. Is any other compiler complaining about this besides Sun
forte ? I'll look into this more in the afternoon.

Comment 5

[Wan-Teh Chang]

Julien,

The reason I don't want to declare environ as
const char * const * is that I am worried some
platforms may declare environ as char ** in a
system header file, which would result in
conflicting declarations.

This compiler warning is very common, not limited
to the Solaris compiler.  There are similar warnings
in lib/certdb/genname.c.

This is one of the few cases where I think the
right fix is a type cast:

    cp = (const char * const *)environ;

Comment 6

[Julien Pierre]

I have no idea why, but this patch causes a coredump on 64-bit sparccSolaris, as
if the environment was corrupt. Reverting the patch on my tree fixed the
problem. Just when I thought this couldn't get weirder, it does.

(/usr/dist/pkgs/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) where  
current thread: t@1
  [1] strlen(0x0, 0x7fffff5096100000, 0x0, 0x7efefeff, 0x81010100, 0x0), at
0x7fffffff7e23d2cc
=>[2] RNG_SystemInfoForRNG(), line 833 in "unix_rand.c"
  [3] nsc_CommonInitialize(pReserved = 0xffffffff7fffdcc8, isFIPS = 0), line
2781 in "pkcs11.c"
  [4] NSC_Initialize(pReserved = 0xffffffff7fffdcc8), line 2835 in "pkcs11.c"
  [5] secmod_ModuleInit(mod = 0x100059ec0), line 108 in "pk11load.c"
  [6] SECMOD_LoadPKCS11Module(mod = 0x100059ec0), line 263 in "pk11load.c"
  [7] SECMOD_LoadModule(modulespec = 0x10005fb70 " name="NSS Internal PKCS #11
Module" parameters="configdir='/home/jp96085/.netscape' certPrefix=''
keyPrefix='' secmod='secmod.db' flags=readOnly,optimizeSpace " NSS="trustOrder=0
cipherOrder=100
slotParams={0x00000001=[slotFlags=RSA,RC4,RC2,DES,DH,SHA1,MD5,MD2,SSL,TLS,AES,RANDOM
] }  Flags=internal,critical"", parent = 0x100059a30, recurse = 1), line 322 in
"pk11pars.c"
  [8] SECMOD_LoadModule(modulespec = 0x100058b00 "name="NSS Internal Module"
parameters="configdir='/home/jp96085/.netscape' certPrefix='' keyPrefix=''
secmod='secmod.db' flags=readOnly,optimizeSpace "
NSS="flags=internal,moduleDB,moduleDBOnly,critical"", parent = (nil), recurse =
1), line 337 in "pk11pars.c"
  [9] nss_Init(configdir = 0x10004bdd8 "/home/jp96085/.netscape", certPrefix =
0x7fffffff7f60b060 "", keyPrefix = 0x7fffffff7f60b068 "", secmodName =
0x7fffffff7f60b070 "secmod.db", readOnly = 1, noCertDB = 0, noModDB = 0,
forceOpen = 0, noRootInit = 0, optimizeSpace = 1), line 456 in "nssinit.c"
  [10] NSS_Init(configdir = 0x10004bdd8 "/home/jp96085/.netscape"), line 493 in
"nssinit.c"
  [11] main(argc = 4, argv = 0xffffffff7fffec78), line 214 in "shlibsign.c"
(/usr/dist/pkgs/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) p cp
cp = 0x7fffffff7e066518
(/usr/dist/pkgs/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) p *cp
*cp = 0x7fffff5096100000 ""
(/usr/dist/pkgs/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) 

Comment 7

[Wan-Teh Chang]

Comment on attachment 162793 [details] [diff] [review]
fix environ type

Julien, were you referring to this patch
in comment 6?

Comment 8

[Julien Pierre]

Wan-Teh,

Yes, I was referring to the patch
https://bugzilla.mozilla.org/attachment.cgi?id=162793 . The problem turned out
to be a silly mistake - environ was redeclared as a local in the patch, rather
than extern, so it had an uninitialized value.

Comment 9

[Julien Pierre]

Attachment: just cast it, which will ensure no compiler warnings

Comment 10

[Wan-Teh Chang]

Comment on attachment 182834 [details] [diff] [review]
just cast it, which will ensure no compiler warnings

Julien, Nelson should review this patch because it was
him who added those 'const' keywords (in rev. 1.2 of
unix_rand.c).  Note that lib/certdb/genname.c has similar
compiler warnings and should be fixed at the same time.

If Nelson doesn't like casting, here is one way to help
the compiler out: we add only one 'const' at a time.
This requires an extra local variable.

Index: unix_rand.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v
retrieving revision 1.16
diff -u -r1.16 unix_rand.c
--- unix_rand.c 8 Dec 2004 23:00:19 -0000	1.16
+++ unix_rand.c 9 May 2005 21:28:19 -0000
@@ -833,12 +833,18 @@
      * is running on.
      */
     if (environ != NULL) {
-	 cp = environ;
-	 while (*cp) {
-	    RNG_RandomUpdate(*cp, strlen(*cp));
-	    cp++;
+	 /*
+	  * Compilers can't go from char ** to const char * const * or
+	  * even const char ** directly, so we need to do it in two steps.
+	  */
+	 char * const *evp;
+	 const char *ev;
+	 evp = environ;
+	 while ((ev = *evp) != NULL) {
+	    RNG_RandomUpdate(ev, strlen(ev));
+	    evp++;
	 }
-	 RNG_RandomUpdate(environ, (char*)cp - (char*)environ);
+	 RNG_RandomUpdate(environ, (char*)evp - (char*)environ);
     }

     /* Give in system information */

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182834 [details] [diff] [review]
just cast it, which will ensure no compiler warnings

r=nelson.bolyard

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 182834 [details] [diff] [review]
just cast it, which will ensure no compiler warnings

really r=nelson this time.

Comment 13

[Julien Pierre]

Nelson, thanks for the review. I have checked this fix in to the tip .

Checking in unix_rand.c;
/cvsroot/mozilla/security/nss/lib/freebl/unix_rand.c,v  <--  unix_rand.c
new revision: 1.17; previous revision: 1.16
done