Last Comment Bug 177184 - NSS_CMSDecoder_Cancel might have a leak
: NSS_CMSDecoder_Cancel might have a leak
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: 3.12
Assigned To: Julien Pierre
:
Mentors:
Depends on:
Blocks: psmleaks
  Show dependency treegraph
 
Reported: 2002-10-28 14:44 PST by Kai Engert (:kaie)
Modified: 2006-10-25 19:52 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Enhance cmsutil to demostrate proper error handling and use of NSS_CMSDecoderCancel (1.61 KB, patch)
2005-05-09 10:56 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Splinter Review
NSS_CMSDecoder_Cancel fix 1 (751 bytes, patch)
2005-05-09 11:03 PDT, Wan-Teh Chang
no flags Details | Diff | Splinter Review
NSS_CMSDecoder_Cancel fix 2 (689 bytes, patch)
2005-05-09 11:38 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Splinter Review
Patch for NSS_CMSMessage_CreateFromDER (687 bytes, patch)
2005-05-09 11:51 PDT, Wan-Teh Chang
julien.pierre: review+
Details | Diff | Splinter Review

Description Kai Engert (:kaie) 2002-10-28 14:44:34 PST
A comment in NSS_CMSDecoder_Cancel says there is a leak.

Striving for leak free, I'm want to call the cancel function, in case the S/Mime
code did not finish the processing. I thought I should call cancel instead of
finish in the cleanup phase.

We should check the code whether there is really a leak, and whether the same
leak happens when calling NSS_CMSDecoder_Finish.
Comment 1 Wan-Teh Chang 2002-10-29 17:22:21 PST
Assigned the bug to Julien.
Comment 2 Wan-Teh Chang 2002-12-06 11:16:08 PST
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Comment 3 Julien Pierre 2003-01-21 18:49:50 PST
For the record, the function looks like this :

/*
 * NSS_CMSDecoder_Cancel - stop decoding in case of error
 */
void
NSS_CMSDecoder_Cancel(NSSCMSDecoderContext *p7dcx)
{
    /* XXXX what about inner decoders? running digests? decryption? */
    /* XXXX there's a leak here! */
    NSS_CMSMessage_Destroy(p7dcx->cmsg);
    (void)SEC_ASN1DecoderFinish(p7dcx->dcx);
    PORT_Free(p7dcx);
}

Chris, do you have any recollection of this issue ?
Comment 4 Kai Engert (:kaie) 2003-01-28 19:49:10 PST
cc'ing Stephane, I think it would be worth spending some time on this issue, to
assure there isn't a leak in this area of the code - since a comment in the code
says there is most likely one.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2003-05-09 21:18:47 PDT
Remove target milestone of 3.8, since these bugs didn't get into that release.
Comment 6 Julien Pierre 2004-12-08 18:32:11 PST
I looked at the code and I'm not confident there is a leak .
The main risk I see is with the ASN.1 decoder . The calls always use arenas,
which are getting freed properly in NSS_CMSMessage_Destroy .

Also, the NSS_CMSDecoder_Finish function does even less, since it does not
destroy the message, and presumably that function is getting called in success
cases. But we don't see leaks with it.

That's in theory, at least.

To be certain, we may need to actually try it and check with purify if calling
this API causes a leak with a tool like purify of solaris 10's libumem . Perhaps
we could modify cmsutil to call this API during the NISCC test suite and observe
if this introduces leaks .
Comment 7 Nelson Bolyard (seldom reads bugmail) 2005-05-07 20:59:01 PDT
In the fourth quarter of 2003, Julien and I did *extensive* testing of libSMIME
for leaks.  We used the NISCC test suite, which has over 1.3 MILLION test 
messages, most of which have errors, so it extensively tests the error paths.
There were quite a few leaks when we started this work.  There were NONE when
we were done.  So I think it is quite unlikely that there remains a leak 
in the code paths that involve the code cited above.  

I personally think that bugzilla bugs should report observed and confirmed bugs, 
not conjectured or suspected bugs.  The comments in the code quoted above show
the author's uncertainty that the error paths were correct, but are not proof
nor observation of a leak.  Perhaps we should return this bug to UNCONFIRMED
state.  I personally think the bug in invalid unless/until someone finds a 
real leak.

