237068 - certutil crashes when NSS is built on Solaris 5.9 Sparc

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

This problem only occurs in the 3.3.x branches of NSS, where certutil is
statically linked with the rest of NSS .

The symptom is that doing "certutil -d . -L" dumps core in a bad way, filling up
all available memory.

The dynamic libraries appear to work fine, though.

I narrowed down the problem to some code in lib/freebl that explicitly checks
for specific versions of Solaris, using the OS_RELEASE variable in the Makefile,
and the SOLARIS2_x macros in sparcfix.c . I'm not sure why the problem only
shows in static versions of certutil, but we probably should fix this in all
currently maintained branches of NSS.

I will attach two patches because I'm unsure which one is best
.
1) a patch that adds support for building under Solaris 9 / Sparc.
I'm not sure if this is the best patch. I don't think we should have to add
anything to the build for future versions of Solaris (S10 is under works, for
instance).

2) a patch that unconditionally uses the correct settings for Solaris 5.5.1 and
later. The logic is that Sun doesn't support anything older than 2.6 anyway.

Comment 1

[Julien Pierre]

Taking bug.

Comment 2

[Julien Pierre]

Attachment: first patch - just add support for Solaris 9 (SunOS5.9)

Comment 3

[Julien Pierre]

Attachment: alternative patch - remove unneeded conditional code and rules

Comment 4

[glen beasley]

Comment on attachment 143557 [details] [diff] [review]
alternative patch - remove unneeded conditional code and rules

The alternative patch is the better approach, it should work for 5.10 when it
comes out as well.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Questions and comments:

1. The worst that should happen on an unrecognized version of Solaris now is
that (a) it only builds the code using the old 32-bit spark v8 instruction set,
and does not also build the v8+ version, and (b) when it runs, it only runs
the "pure32" v8 code, not the v8+ code.  

So, I would like to understand the crash better.  Why does this crash?
Does solaris 2_9 not support the old v8 stuff?  
Do the newest ultraSparc CPUs not support the old v8 instructions?

2. Is there no longer a requirement to be able to build NSS on Sparc
(non-ultra) systems?   I ask this because the second patch would cause NSS
to always be built for both V8 and V8+, implying that it always is built on
systems capable of v8+, which also implies (I believe) that it will be RUN
on systems capable of v8+ (that is, UltraSparcs).  

If that is true, if there is no longer a requirement to build and run on 
v8 (not plus) CPUs, then I'd propose a bigger change than the second patch
above makes.  I'd propose a change that stops building the old "pure32" 
code completely, and always builds and links with the v8+ code.  

Comment 6

[Julien Pierre]

Nelson,

The symptom of the crash is a recursive loop in RNG_RNGInit(), as called through
NSS_Initialize, from certutil . This eventually fills up memory, and dumps a
huge core.

(.../dist/share/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) w   
current thread: t@1
=>[1] RNG_RNGInit(), line 881 in "loader.c"
  [2] RNG_RNGInit(), line 883 in "loader.c"
  [3] RNG_RNGInit(), line 883 in "loader.c"
  [4] nss_Init(configdir = 0x1f98f8 ".", certPrefix = 0x1e773c "", keyPrefix =
0x1e773c "", secmodName = 0x1c0a34 "secmod.db", readOnly = 0, noCertDB = 0,
noModDB = 0, forceOpen = 0), line 254 in "nssinit.c"
  [5] NSS_Initialize(configdir = 0x1f98f8 ".", certPrefix = 0x1e773c "",
keyPrefix = 0x1e773c "", secmodName = 0x1c0a34 "secmod.db", flags = 0), line 348
in "nssinit.c"
  [6] main(argc = 4, argv = 0xffbfef84), line 2492 in "certutil.c"
(.../dist/share/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) up  
Current function is RNG_RNGInit
  883     return (vector->p_RNG_RNGInit)();
(.../dist/share/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) file
loader.c
(.../dist/share/forte_dev,v6.2/SUNWspro/bin/../WS6U2/bin/sparcv9/dbx) 

It seems the problem is that vector->p_RNG_RNGInit() points back to the code in
loader.c . Why that is the case, I don't exactly know.

This loop only occurs when NSS is linked statically to the program, so you can
only reproduce it on the 3.3 branch or older . 

If you have a Solaris box, you don't need a 2.9 box to see the crash - you can
get it on 2.8 too if you set OS_RELEASE to 5.9 in coreconf/arch.mk without patches.

From what I can tell in the freebl Makefile, the SYSV_SPARC variable only
depends on the Solaris version - it is independent from the CPU type. I don't
believe the requirement for supporting older CPUs has changed at all, only the
requirement for supporting older Solaris versions, prior to SunOS 5.5.1 where
SYSV_SPARC was not set. It's quite possible this case never worked properly. I
don't personally recall ever using any version of Solaris prior to 2.6.

Comment 7

[Wan-Teh Chang]

Comment on attachment 143550 [details] [diff] [review]
first patch - just add support for Solaris 9 (SunOS5.9)

1. mozilla/security/nss/lib/freebl/Makefile

>@@ -152,6 +152,9 @@
> ifeq ($(OS_RELEASE),5.8)
>     SYSV_SPARC = 1
> endif
>+ifeq ($(OS_RELEASE),5.9)
>+    SYSV_SPARC = 1
>+endif

