Closed
Bug 562636
Opened 14 years ago
Closed 14 years ago
Memory leak when decoding PKCS12 because aSafeCinfo is not freed
Categories
(NSS :: Libraries, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.7
People
(Reporter: ch-bugsmozilla, Assigned: nelson)
Details
Attachments
(3 files, 1 obsolete file)
1.56 KB,
application/octet-stream
|
Details | |
7.08 KB,
patch
|
Details | Diff | Splinter Review | |
37.59 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 ( .NET CLR 3.5.30729) Build Identifier: trunk I use NSS as a library in a Windows application and noticed that the application used additional 20 KB of memory each time I imported a PKCS12. Using the umdh.exe tool from Microsoft, I got a couple of stack traces from where the leaking memory was allocated. Using one of the stack traces and some debugging, I noticed that the SEC_PKCS12DecoderContextStr contains a member SEC_PKCS7ContentInfo *aSafeCinfo which is not freed in SEC_PKCS12DecoderFinish. With the (to be) attached patch, there is 2 KB less memory leaking when importing a PKCS12 and using the patch. The only NSS tool calling SEC_PKCS12DecoderFinish is pk12util and this tool still works with the patch applied. Reproducible: Always Steps to Reproduce: I assume that the problem can be reproduced with the NSS command line tool pk12util -l and pk12util -i, but I don't know how to easily find out whether there is memory leaking or not. I can see the memory leaking in my own application with umdh, but this will be difficult to reproduce for others :-( I am going to search for the memory leaks causing the other 18 KB but I believe that these are unrelated to this bug.
Reporter | ||
Comment 1•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #442369 -
Flags: review?(nelson)
Assignee | ||
Comment 2•14 years ago
|
||
If you would attach to this bug a PKCS#12 file with which this leak can be reproduced using pk12util -i or pk12util -l, and tell us its password, I would appreciate it.
Reporter | ||
Comment 3•14 years ago
|
||
The password to the PKCS#12 is 'password', all in lower case
Reporter | ||
Comment 4•14 years ago
|
||
I have run some further tests to confirm that the leak can be reproduced with pk12util in Windows 7. I have slightly modified pk12util such that the P12U_ListPKCS12File is executed 1000 times instead of only once. I could then see a difference of memory consumption with the task manager: The unpatched pk12util leaks 15796 KB - 2868 KB = 12928 KB in 999 runs The patched pk12util leaks 13484 - 2868 KB = 10616 KB in 999 runs This means P12U_ListPKCS12File leaks about 1300 Byte without patch and only about 1062 Byte with the patch. (using the attached PKCS#12 file)
Reporter | ||
Comment 5•14 years ago
|
||
I have made a mistake in the calculation, this would only be correct if 1 KB = 100 Bytes ;-) So with the attached PKCS#12 file, P12U_ListPKCS12File leaks about 12941 Bytes without the patch and only about 10626 Bytes with the patch.
Assignee | ||
Comment 6•14 years ago
|
||
Slavo, Do our many leak check tests include tests of pk12util?
OS: Windows 7 → All
Hardware: x86 → All
Assignee | ||
Updated•14 years ago
|
Priority: -- → P2
Version: unspecified → 3.12.4
Assignee | ||
Updated•14 years ago
|
Severity: trivial → minor
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: P2 → P3
Target Milestone: --- → 3.12.7
Assignee | ||
Comment 7•14 years ago
|
||
Comment on attachment 442369 [details] [diff] [review] Patch for revision 1.46 which deallocates some of the leaking memory r=nelson This matches what is done when encoding. We should look at the rest of the leaks, but this is lower priority than some other issues at the moment.
Attachment #442369 -
Flags: review?(nelson) → review+
Reporter | ||
Comment 8•14 years ago
|
||
These leaks are bothering me since my application decodes a lot of PKCS#12 files. Thus, I will search for the other leaks anyway, although I'm not sure when I find the time for it. If somebody else is also interested to do that, I can share those stack traces I've created with umdh. In this case, I think it's a good idea to coordinate who looks for which leak to avoid redundant work. Just mail me :-)
Assignee | ||
Comment 9•14 years ago
|
||
If you use a real leak finding tool, you should be able to find them all at once. Since you're using Windows, I'll guess you're using MSVC and point you to http://msdn.microsoft.com/en-us/library/e5ewb1h3(VS.80).aspx http://msdn.microsoft.com/en-us/library/x98tx3cf(VS.80).aspx This problem is not unique to Windows. Other OSes have other techniques which may be less cumbersome.
Assignee | ||
Comment 10•14 years ago
|
||
Some of the leaks in pk12util are simply because pk12util fails to call all the appropriate library clean up functions before it exits. Here's a patch to pk12util.c that helps considerably: @@ -1124,5 +1133,7 @@ done: if (NSS_Shutdown() != SECSuccess) { pk12uErrno = 1; } + PR_Cleanup(); + PL_ArenaFinish(); return pk12uErrno; }
Assignee | ||
Comment 11•14 years ago
|
||
This patch shows the changes I made to find the leaks using MSVC's debug run time leak finding apparatus. It also includes the fixes that eliminate the leaks that I found. (I left an NSPR leak that is known). It would be nice, but is not necessary to commit this entire patch. The relevant parts to the bug at hand are in p12d.c and pk12util.c. Please test this patch, at least the portion affecting those two files.
Assignee: nobody → nelson
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•14 years ago
|
||
Comment on attachment 443050 [details] [diff] [review] patch v2 - fixes all NSS leaks demonstrated with sample file I'm asking myself to review this patch, just so I won't forget it. I'm pretty sure this patch is bogus and will cause some double frees, even though I've not yet been able to reproduce them, but if I mark it r-, it will fall off my radar. :-/
Attachment #443050 -
Attachment description: patch v2 - fixes all leaks demonstrated with sample file → patch v2 - fixes all NSS leaks demonstrated with sample file
Attachment #443050 -
Flags: review?(nelson)
Comment 13•14 years ago
|
||
(In reply to comment #6) > Slavo, Do our many leak check tests include tests of pk12util? No, pk12util is not covered by memleak tests.
Assignee | ||
Comment 14•14 years ago
|
||
Years ago, when I worked on bug 210179, I cleaned up the pkcs12 encoder, renamed many variables to make their names meaningful, and created a diagram to explain the quite complicated nested layers of encoders used. See bug 210179 comment 4, the diagram at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pkcs12/p12e.c&rev=1.20&mark=53#53 and the renaming beginning at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pkcs12/p12e.c&rev=1.20&mark=170,182-184,190-191#162 Now, it's clear to me that, before I'm done with THIS bug for the decoder, I must at least similarly document the structure of the nested decoders, and it would probably be useful to give more meaningful names to some of the variables in the various contexts, too.
Assignee | ||
Comment 15•14 years ago
|
||
This patch is much larger than the previous one. It has the beginning of the variable renaming, and it also cleans up the use of the arena allocation functions. While producing it, I found and eliminated one possible cause of double-free, and found and eliminated one dead context pointer that was being deallocated but never used. This may be good enough. I want to look it over a little more before I ask for review.
Assignee | ||
Comment 16•14 years ago
|
||
Comment on attachment 445580 [details] [diff] [review] Patch v3 for trunk - ready for review (checked in) Bob, please review the NSS part of this patch. Much of this is merely variable renaming. You can compare it with the previous patch to see what else changed.
Attachment #445580 -
Flags: review?(rrelyea)
Assignee | ||
Updated•14 years ago
|
Attachment #445580 -
Attachment description: Patch v3 for trunk - Maybe ready for review → Patch v3 for trunk - ready for review
Assignee | ||
Comment 17•14 years ago
|
||
Comment on attachment 443050 [details] [diff] [review] patch v2 - fixes all NSS leaks demonstrated with sample file Please review patch v3 isntead.
Attachment #443050 -
Flags: review?(nelson)
Assignee | ||
Updated•14 years ago
|
Attachment #442369 -
Attachment is obsolete: true
Comment 18•14 years ago
|
||
Comment on attachment 445580 [details] [diff] [review] Patch v3 for trunk - ready for review (checked in) r+ rrelyea. With some comments, questions, and notes. Questions: In sec_pkcs12_decoder_asafes_notify there are two things going on. I understand the capturing cinfo so we can free it from SEC_PKCS7DecoderFinish(), but what is going on with the last safeContentList entry. We seem to be freeing just that entry (but no nexted contexts associated with it) here, Won't this get freed when we finally finish the p12dcx? There appears to be no leak.. the nested context pointers are not cleared here, so they will get freed when we Finish, I'm just a little confused why we are freeing this one here. Comments: 1) it would be nice to have something in the file that tells us A1D means ASN.1 Decode context. It wasn't clear to me until I went back to the bug. 2) You comment in the bug about including a picture off how all these nested structures work like you did for p12e, but I didn't see any in this patch. 3) You can move PORT_ArenaGrowArray to secport.h without further review. Note on name, All other macro versions have 'New' in the name, but 'New' doesn't seem to fit with Grow (GrowNewArray?). Notes: Here's what I reviewed: The vast amounts of clean up, I only verified that the new lines had the same semantics as the old lines. I did not verify that the semantic was correct. I have no problem with any of these changes from a style perspective. RTL_DBG plumbing looked fine. The NSPR changes look fine to me, but it's prrobably best for wtc to look at them. leak fix parts of the patch were reviewed in more detail (sec_pkcs12_decoder_asafes_notify, SEC_PCS12DecoderFinish, and pk12util.c. bob
Attachment #445580 -
Flags: review?(rrelyea) → review+
Assignee | ||
Comment 19•14 years ago
|
||
(In reply to comment #18) Bob, thanks for your excellent review questions. You found a bug in my patch so I'm going to fix it and submit another. I _may_ commit this patch and submit a much smaller patch to fix the bug. > Questions: In sec_pkcs12_decoder_asafes_notify [...] what is going on with > the last safeContentList entry. We seem to be freeing just that entry (but > no nested contexts associated with it) here, Won't this get freed when we > finally finish the p12dcx? The failure to free the nested contexts there is a bug, I believe. I didn't catch it in my own testing because nested contexts are rare, and dependent on the encoder. I think the test file I am using doesn't generate any nested contexts when decoded, so my testing was inadequate. Thanks for mentioning it. I put this code in sec_pkcs12_decoder_asafes_notify early in the debugging of this bug. I had it "clean up" each safeContent as it went along, so that we didn't have a large number of these building up, and then all getting freed at the end. Then later I realized that that method will not free them all, so I added a loop in SEC_PKCS12DecoderFinish to go back over the array and clean up any that were missed along the way. Now, I _could_ eliminate the attempt to clean up as I go along, and just do all the cleanup at the end. Do you think that would be preferable? The main difference would be the peak amount of allocated memory before it all gets cleaned up, I believe. > There appears to be no leak.. the nested context pointers are not cleared > here, so they will get freed when we Finish, Will they? Doesn't the structure that contains them get destroyed? I'll double check. > I'm just a little confused why we are freeing this one here. It's an oversight. > Comments: > 1) it would be nice to have something in the file that tells us A1D means > ASN.1 Decode context. It wasn't clear to me until I went back to the bug. Will do. > 2) You comment in the bug about including a picture off how all these nested > structures work like you did for p12e, but I didn't see any in this patch. I ran out of motivation. I decided that this bug is fixed, the leaks are plugged, and investing a lot in this body of code would probably have no return on that investment. > 3) You can move PORT_ArenaGrowArray to secport.h without further review. Note > on name, All other macro versions have 'New' in the name, but 'New' doesn't > seem to fit with Grow (GrowNewArray?). Right. It's not a "New" as such, which is why I didn't put the word New in the name.
Assignee | ||
Comment 20•14 years ago
|
||
Comment on attachment 445580 [details] [diff] [review] Patch v3 for trunk - ready for review (checked in) coreconf/WIN32.mk; new revision: 1.38; previous revision: 1.37 nss/cmd/pk12util/pk12util.c; new revision: 1.44; previous revision: 1.43 nss/lib/pkcs12/p12d.c; new revision: 1.47; previous revision: 1.46 I decided to commit these patches as-is. Even if they do not fix 100% of the leaks, they leave fewer leaks than before. I will also move the macro to the util header file later.
Assignee | ||
Updated•14 years ago
|
Attachment #445580 -
Attachment description: Patch v3 for trunk - ready for review → Patch v3 for trunk - ready for review (checked in)
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•