607058 - crash [@ nss_cms_decoder_work_data]

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wayne Mery (:wsmwk)]

crash [@ nss_cms_decoder_work_data]
#17 crash for Thunderbird v3.1.5
about 3/4 appear to be startup (or near end of startup) crashes

bp-289d7f56-6412-4fec-8d7c-af0522101021
0	smime3.dll	nss_cms_decoder_work_data	security/nss/lib/smime/cmsdecode.c:467
1	smime3.dll	nss_cms_decoder_update_filter	security/nss/lib/smime/cmsdecode.c:608
2	nssutil3.dll	SEC_ASN1DecoderUpdate_Util	security/nss/lib/util/secasn1d.c:2783
3	smime3.dll	NSS_CMSDecoder_Update	security/nss/lib/smime/cmsdecode.c:670
4	thunderbird.exe	nsCMSDecoder::Update	security/manager/ssl/src/nsCMS.cpp:813
5	thunderbird.exe	MimeCMS_write	mailnews/mime/src/mimecms.cpp:527
6	thunderbird.exe	MimeEncrypted_parse_decoded_buffer	mailnews/mime/src/mimecryp.cpp:191
7	thunderbird.exe	mime_decode_base64_buffer	mailnews/mime/src/mimeenc.cpp:326
8	thunderbird.exe	MimeDecoderWrite	mailnews/mime/src/mimeenc.cpp:838
9	thunderbird.exe	MimeEncrypted_parse_buffer	mailnews/mime/src/mimecryp.cpp:170
10	thunderbird.exe	MimeMessage_parse_line	mailnews/mime/src/mimemsg.cpp:232
11	thunderbird.exe	convert_and_send_buffer	mailnews/mime/src/mimebuf.cpp:184
12	thunderbird.exe	mime_LineBuffer	mailnews/mime/src/mimebuf.cpp:272
13	thunderbird.exe	MimeObject_parse_buffer	mailnews/mime/src/mimeobj.cpp:275
14	thunderbird.exe	mime_display_stream_write	mailnews/mime/src/mimemoz2.cpp:944
15	thunderbird.exe	nsStreamConverter::OnDataAvailable	mailnews/mime/src/nsStreamConverter.cpp:979
16	thunderbird.exe	nsMailboxProtocol::ReadMessageResponse	mailnews/local/src/nsMailboxProtocol.cpp:586
17	thunderbird.exe	nsMailboxProtocol::ProcessProtocolState	mailnews/local/src/nsMailboxProtocol.cpp:683
18	thunderbird.exe	nsMsgProtocol::OnDataAvailable	mailnews/base/util/nsMsgProtocol.cpp:359
19	thunderbird.exe	nsInputStreamPump::OnStateTransfer	netwerk/base/src/nsInputStreamPump.cpp:510
20	thunderbird.exe	nsInputStreamPump::OnInputStreamReady	netwerk/base/src/nsInputStreamPump.cpp:400

Comment 1

[timeless]

Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x3c

