171263 - All NSS test apps should check the return value of NSS_Shutdown

Status: RESOLVED FIXED

(NSS :: Test, defect, P1)

Description

[Stephane Saux]

If NSS_Shutdown is not called on all the paths, it should be added.
If NSS_Shutdown fails (e.g., reference leaks) it should fail the test.
This simple methodology should greatly improve the value of the tinderbox
regression testing.

Comment 1

[Stephane Saux]

cc wtc. I thought he would be the owner.

Comment 2

[Robert Relyea]

setting NSS_STRICT_SHUTDOWN will give us some of the benefit immediately. Debug
builds will assert if you set this environment variable and try to Shutdown with
open references.

It's still probably a good idea to have the test cases explicitly check the
return code from Shutdown() to 1) make sure all test cases are actually calling
shutdown, 2) detect problem in the optimized builds, 3) detect problems when
developers hand run the regression tests.

Just a Note: we can't depend on this to be 100% we have all the leaks. To date,
the regression tests continue to work correctly with NSS_STRICT_SHUTDOWN on,
even though we have found 2 leaks since then because the test coverage did not
cover those scenarios.

bob

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Whether it succeeds or fails, NSS_Shutdown always clears the static 
nss_IsInitted flag, as if it had succeeded.  Is that the right thing to do?

Comment 4

[Bishakha Banerjee]

Based on a discussion with Bob and others at the NSS meeting, setting
NSS_STRICT_SHUTDOWN in all.sh
Please review.

haddock:/u/bishakhabanerjee/mozilla/security/nss/tests 4% cvs diff -u all.sh
Index: all.sh
===================================================================
RCS file: /cvsroot/mozilla/security/nss/tests/all.sh,v
retrieving revision 1.17
diff -u -r1.17 all.sh
--- all.sh      10 Oct 2002 20:36:49 -0000      1.17
+++ all.sh      17 Oct 2002 17:40:23 -0000
@@ -75,6 +75,7 @@

 TESTS="cert ssl sdr cipher smime perf tools fips dbtests"
 SCRIPTNAME=all.sh
+NSS_STRICT_SHUTDOWN=1
 CLEANUP="${SCRIPTNAME}"
 cd `dirname $0`        # will cause problems if sourced

Comment 5

[Wan-Teh Chang]

Bishakha,

I believe you also need "export NSS_STRICT_SHUTDOWN".

This patch is a good first step but more work is needed.
Setting NSS_STRICT_SHUTDOWN is useless if the test
program is not calling NSS_Shutdown.  Also, NSS_STRICT_SHUTDOWN
only has an effect on debug builds of NSS.

What you need to do is to examine all the test programs
(certutil, signtool, cmsutil, selfserv, strsclnt, tstclnt,
to name a few) and make sure that they call NSS_Shutdown
and check the return value.  If NSS_Shutdown returns
SECFailure, the test programs should exit with a nonzero
status.  For example
    if (NSS_Shutdown() == SECFailure) {
        exit(1);
    }

The exact code may depend on the test programs.

Comment 6

[Bishakha Banerjee]

Attachment: patch of apps/tests that already called NSS_Shutdown() but did not check returned value

Comment 7

[Bishakha Banerjee]

Attachment: patch of apps/tests that call NSS_Init*, does not call exit() on its own,

Comment 8

[Bishakha Banerjee]

Attachment: revised patch of all.sh

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I suggest that, rather than testing that the return is == SECFailure,
the test should be != SECSuccess .

Comment 10

[Wan-Teh Chang]

Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.

Comment 11

[Bishakha Banerjee]

Attachment: revised patch of attachment id=107185

Comment 12

[Bishakha Banerjee]

Attachment: revised patch to attachment id=107186

Incorporates Nelson's suggestion.

Comment 13

[Wan-Teh Chang]

Comment on attachment 109488 [details] [diff] [review]
revised patch of attachment id=107185

r=wtc.	This patch is good.

Comment 14

[Wan-Teh Chang]

Comment on attachment 107187 [details] [diff] [review]
revised patch of all.sh

It is better to set NSS_STRICT_SHUTDOWN in
common/init.sh because it is included by not
only all.sh but also the individual test
scripts (cert/cert.sh, ssl/ssl.sh, smime/smime.sh,
etc.).

