557922 - Memory leaks in NSPR

Status: NEW

(NSPR :: NSPR, defect)

Description

[Ben Boeckel]

User-Agent:       Uzbl (Webkit 1.1.15) (GNU/Linux i686 [i686]) (Commit 38ef86e4d1827338495ff52fb25a6808cf4e45d2)
Build Identifier: nss-3.12.6-1.2.fc13.i686

While going through fixing memory leaks in CHASM[1], I fixed some leaks                         
within NSPR. Here's a list of the leaks that CHASM exposed                       
(doing minimal things with NSS, basically just hashing):

  * The error tables are not cleaned up. This is the most invasive                
    change since it adds a new public function                                                                                                   
    (nspr_CleanupPRErrorTables) which cleans up *all* error tables. This                                                                         
    means that anything that calls this cleans up every error table that                                                                         
    has been installed. Unfortunately, I see no better way of handling                
    this since everything is already not threadsafe. This is then called                                    
    in PR_Cleanup (The positioning may be too early, but I put it in                           
    reverse order that PR_InitStuff does). A note was also added to the               
    PR_Cleanup comment.                                                                                                                          
  * Clean up the TPD pointer within _PR_CleanupTPD.

[1]http://chasmd.org

Reproducible: Always

Comment 1

[Ben Boeckel]

Attachment: Patch to fix memory leaks

Comment 2

[Ben Boeckel]

Ping?

Comment 3

[Marco Castelluccio [:marco]]

This patch needs a review.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 437676 [details] [diff] [review]
Patch to fix memory leaks

I'm sorry, but I don't know this code well enough to feel comfortable reviewing it.

Comment 5

[Wan-Teh Chang]

Comment on attachment 437676 [details] [diff] [review]
Patch to fix memory leaks

Thank you for the patch.  I took a quick look at the patch.
It seems correct.  I suggest some changes.

1. It doesn't seem necessary to have two functions
nspr_CleanupPRErrorTables and PR_ErrorCleanupTables
that do the same thing.

I think we can remove nspr_CleanupPRErrorTables and
just use PR_ErrorCleanupTables.

2. PR_ErrorCleanupTables should be an internal function,
called by PR_Cleanup only.

In nsprpub/pr/src/linking/prlink.c:

>+    PR_FREEIF(pr_exe_loadmap->name);
>+    PR_FREEIF(pr_exe_loadmap);

pr_exe_loadmap->name should be freed with free() because
it is allocated with strdup().

In nsprpub/pr/src/misc/prerrortable.c:

