301496 - NSS_Shutdown failure in p7sign

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Alexei Volkov]

p7sign signs file correctly. The file can be late verified with p7verify.
But p7sign ties to shutdown nss NSS_Shutdown() returns the following error:

NSS could not shutdown. Objects are still in use.

The problem was missed ealier due to absents of SECU_PrintError call in case of
error.

 --- p7sign.c	2005-07-20 14:28:48.000000000 -0700
***************
*** 287,292 ****
--- 287,293 ----
      }
  
      if (NSS_Shutdown() != SECSuccess) {
+         SECU_PrintError(progName, "NSS shutdown:");
          exit(1);
      }

Comment 1

[Alexei Volkov]

The following error is reported if  NSS_STRICT_SHUTDOWN is set.

p7sign -d . -k Alice -i file.in -o file.out -p nss
Assertion failure: secmod_PrivateModuleCount == 0, at pk11util.c:119
asn1reg.sh: line 170:  8516 Aborted                 (core dumped) $*
asn1reg.sh ERROR: Signing file for user Alice p7sign failed 134

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Shutdown failures almost always are caused by object refrence leaks.
In this case, the leak could be in the p7sign test program or in the
older SEC_PKCS7 library functions.  

Do we routinely test any of the old p7* test programs in all.sh? 
I'll guess we don't.

Comment 3

[Alexei Volkov]

> Do we routinely test any of the old p7* test programs in all.sh? 
> I'll guess we don't.

We do not have any p7* tests we run by all.sh.
I wrote a couple simple tests recently. Will put them in to the tree ones we
done with asn1 patch tests.

Comment 4

[Alexei Volkov]

Attachment: clean before calling NSS_Shutdown

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 241737 [details] [diff] [review]
clean before calling NSS_Shutdown 

Alexei,  in comment 0 you had a small patch that output an
error message if the program did not shut down cleanly.
I think that change should be part of the patch you check in.  
Also, might as well set rv, rather than exiting, if shutdown
fails.  

Please assign SECFailure to rv, rather than -1, in all the 
places where your patch now assigns -1 to it.
At the end, instead of returning rv, return (rv != SECSuccess);
That way, main will return 0 (success) or 1 (failure).

Comment 6

[Alexei Volkov]

Attachment: use SECFailure instead of -1 error code

All nss commands use the same pattern the handles NSS_Shutdown error. They exit with error code set to 1 without warning a user. I prefer p7sign handles it in a similar way. If we need to change it, let's change it across the whole set, as a work related to a separate bug..

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 241765 [details] [diff] [review]
use SECFailure instead of -1 error code 

I'm OK with not outputting a message about the shutdown failure,
PROVIDED that the test scritp that runs this program doesn't ignore the non-zero return value from the process, which it definitely was (probably still is) doing.  This failure was going on for YEARS, and was unnoticed until I happened to run it in a debugger (IIRC).  So, let's be sure that our test script is not ignoring shutdown failures.  
This bug should not be closed until shutdown failures are detected and reported as test failures by the script.  That probably means that another patch must be written, for the QA test script.

Comment 8

[Alexei Volkov]

Attachment: p7sign tests

Nelson. We didn't have any tests for p7sign integrated yet.

I had this patch for a while sitting in workspace. Looks like a good time to integrate, since this bug will resolve problem with assertion at NSS_Shutdown in p7sign.

Comment 9

[Alexei Volkov]

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

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 241765 [details] [diff] [review]
use SECFailure instead of -1 error code 

I previously gave this patch r+, but now I realize that it was 
missing something.  It was missing the fix shown in comment 0.

>+    if (outFile && outFile != stdout) {
>+        fclose(outFile);
>+    }
>     if (NSS_Shutdown() != SECSuccess) {

Need to add here:
+         SECU_PrintError(progName, "NSS shutdown:");

>         exit(1);
>     }

Please add a new patch that fixes that.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

oh, I see my comment 10 contradicts my comment 7.
I guess I really want that error message about shutdown failure after all.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250514 [details] [diff] [review]
p7sign tests

The p7* programs test our old PKCS7 (SMIME) library.
IMO, These tests should be part of tests/smime/smime.sh, not tools.sh

Comment 13

[Alexei Volkov]

Attachment: moving tests to smime

Needed to make some changes in p7decode.c (memory leaks cleanup) and p7content.c (create a PK11 password function) in order for tests to run.

Comment 14

[Alexei Volkov]

Attachment: print error with SECU_PrintError in case of error during shutdown

Still, would like to see all our tools behave uniformly, that is either print and don't print error at shutdown. Non of the command prints such errors, and I agree with original design: user should not now about shutdown problem at all. This is why we assert only if when NSS_STRICT_SHUTDOWN is set. I don't this we should integrate this patch.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250889 [details] [diff] [review]
moving tests to smime

Review comments & questions:

>Index: lib/pkcs7/p7decode.c

>     if (cert == NULL) {
> 	p7dcx->error = SEC_ERROR_NOT_A_RECIPIENT;
> 	goto no_key_found;
>     }
> 
>-    ri->cert = cert;		/* so we can find it later */

Please explain the above change.  
I'm concerned that this breaks backwards binary compatiblity.
Doesn't some caller depend on this to get the recipient's cert?  
Doesn't this change make it difficult for the caller to get the 
cert associated with this ricipientInfo?  
Maybe the leak is that the caller doesn't destroy that reference?


>@@ -588,27 +587,29 @@