Comment 15

[Wan-Teh Chang]

Comment on attachment 109490 [details] [diff] [review]
revised patch to attachment id=107186

Most of the changes in this patch are good, but some
require more work.

1. There are two issues with blapitest.c.  First, it
does not call any NSS initialization function.	(It
calls RNG_RNGInit.)  So I'm not sure if it is even
appropriate to call NSS_Shutdown.  Second, its main()
function has multiple return statements.  All of them
would need to call NSS_Shutdown.  For example,

	return blapi_selftest(...);

would need to be changed to something like

	SECStatus rv;  (already declared in main)

	rv = blapi_selftest(...);
	if (NSS_Shutdown() != SECSuccess) {
	    exit(1);
	}
	return rv;

Please ask Bob, Ian, or Nelson if blapitest.c may
call NSS_Shutdown and whether it should be changed
to call NSS_NoDB_Init instead of RNG_RNGInit.

2. In dbtest.c, we should call NSS_Shutdown only if
the NSS_Initialize call succeeded.

3. Although we have a preferred indentation style,
you should follow the prevalent style in the file
you are modifying, even if it is different from our
standard style.

So in signtool.c, you should indent with one tab
character.  In sslstrength.c, you should indent with
two spaces.

You can go ahead and check in the changes for the
files other than the four named above.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

blapitest should use no part of NSS except blapi/freebl.  
blapitest should be able to run with just the freebl library, 
not needing any other NSS libraries.
blapitest does not initialize NSS, because it doesn't use it.
blapitest does not shutdown NSS because it doesn't use NSS.

So, blapitest should be removed from the list of programs to which 
NSS_Shutdown calls will be added.

Comment 17

[Bishakha Banerjee]

Attachment: patch

Corrected indentation style to follow conventions in signtool.c, sslstrength.c,
and dbtest.c. 

Also changed dbtest.c to call NSS_Shutdown only when NSS_Initialize call
succeded

Comment 18

[Bishakha Banerjee]

Changes to all other files reviewed okay checked in:

Checking in client.c;
/cvsroot/mozilla/security/nss/cmd/SSLsample/client.c,v  <--  client.c
new revision: 1.5; previous revision: 1.4

Checking in server.c;
/cvsroot/mozilla/security/nss/cmd/SSLsample/server.c,v  <--  server.c
new revision: 1.4; previous revision: 1.3

Checking in addbuiltin.c;
/cvsroot/mozilla/security/nss/cmd/addbuiltin/addbuiltin.c,v  <--  addbuiltin.c
new revision: 1.6; previous revision: 1.5

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.70; previous revision: 1.69

Checking in crlutil.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.19; previous revision: 1.18

Checking in modutil.c;
/cvsroot/mozilla/security/nss/cmd/modutil/modutil.c,v  <--  modutil.c
new revision: 1.16; previous revision: 1.15

Checking in strsclnt.c;
/cvsroot/mozilla/security/nss/cmd/strsclnt/strsclnt.c,v  <--  strsclnt.c
new revision: 1.27; previous revision: 1.26

Checking in tstclnt.c;
/cvsroot/mozilla/security/nss/cmd/tstclnt/tstclnt.c,v  <--  tstclnt.c
new revision: 1.23; previous revision: 1.22

Checking in vfyserv.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyserv.c,v  <--  vfyserv.c
new revision: 1.5; previous revision: 1.4

Checking in certcgi.c;
/cvsroot/mozilla/security/nss/cmd/certcgi/certcgi.c,v  <--  certcgi.c
new revision: 1.11; previous revision: 1.10

Checking in digest.c;
/cvsroot/mozilla/security/nss/cmd/digest/digest.c,v  <--  digest.c
new revision: 1.3; previous revision: 1.2

Checking in p7sign.c;
/cvsroot/mozilla/security/nss/cmd/p7sign/p7sign.c,v  <--  p7sign.c
new revision: 1.8; previous revision: 1.7

Checking in p7content.c;
/cvsroot/mozilla/security/nss/cmd/p7content/p7content.c,v  <--  p7content.c
new revision: 1.8; previous revision: 1.7

Checking in p7verify.c;
/cvsroot/mozilla/security/nss/cmd/p7verify/p7verify.c,v  <--  p7verify.c
new revision: 1.8; previous revision: 1.7