>+    while (et) {
>+        struct PRErrorTableList *cet;
>+
>+        cet = et->next;

Nit: rename the variable 'cet' to 'next' or 'next_et'.

In nsprpub/pr/src/misc/prinit.c:

>+ *   PR_Cleanup() calls nspr_CleanupPRErrorTables() which invalidates
>+ *   all error tables that have been installed.
>+ *

It is not necessary to add this comment.  We don't enumerate all the
cleanups performed by PR_Cleanup().

In nsprpub/pr/src/threads/prtpd.c:

> void _PR_CleanupTPD(void)
> {
>+    PR_FREEIF(_pr_tpd_destructors);
> }  /* _PR_CleanupTPD */

I don't know why _PR_CleanupTPD is a no-op.  It should
set _pr_tpd_destructors to NULL after freeing, and
also set _pr_tpd_length and _pr_tpd_highwater to 0.

Comment 6

[Ben Boeckel]

Attachment: Updated patch

Updated per comments. My CVS checkout has bitrotted to some extent (cvs diff was taking forever at the toplevel), so the patch is based in the nsprpub directory.

Comment 7

[hduston]

Attachment: nspr-errortable-memleak.patch

Proposed patch for the PR_ErrorInstallTable() memory leak.

Add new nspr_CleanupPRErrorTable() function to destroy only the nspr error table.
Add new public function PR_ErrorRemoveTable() to remove one entry from the error table and call it from nspr_CleanupPRErrorTable().

Comment 8

[hduston]

Attachment: nspr-tpd-memleak.patch

Proposed patch for the TPD memory leak.

Free the TBD data in _PR_CleanupTPD(), and add a call to _PR_CleanupTPD to _PR_Cleanup() nspr/pr/src/pthreads/ptthread.c.

Comment 9

[hduston]

Attachment: nspr-loadmap-memleak.patch

Proposed patch for the pr_exe_loadmap memory leak.

Add a call of PR_UnloadLibary(pr_exe_load_map) to _PR_ShutdownLinker(), and set pr_exe_load_map to NULL thereby bypassing the dereference PR_LoadStaticLibrary().

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 8692299 [details] [diff] [review]
nspr-loadmap-memleak.patch

Let's discuss this change in bug 96122, where you also attached the patch. It has more comments. Marking as obsolete in this bug.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: ben-merged-v3.patch

Looking at the old patches, it seems that Ben had correctly addressed Wan-Teh's review comments in 2012, but the newer attached patch (comment 6) was not handled. It still applies with a trivial merge necessary.

I think we should commit that patch.

Wan-Teh had requested to rename "cet" to "next_et" inside _PR_ErrorCleanupTables, which wasn't done. I'll make that change.

Looking at the names of the other cleanup functions, they all appear to being with _PR_ErrorCleanupTables. Therefore it seems more consistens to rename _PR_ErrorCleanupTables to _PR_CleanupErrorTables. It also allows one to find the function when searching for the string ErrorTable. I'll make that change.

I'm attaching an updated version of Ben's patch. I'm giving it r=kaie based on Wan-Teh's review. I think ny renames don't need a review.

Comment 12

[Kai Engert (:KaiE:)]

hduston: IIUC you attempted to provide an alternative implementation to the patch that already existed. Do you think your patches are better than the one that existed? If yes, could you please explain why?

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 8994783 [details] [diff] [review]
ben-merged-v3.patch

https://hg.mozilla.org/projects/nspr/rev/12f5162b65c9

Comment 14

[Kai Engert (:KaiE:)]

Prior to uplifting to firefox, let's check it passes all firefox tests.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd345196a91432a20dc912f355630419bfb1e945

Comment 15

[Kai Engert (:KaiE:)]

The try build shows multiple crashes in tests on Linux and Android, so something still depends on the items that are cleaned up. I'll revert the commit.

Comment 16

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/9510317eda5b

Comment 17

[hduston]

Attachment: Updated nspr-tpd-memleak.patch

Added zeroing of static tpd_length and tpd_highwater values.

Comment 18

[hduston]

(In reply to Kai Engert (:kaie:) from comment #12)
> hduston: IIUC you attempted to provide an alternative implementation to the
> patch that already existed. Do you think your patches are better than the
> one that existed? If yes, could you please explain why?

The most significant difference, in my view, is that I supplied multiple patches.  Each one addressed one and only one unfreed-allocation-at-exit issue.  This permits each one to be applied independently.

I have attached an updated tpd-memleak patch which zeroes the two static values as the original multi-unfreed patch did. The call to the cleanup function remains in my chosen location, between CleanupEnv and DestroyZones, as my initial analysis leads me to conclude that is the more correct sequence.

Comment 19

[hduston]

(In reply to Kai Engert (:kaie:) from comment #14)
> Prior to uplifting to firefox, let's check it passes all firefox tests.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bd345196a91432a20dc912f355630419bfb1e945

If you decide to commit/test any of my patches, would you mind indulging me and run a "before" baseline, so as to verify that none of the failures preexist these patches?

Comment 20

[hduston]

Attachment: Even more updated nspr-tpd-memleak.patch

Corrected to apply against a vanilla tree not already patched with the nspr-errortable-memleak.patch.

Comment 21

[hduston]

Attachment: tpd patch to be against 4.19

I just realized I was patching against a 4.10 tree instead of a 4.19 tree.  Oops.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.