The NSS tip has a better fix for this:

ifeq (5.5.1,$(firstword $(sort 5.5.1 $(OS_RELEASE))))
    SYSV_SPARC = 1
endif

2. mozilla/security/nss/lib/freebl/sparcfix.c

This file is obsolete in NSS 3.2 or later.

>-#if defined(SOLARIS2_6) || defined(SOLARIS2_7) || defined(SOLARIS2_8)
>+#if defined(SOLARIS2_6) || defined(SOLARIS2_7) || defined(SOLARIS2_8) || defined(SOLARIS2_9)
> #define NEW_SYSV_SPARC 1
> #include <gelf.h>
> #endif

A better fix is to add -DNEW_SYSV_SPARC to the C
flags if $(OS_RELEASE) is >= 5.6 in lib/freebl/Makefile.

Comment 8

[Julien Pierre]

Wan-Teh,

I actually hadn't noticed the fix in the tip. It's certainly cleaner the way you
suggest. I will generate a 3rd partch ;)

Comment 9

[Wan-Teh Chang]

SYSV_SPARC is set if OS_TARGET is SunOS, CPU_ARCH
is sparc, and OS_RELEASE is 5.5.1 or higher.  So
it is dependent on the CPU type.

Comment 10

[Julien Pierre]

Wan-Teh,

Correct. I meant it's not dependent on a particular revision of the Sparc
architecture of chips, 32 or 64 bits, v8 or v9.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Hmmm.  Are you staticly linking freebl into certutil?
In that case, loader.o should not be linked in at all.  
loader.o should only be linked in when you're not staticly linking the 
freebl DSO to the application, and are loading it dynamically instead.  
If you're linking both freebl and loader.o, I would expect you'd see lots of 
linker errors or warnings about duplicate entries/symbols.

Comment 12

[Julien Pierre]

No. It appears libfreebl*.so still get loaded dynamically even from certutil.
But the code that does the loading is linked in the certutil process, not in
libnss3.so .

Comment 13

[Julien Pierre]

Attachment: patch using Wan-Teh's suggestions

Comment 14

[Julien Pierre]

checked in to SUN_SECURITY_3_3_BRANCH

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.34.2.2.2.1; previous revision: 1.34.2.2
done
Checking in sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/sparcfix.c,v  <--  sparcfix.c
new revision: 1.1.224.1; previous revision: 1.1
done

As well as NSS_3_3_4_1_BRANCH :

Checking in Makefile;
/cvsroot/mozilla/security/nss/lib/freebl/Makefile,v  <--  Makefile
new revision: 1.34.2.1.20.2; previous revision: 1.34.2.1.20.1
done
Checking in sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/sparcfix.c,v  <--  sparcfix.c
new revision: 1.1.242.1; previous revision: 1.1
done

Comment 15

[Julien Pierre]

Responding to comment #7 :
I read your comment too quickly today, and didn't realize Sparcfix wasn't even
used even in the 3.3 branch - I thought its use got dropped later
So it looks like I patched that one for nothing, since we don't use it after
all. Since this source file is no longer used, I will cvs remove it from all
currently maintained branches.

SUN_SECURITY_3_3_BRANCH .

Removing sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/sparcfix.c,v  <--  sparcfix.c
new revision: delete; previous revision: 1.1.224.1
done

NSS_3_3_4_1_BRANCH :
Removing sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/sparcfix.c,v  <--  sparcfix.c
new revision: delete; previous revision: 1.1.242.1
done

tip :

Removing sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/sparcfix.c,v  <--  sparcfix.c
new revision: delete; previous revision: 1.1
done


NSS_3_9_BRANCH :

Removing sparcfix.c;
/cvsroot/mozilla/security/nss/lib/freebl/Attic/sparcfix.c,v  <--  sparcfix.c
new revision: delete; previous revision: 1.1.258
done

Comment 16

[Wan-Teh Chang]

Comment on attachment 143578 [details] [diff] [review]
patch using Wan-Teh's suggestions

>+ifeq (5.5.1,$(firstword $(sort 5.5.1 $(OS_RELEASE))))
>     SYSV_SPARC = 1
>+    OS_CFLAGS += /DNEW_SYSV_SPARC
> endif

This code has two problems.  1. /DNEW_SYSV_SPARC should
not work on Solaris.  2. To be fully equivalent to the
original code, you should add -DNEW_SYSV_SPARC only if
OS_RELEASE is 5.6 or higher.

Since sparcfix.c is obsolete, I suggest a different
solution: just cvs remove it and not worry about updating
it for new Solaris releases.

Comment 17

[Julien Pierre]

Wan-Teh,

I just removed sparcfix.c last night - we had the same idea. I thought this code
played a role because seemingly the only compiler difference between the 5.8 and
5.9 builds was the defines in SunOS5.8.mk and SunOS5.9.mk differences. The other
difference as OS_RELEASE set to 5.8, and tested in the freebl Makefile .

You are correct that /DNEW_SYSV_SPARC does not work on Solaris . But apparently,
when used in conjunction with -c, the compiler ignores it silently .This is no
longer relevant, since the only file that used the macro was sparcfix.c, and
it's no longer there. I have just removed the OS_CFLAGS += /DNEW_SYSV_SPARC from
the 3.3.8 and 3.3.4.1 Makefiles .