Checking in rsaperf.c;
/cvsroot/mozilla/security/nss/cmd/rsaperf/rsaperf.c,v  <--  rsaperf.c
new revision: 1.7; previous revision: 1.6

Checking in signver.c;
/cvsroot/mozilla/security/nss/cmd/signver/signver.c,v  <--  signver.c
new revision: 1.7; previous revision: 1.6

Comment 19

[Bishakha Banerjee]

Also checked in approved changes to tests/common/init.sh.  New version is 1.35

Index: init.sh
===================================================================
RCS file: /cvsroot/mozilla/security/nss/tests/common/init.sh,v
retrieving revision 1.34
retrieving revision 1.35
diff -r1.34 -r1.35
71c71,72
<
---
> NSS_STRICT_SHUTDOWN=1
> export NSS_STRICT_SHUTDOWN

Comment 20

[Bishakha Banerjee]

Attachment: correct patch - revisions to patch 107186

oops, forgot to update dbtest.c in last patch submitted. Here's the correct
one.

Comment 21

[Wan-Teh Chang]

Comment on attachment 110900 [details] [diff] [review]
correct patch - revisions to patch 107186

>Index: dbtest/dbtest.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/cmd/dbtest/dbtest.c,v
>retrieving revision 1.2
>diff -u -r1.2 dbtest.c
>--- dbtest.c    7 Dec 2001 03:18:23 -0000       1.2
>+++ dbtest.c    7 Jan 2003 23:21:00 -0000
>@@ -173,6 +173,11 @@
>     }
>
> loser:
>+    if (rv == SECSuccess) {
>+        if (NSS_Shutdown() != SECSuccess) {
>+            exit(1);
>+        }
>+    }
>     return ret;
> }

This change is incorrect because 'rv' may be uninitialized
at this point.

It is best to call NSS_Shutdown here:

    rv = NSS_Initialize(SECU_ConfigDirectory(dbDir), dbprefix, dbprefix,
		   secmodName, flags);
    if (rv != SECSuccess) {
	SECU_PrintPRandOSError(progName);
	ret=NSS_INITIALIZE_FAILED_ERR;
    } else {
+	if (NSS_Shutdown() != SECSuccess) {
+	    exit(1);
+	}
	ret=SUCCESS;
    }

The changes to signtool.c and sslstrength.c are fine.
I think in signtool.c it is safer to call NSS_Shutdown()
only if 'retval' is 0 (success).

Comment 22

[Bishakha Banerjee]

Checked in reviewed/approved changes to dbtest.c and sslstrength.c

Checking in sslstrength.c;
/cvsroot/mozilla/security/nss/cmd/sslstrength/sslstrength.c,v  <--  sslstrength.c
new revision: 1.5; previous revision: 1.4
done

Checking in dbtest.c;
/cvsroot/mozilla/security/nss/cmd/dbtest/dbtest.c,v  <--  dbtest.c
new revision: 1.3; previous revision: 1.2
done

Comment 23

[Bishakha Banerjee]

Attachment: enhancement to patch for signtool.c

Adding Wan-Teh's enhancement of calling NSS_Shutdown only if retval=0

Comment 24

[Wan-Teh Chang]

Attachment: patch for signtool.c

The previous patch (attachment 113012 [details] [diff] [review]) contains some
extraneous things.  This patch corrects that.  I've
checked it in.

Note that this patch causes two NSS tests to fail.
Apparently it exposes a leak of NSS objects.

Comment 25

[Wan-Teh Chang]

All patches have been checked in.  Marked the bug fixed.

Comment 26

[Bishakha Banerjee]

Reopening bug. Additional NSS cmds need this fix based on new information. Patch
follows.

Comment 27

[Bishakha Banerjee]

Attachment: additional cmds that need fix based on Nelson's comment

Comment 28

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 118980 [details] [diff] [review]
additional cmds that need fix based on Nelson's comment

This patch fixes several, but still not all, of the remaining calls to
NSS_Shutdown in the NSS commands.  I'll add more comments below.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

The patch in attachment id=118980 fixes one of the two NSS_Shutdown calls in
nss/cmd/vfychain/cfychain.c.  It should fix both.