465 cinfo = NSS_CMSContent_GetContentInfo(p7dcx->content.pointer, p7dcx->type);
466
467 if (cinfo->ciphcx != NULL) { 

cinfo is null

Comment 2

[timeless]

Attachment: wallpaper

280 NSS_CMSContent_GetContentInfo(void *msg, SECOidTag type)
281 {
283     NSSCMSContentInfo *cinfo = NULL;
285     if (!msg)
286         return cinfo;
288     switch (type) {
289     case SEC_OID_PKCS7_SIGNED_DATA:
291         break;
292     case SEC_OID_PKCS7_ENVELOPED_DATA:
294         break;
295     case SEC_OID_PKCS7_ENCRYPTED_DATA:
297         break;
298     case SEC_OID_PKCS7_DIGESTED_DATA:
300         break;
301     default:
302         cinfo = NULL;
304     return cinfo;

roughly speaking, null cinfo can happen. The other callers in this file all check p7dcx->type before calling NSS_CMSContent_GetContentInfo.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to comment #0)
> crash [@ nss_cms_decoder_work_data]
> #17 crash for Thunderbird v3.1.5
> about 3/4 appear to be startup (or near end of startup) crashes

Just a guess, maybe this happens if the last shown message (in the previous session) was an encrypted message, that message is remembered to be shown on startup. On startup, password and key might not be available (user has not yet entered master password, our multiple prompts race etc.), and decoder fails because of a lack of encryption key.

?

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 486305 [details] [diff] [review]
wallpaper

I spent a while reading the related code and agree with the code in general.
The existing code really expected cinfo to always be available, and in some places we're in functions that can't return a failure, even.

Without knowing all of the code flow, I propose we should at least set error flags when returning.
Other layers should notice that we have a problem and should abort processing, giving the user a change to notice.

I propose to use a general error message that indicates an internal failure. Couldn't find anything better than SEC_ERROR_LIBRARY_FAILURE.


Please consistently use a 4-space-per-indent-level in this file



(a)
> 
>     cinfo = NSS_CMSContent_GetContentInfo(p7dcx->content.pointer, p7dcx->type);
>+    if (!cinfo)

                  { 
          /* The original programmer didn't expect this to happen */
          p7dcx->error = SEC_ERROR_LIBRARY_FAILURE;

>+	goto loser;

      }
    


(b)
>     /* we got data (either from the caller, or from a lower level encoder) */
>     cinfo = NSS_CMSContent_GetContentInfo(p7ecx->content.pointer, p7ecx->type);
>+    if (!cinfo)

                  {
          /* The original programmer didn't expect this to happen */
          p7ecx->error = SEC_ERROR_LIBRARY_FAILURE;

>+	return SECFailure;

      }



(c)
> 	/* we are at innermost decoder */
> 	/* find out about our inner content type - must be data */
> 	cinfo = NSS_CMSContent_GetContentInfo(p7ecx->content.pointer, p7ecx->type);
>+	if (!cinfo)

                    {
            /* The original programmer didn't expect this to happen */
            p7ecx->error = SEC_ERROR_LIBRARY_FAILURE;

>+	    return SECFailure;

        }


(d)
>     /* find out about our inner content type - must be data */
>     cinfo = NSS_CMSContent_GetContentInfo(p7ecx->content.pointer, p7ecx->type);
>+    if (!cinfo) {

          /* The original programmer didn't expect this to happen */
          p7ecx->error = SEC_ERROR_LIBRARY_FAILURE;

>+	rv = SECFailure;
>+	goto loser;
>+    }



r=kaie with these changes

Comment 6

[Ludovic Hirlimann [:Usul]]

(In reply to comment #5)
> *** Bug 616931 has been marked as a duplicate of this bug. ***

sorry the bug wasn't showing up in soccoro.

Comment 7

[timeless]

please find out *why* it isn't showing up...

Comment 8

[timeless]

Attachment: wallpaper per kaie

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 495552 [details] [diff] [review]
wallpaper per kaie

Timelsess, please stop putting false r= comments into your patches when 
you submit them.  

I'd be happy to review this patch if it was a CVS diff against the NSS 
source tree.  I think no NSS developer should review any patch that is 
NOT a CVS diff, since BMO cannot show any more context for such a patch.

The question to ask here is: given that the NSS_CMS code hasn't changed 
in years, why does this crash now suddenly begin to appear and become a 
top crasher?   

The answer is probably: because the caller of NSS has changed, and is 
ignoring error results of previous calls, and going ahead with an object
that it should have known is in a bad state.  I'm not saying that the NSS
code shouldn't be fixed, but when code suddenly breaks after years, you 
have to look at what changed.

Comment 10

[Kai Engert (:KaiE:)]

Attachment: timeless patch v2 converted to a cvs diff -u20 -p

Nelson, here is the cvs version patch that you have requested.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 499047 [details] [diff] [review]
timeless patch v2 converted to a cvs diff -u20 -p

I agree with Timeless's comment that this is "wallpaper", because it papers 
over the real underlying problem, rather than fixing it.  I wish we could 
enlist the help of the people who see this crash in figuring out how to 
reproduce it. Then we'd have a chance to really fix it.

I agree that error codes need to be set, and this patch uses the right method 
to set them, but I'm not very happy with this particular choice of error code.
Nonetheless, I'm willing to let this patch go in.  We can tweak the error code
later, if needed.

Comment 12

[Kai Engert (:KaiE:)]

fixed on trunk

Checking in cmsdecode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsdecode.c,v  <--  cmsdecode.c
new revision: 1.10; previous revision: 1.9
done
Checking in cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.7; previous revision: 1.6
done


This should land on the stable NSS 3.12.x branch,
not yet sure whether it will be before or after 3.12.9

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I was thinking we should not close this bug until we have found the root cause 
of the problem that leads to a NULL cinfo pointer, and fixed it.  Please file 
an additional bug to record this problem and track our progress on finding 
that root cause.

Comment 14

[Kai Engert (:KaiE:)]

I'm OK to keep this open.

Comment 15

[timeless]

one commit per bug, if you want to track something down, file a new bug.

Comment 16

[Kai Engert (:KaiE:)]

3.12 branch checkin

Checking in cmsdecode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsdecode.c,v  <--  cmsdecode.c
new revision: 1.9.66.1; previous revision: 1.9
done
Checking in cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.6.66.1; previous revision: 1.6
done