Bug 501080 - [PEM] way to free memory allocated by internal objects
Summary: [PEM] way to free memory allocated by internal objects
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: nss
Version: 10
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Elio Maldonado Batiz
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 501138
TreeView+ depends on / blocked
 
Reported: 2009-05-15 21:52 UTC by Kamil Dudka
Modified: 2009-12-04 16:54 UTC (History)
3 users (show)

Fixed In Version: 3.12.3.99.3-2.10.4.fc10
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2009-07-19 10:25:28 UTC


Attachments (Terms of Use)
test case (1.58 KB, application/octet-stream)
2009-05-15 21:52 UTC, Kamil Dudka
no flags Details
proposed fix (3.34 KB, patch)
2009-05-15 21:53 UTC, Kamil Dudka
no flags Details | Diff
follow-up patch (5.59 KB, patch)
2009-05-16 20:05 UTC, Kamil Dudka
no flags Details | Diff
proposed fix for possibly undestroyed arena in prsa.c (969 bytes, patch)
2009-05-23 17:54 UTC, Kamil Dudka
no flags Details | Diff

Description Kamil Dudka 2009-05-15 21:52:45 UTC
Created attachment 344238 [details]
test case

Description of problem:
While investigating huge memory leaks in curl I discovered the function pem_DestroyInternalObject() is pretty broken, moreover it wasn't called at all. 
Attached patch frees internal objects on module destruction.

It gives zero memory leaks on attached (brute force) test case, but it is still leaking within pfind.c in more complex cases. As time permits other patches will follow.

Version-Release number of selected component (if applicable):
nss-3.12.2-5.fc10

Steps to Reproduce:
1. run the attached test case
  
Actual results:
huge memory leaks

Additional info:
patch ready and awaiting review

Comment 1 Kamil Dudka 2009-05-15 21:53:48 UTC
Created attachment 344239 [details]
proposed fix

Comment 2 Kamil Dudka 2009-05-16 20:05:30 UTC
Created attachment 344296 [details]
follow-up patch

An incremental patch is attached. It catches tons of memory leaks while using a client certificate.

Comment 3 Elio Maldonado Batiz 2009-05-16 21:34:53 UTC
(In reply to comment #2) Kamil, Reassigning to you, please check the ? review flag and reassign back to me.

Comment 4 Elio Maldonado Batiz 2009-05-16 22:22:22 UTC
Oops, I messed it up again. I should have asked you to keep it assigned to you and set the ? flag with me as the reviewer. Now it came out assigned to me with you as the reviewer :-) Sorry about that.

Comment 5 Elio Maldonado Batiz 2009-05-22 16:45:21 UTC
Both the proposed fix and follow up patch get +r from me.

Comment 6 Rob Crittenden 2009-05-22 19:29:07 UTC
First patch +r

In the second patch, you are adding a nss_ZFreeIf() before each allocation of the key data. Is this really needed? I don't think there is a way to re-load a key on top of an existing one so these should always be unallocated.

Comment 7 Kamil Dudka 2009-05-22 19:44:19 UTC
It is, because the memory is really freed by the nss_ZFreeIf() calls. I don't know the PEM reader internals enough to explain why. It might be another bug. But if you omit them, you will loose the previously allocated memory on the second allocation. I have nothing like minimal example for this one, but I used this (you need to replace userID with the your one):

LD_PRELOAD=libnsspem.so valgrind --leak-check=full curl 'https://koji.fedoraproject.org/koji/userinfo?userID=750' --verbose --silent --cert $HOME/.fedora.cert -o /dev/null

Comment 8 Kamil Dudka 2009-05-23 07:04:03 UTC
(In reply to comment #7)
> this (you need to replace userID with the your one):

Actually you don't need to change anything. Feel free to download my build list :-)

Comment 9 Elio Maldonado Batiz 2009-05-23 17:27:42 UTC
Spotted another memory leak in prsa.c. pem_PopulateModulusExponent allocates an arena, then if the test commented by /* make sure we have the right objects */ fails it returns without deallocating the arena.

Comment 10 Kamil Dudka 2009-05-23 17:54:22 UTC
Created attachment 345202 [details]
proposed fix for possibly undestroyed arena in prsa.c

Good catch. It's possible memory leak for nothing. Moving the allocation of arena down should fix it.

Comment 11 Fedora Update System 2009-06-23 04:05:57 UTC
nss-3.12.3.99.3-2.11.3.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/nss-3.12.3.99.3-2.11.3.fc11

Comment 12 Fedora Update System 2009-06-27 02:38:54 UTC
nss-3.12.3.99.3-2.11.3.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-6948

Comment 13 Fedora Update System 2009-06-27 02:56:42 UTC
nss-3.12.3.99.3-2.10.4.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update nss'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-7017

Comment 14 Fedora Update System 2009-07-19 10:24:56 UTC
nss-3.12.3.99.3-2.11.3.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 15 Fedora Update System 2009-07-19 10:26:04 UTC
nss-3.12.3.99.3-2.10.4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.


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