That patch does not appear to fix the NSS_Shutdown calls in 
nss/cmd/crlutil/crlutil.c
nss/cmd/ocspclnt/ocspclnt.c
nss/cmd/sdrtest/sdrtest.c
nss/cmd/tests/remtest.c  

Comment 30

[Bishakha Banerjee]

|The patch in attachment id=118980 fixes one of the two NSS_Shutdown calls in
|nss/cmd/vfychain/cfychain.c.  It should fix both.
|

One of these NSS_Shutdown calls is immediately followed by an exit(1). Hence,
and also at Wan-Teh's verbal suggestion, ignoring the return value of
NSS_Shutdown by casting to (void).

Also reworking the patches for SSLsample.c/sslsample.c and vfyserv/vfyutil.c
from attachment id=118980 for same reasons.

|That patch does not appear to fix the NSS_Shutdown calls in 
|nss/cmd/crlutil/crlutil.c
|nss/cmd/ocspclnt/ocspclnt.c
|nss/cmd/sdrtest/sdrtest.c
|nss/cmd/tests/remtest.c  

Attaching revamped patch for crlutil/crlutil.c

ocspclnt/ocspclnt.c and sdrtest/sdrtest.c are part of a previous approved patch
-attachment 109488 [details] [diff] [review] (as mentioned in my email to you) that will be checked in
alongwith the rest of these patches. However, attaching these patches again.

tests/remtest.c was fixed in attachment id=118980

Comment 31

[Bishakha Banerjee]

Attachment: patch

Comment 32

[Wan-Teh Chang]

Comment on attachment 119081 [details] [diff] [review]
patch

In crlutil/crlutil.c, we have

>-       NSS_Shutdown();
>+       if (NSS_Shutdown() != SECSuccess) {
>+           exit(1);
>+       }
>        return (-1);

This one can be replaced by a (void) cast as well because
the main() function will return -1 anyway, which is equivalent
to an exit(-1) call.

All the other changes are fine.

Comment 33

[Bishakha Banerjee]

Attachment: patch for crlutil/crlutil.c

ignore return value of NSS_Shutdown()

Comment 34

[Bishakha Banerjee]

Checked in patches. Marking fixed.

Checking in ocspclnt.c;
/cvsroot/mozilla/security/nss/cmd/ocspclnt/ocspclnt.c,v  <--  ocspclnt.c
new revision: 1.5; previous revision: 1.4
done

Checking in sslsample.c;
/cvsroot/mozilla/security/nss/cmd/SSLsample/sslsample.c,v  <--  sslsample.c
new revision: 1.6; previous revision: 1.5
done


Checking in sdrtest.c;
/cvsroot/mozilla/security/nss/cmd/sdrtest/sdrtest.c,v  <--  sdrtest.c
new revision: 1.9; previous revision: 1.8
done

Checking in vfyutil.c;
/cvsroot/mozilla/security/nss/cmd/vfyserv/vfyutil.c,v  <--  vfyutil.c
new revision: 1.5; previous revision: 1.4
done


Checking in vfychain.c;
/cvsroot/mozilla/security/nss/cmd/vfychain/vfychain.c,v  <--  vfychain.c
new revision: 1.3; previous revision: 1.2
done

Checking in pk12util.c;
/cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v  <--  pk12util.c
new revision: 1.25; previous revision: 1.24
done

Checking in pkiutil.c;
/cvsroot/mozilla/security/nss/cmd/pkiutil/pkiutil.c,v  <--  pkiutil.c
new revision: 1.3; previous revision: 1.2
done

Checking in shlibsign.c;
/cvsroot/mozilla/security/nss/cmd/shlibsign/shlibsign.c,v  <--  shlibsign.c
new revision: 1.10; previous revision: 1.9
done

Checking in remtest.c;
/cvsroot/mozilla/security/nss/cmd/tests/remtest.c,v  <--  remtest.c
new revision: 1.2; previous revision: 1.1
done

Checking in cmsutil.c;
/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.41; previous revision: 1.40
done


Checking in crlutil.c;
/cvsroot/mozilla/security/nss/cmd/crlutil/crlutil.c,v  <--  crlutil.c
new revision: 1.20; previous revision: 1.19
done

Grep for NSS_Shutdown() in cmd/* shows every call either checks return value or
explicitly ignores return value.