NSS_CMSDecoder_Cancel might have a leak

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: kaie, Assigned: Julien Pierre)

Tracking

unspecified
3.12

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Updated

15 years ago
Blocks: 157937

Comment 1

15 years ago
Assigned the bug to Julien.
Assignee: wtc → jpierre
Priority: -- → P2
Target Milestone: --- → 3.7

Comment 2

15 years ago
Moved to target milestone 3.8 because the original
NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
(Assignee)

Comment 3

15 years ago
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 ?
(Reporter)

Comment 4

14 years ago
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.
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
(Assignee)

Comment 6

13 years ago
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 .
Target Milestone: --- → 3.10
(Assignee)

Updated

12 years ago
Target Milestone: 3.10 → ---
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

12 years ago
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

12 years ago
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

12 years ago
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

12 years ago
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

12 years ago
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

12 years ago
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.
QA Contact: bishakhabanerjee → jason.m.reid
QA Contact: jason.m.reid → libraries
(Assignee)

Updated

11 years ago
Attachment #183061 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #183063 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #183056 - Flags: review+
(Assignee)

Comment 14

11 years ago
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
You need to log in before you can comment on or make changes to this bug.