>       default:
> 	p7dcx->error = SEC_ERROR_UNSUPPORTED_KEYALG;
> 	goto no_key_found;

Please change that goto to a break;

>     }
> 
>-    return bulkkey;
>-
> no_key_found:
>     if (privkey != NULL)
> 	SECKEY_DestroyPrivateKey (privkey);
>+    if (cert != NULL)
>+        CERT_DestroyCertificate(cert);
>+    if (slot != NULL)
>+        PK11_FreeSlot(slot);
> 
>-    return NULL;
>+    return bulkkey;
> }

There's one problem with this change.  bulkkey is not initialized.
There are paths through the function that get to no_key_found: where
bulkkey is still uninitialized.  So, initialize bulkkey to NULL in its 
definition.  Then this change will be OK.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250890 [details] [diff] [review]
print error with SECU_PrintError in case of error during shutdown

(In reply to comment #14)

> Still, would like to see all our tools behave uniformly, that is either print
> and don't print error at shutdown. 

I agree with that.  
I'd say they should all uniformly output a message when shutdown fails.

> None of the command prints such errors, 

Several comments:

a) Remember that the NSS commands are our QA tools first, and general 
utilities for users second.  IMO, it is more important that we know when
our libraries are leaking references than that the output is beautiful.
If users see this message, they will complain to us, and thus we will 
learn that our code is leaking references.  

b) The following commands already print a message when shutdown fails:

  cmsutil, crmftest, pp, pwdecrypt, selfserv, strsclnt, vfychain

Comment 17

[Alexei Volkov]

(In reply to comment #15)
> (From update of attachment 250889 [details] [diff] [review])
> Review comments & questions:
> 
> >Index: lib/pkcs7/p7decode.c
> 
> >     if (cert == NULL) {
> > 	p7dcx->error = SEC_ERROR_NOT_A_RECIPIENT;
> > 	goto no_key_found;
> >     }
> > 
> >-    ri->cert = cert;		/* so we can find it later */
> 
> Please explain the above change.  
> I'm concerned that this breaks backwards binary compatiblity.
> Doesn't some caller depend on this to get the recipient's cert?  
> Doesn't this change make it difficult for the caller to get the 
> cert associated with this ricipientInfo?  
> Maybe the leak is that the caller doesn't destroy that reference?

Missed this part. Should not have made any changes. Cert is not a problem
of assertion at shutdown. Referenced  cert get destructed by caller by SEC_PKCS7DestroyContentInfo function call.

Comment 18

[Alexei Volkov]

Attachment: smime tests + review comments

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 250919 [details] [diff] [review]
smime tests + review comments

r=nelson.  Please fix the spelling errors below before committing.

>+  html_msg $? 0 "Creating envilope for user Alice" "."
                            envelope

>+  html_msg $? 0 "Verifiing file delivered to user Alice" "."
                   Verifying

Comment 20

[Alexei Volkov]

/cvsroot/mozilla/security/nss/cmd/p7content/p7content.c,v  <--  p7content.c
new revision: 1.11; previous revision: 1.10
/cvsroot/mozilla/security/nss/cmd/p7sign/p7sign.c,v  <--  p7sign.c
new revision: 1.12; previous revision: 1.11
/cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v  <--  p7decode.c
new revision: 1.24; previous revision: 1.23
/cvsroot/mozilla/security/nss/tests/smime/smime.sh,v  <--  smime.sh
new revision: 1.24; previous revision: 1.23

Comment 21

[Slavomir Katuscak]

Alexei, this patch causes failures on Windows:

p7env -d ../alicedir -r Alice -i alice.txt -o alice_p7.env
smime.sh: Creating envelope for user Alice . PASSED
p7content -d ../alicedir -i alice.env -o alice_p7.data
p7content: problem decoding data: security library: improperly formatted DER-encoded message.
[1] + Done(134) ?
  2960	Abort	p7content
smime.sh: Verifying file delivered to user Alice . FAILED
diff alice.txt alice_p7.data.sed
1,4d0
< Date: Wed, 20 Sep 2000 00:00:01 -0700 (PDT)
< From: [e-mail]
< Subject: message Alice --> Bob
< To: [e-mail]
6c2
< This is a test message from Alice to Bob.
---
> ---------------------------------------------
smime.sh: Compare Decoded Enveloped Data and Original . FAILED
p7sign -d ../alicedir -k Alice -i alice.txt -o alice.sig -p nss -e
smime.sh: Signing file for user Alice . PASSED
p7verify -d ../alicedir -c alice.txt -s alice.sig
p7verify: problem decoding/verifying signature: security library: improperly formatted DER-encoded message.
[1] + Done(134) ?
  2468	Abort	p7verify
smime.sh: Verifying file delivered to user Alice . FAILED

Comment 22

[Alexei Volkov]

Attachment: tbox problem fix

p7env and p7sign were opening output files for writing for a text, not binary data. Patch will fix the problem.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 252846 [details] [diff] [review]
tbox problem fix

Thanks, Alexei.  Evidently these programs have never before been tested on Windows.  :-(

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Committed lastest patch on trunk to fix tinderbox.

Checking in p7env/p7env.c;   new revision: 1.8;  previous revision: 1.7
Checking in p7sign/p7sign.c; new revision: 1.13; previous revision: 1.12

Will mark this fixed if/when tinderbox goes green for windows.

Comment 25

[Alexei Volkov]

tinderbox is green. marking the bug as fixed