Perhaps we should change the code comments quoted above to say something like
"The author was not sure that all error paths were adequately handled.
 This code path should be subjected to additional leak testing."
and then resolve this bug invalid.
Comment 8 Wan-Teh Chang 2005-05-09 10:40:40 PDT
Mozilla's LXR shows that NSS_CMSDecoder_Cancel is not
reachable by any NSS test program.  So the NISCC S/MIME
tests we ran did not cover this function.

(In the entire Mozilla cvs repository, NSS_CMSDecoder_Cancel
is only called by PSM's nsCMSDecoder::destructorSafeDestroyNSSReference:
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsCMS.cpp#797)

I will propose a couple of things we can do about this
bug.



Comment 9 Wan-Teh Chang 2005-05-09 10:56:40 PDT
Created attachment 183056 [details] [diff] [review]
Enhance cmsutil to demostrate proper error handling and use of NSS_CMSDecoderCancel

This patch makes cmsutil handle the failures of the
NSS_CMSDecoder_XXX functions and call NSS_CMSDecoder_Cancel
instead of NSS_CMSDecoder_Finish on NSS_CMSDecoder_Update
failure.

This patch requires fixing NSS_CMSDecoder_Cancel first.
When NSS_CMSDecoder_Update fails, it calls
SEC_ASN1DecoderFinish(p7dcx->dcx) and sets p7dcx->dcx to
NULL, so NSS_CMSDecoder_Cancel needs to test p7dcx->dcx
before calling SEC_ASN1DecoderFinish(p7dcx->dcx).
Comment 10 Wan-Teh Chang 2005-05-09 11:03:24 PDT
Created attachment 183057 [details] [diff] [review]
NSS_CMSDecoder_Cancel fix 1

The first way to fix NSS_CMSDecoder_Cancel is to simply
define it in terms of the closely related NSS_CMSDecoder_Finish
function.  Basically we call NSS_CMSDecoder_Finish and throw
away the result.
Comment 11 Wan-Teh Chang 2005-05-09 11:38:39 PDT
Created attachment 183061 [details] [diff] [review]
NSS_CMSDecoder_Cancel fix 2

If we want to let NSS_CMSDecoder_Cancel have its own
code, it should be as close to the error path in
NSS_CMSDecoder_Cancel as possible.  It definitely
needs to test p7dcx->dcx for NULL.  I don't think
the ordering of SEC_ASN1DecoderFinish and
SEC_ASN1DecoderFinish matters, but I reversed the
order just to be safe because p7dcx->dcx contains
a reference to p7dcx->cmsg (the destination of
decoder output).
Comment 12 Wan-Teh Chang 2005-05-09 11:51:01 PDT
Created attachment 183063 [details] [diff] [review]
Patch for NSS_CMSMessage_CreateFromDER

This function needs to at least handle
NSS_CMSDecoder_Start failure.  It may also
want to check the return value of NSS_CMSDecoder_Update
and call NSS_CMSDecoder_Cancel on failure.
Comment 13 Wan-Teh Chang 2005-05-09 11:54:04 PDT
I recommend we do the following for this bug.

1. Take either fix 1 or fix 2 for NSS_CMSDecoder_Cancel.

2. Fix cmsutil.c and NSS_CMSMessage_CreateFromDER the
same way.  They both need to handle NSS_CMSDecoder_Start
failure.  Optionally, they should also check the return
status of NSS_CMSDecoder_Update and call NSS_CMSDecoder_Cancel
on failure.
Comment 14 Julien Pierre 2006-08-04 18:20:53 PDT
I checked these patches in to the tip .

Checking in cmd/smimetools/cmsutil.c;
/cvsroot/mozilla/security/nss/cmd/smimetools/cmsutil.c,v  <--  cmsutil.c
new revision: 1.53; previous revision: 1.52
done
Checking in lib/smime/cmsdecode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsdecode.c,v  <--  cmsdecode.c
new revision: 1.9; previous revision: 1.8
done

Note You need to log in before you can comment on or make changes to this bug.