96122 - memory leak in prlink.c

Status: NEW

(NSPR :: NSPR, defect)

Description

[Jeff Hostetler]

in nsprpub/pr/src/linking/prlink.c, the routine _PR_ShutdownLinker()
is #ifdef'd WIN16.  The cleanup there is also applicable to WIN32.

consider changing:
<#if defined(WIN16)
>#if defined(WIN16) || defined(WIN32)

also, make the same change in prinit.c around the call to this
function.

you may also want to consider adding the following to 
the bottom of this function:

    if (_pr_currentLibPath)
    {
	PR_Free(_pr_currentLibPath);
	_pr_currentLibPath = NULL;
    }

this is a static leak rather than a per-connection leak.

Comment 1

[Keyser Sose]

More memory leak goodness thanks jeff.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch. Incomplete (pr_exe_loadmap is not destroyed)

I agree that the cleanup in _PR_ShutdownLinker() is also
applicable to other platforms.	However, I don't think
it is NSPR's responsibility to unload the DLLs for the
application.  Any DLLs loaded by the application via
PR_LoadLibrary should be unloaded by the application via
PR_UnloadLibrary.  Therefore, I left the original
_PR_ShutdownLinker() as is (still ifdef's WIN16) and
wrote a new implementation that destroys the static
data created by NSPR.  This patch is not complete
because I haven't figured out how to destroy
pr_exe_loadmap.

While I was at it, I also fixed the problem that
the allocation and free routines for _pr_currentLibPath
and lib->name were mismatched.	They are now allocated
by either malloc() or strdup() and freed by free().

Comment 3

[Wan-Teh Chang]

Comment on attachment 62906 [details] [diff] [review]
Proposed patch.  Incomplete (pr_exe_loadmap is not destroyed)

Checked in this patch into the tip of NSPR.

Comment 4

[Wan-Teh Chang]

I just wanted to note that this bug is not fixed until
pr_exe_loadmap has been dealt with.

Comment 5

[Boris Zbarsky [:bzbarsky]]

clearing keywords to reflect that the patch is in.

Comment 6

[Wan-Teh Chang]

In comment 2, I didn't write down why it's hard to
destroy pr_exe_loadmap.  Now I don't remember what
I thought the issues were.

I just studied all the code that uses pr_exe_loadmap.
The only issue I found is that PR_LoadStaticLibrary
may reference pr_exe_loadmap->dlh:
http://mxr.mozilla.org/nspr/source/nsprpub/pr/src/linking/prlink.c#1613

1611     lm->name = strdup(name);
1612     lm->refCount    = 1;
1613     lm->dlh         = pr_exe_loadmap ? pr_exe_loadmap->dlh : 0; <=== REFERENCE
1614     lm->staticTable = slt;
1615     lm->next        = pr_loadmap;
1616     pr_loadmap      = lm;

So we can destroy pr_exe_loadmap only if no lm on pr_loadmap
has lm->staticTable != NULL and lm->dlh == pr_exe_loadmap->dlh.

Comment 7

[hduston]

Attachment: nspr-loadmap-memleak.patch

Proposed patch to correctly destroy pr_exe_loadmap in PR_ShutdownLinker() (nspr/pr/src/linking/prlink.c)

Comment 8

[hduston]

I found this old bug, and I have a proposed patch that might destroy pr_exe_load_map correctly.

Call PR_UnloadLibrary(pr_exe_loadmap);
and then set it to NULL p_exe_load_map = NULL;

This should bypass the reference in PR_LoadStaticLibrary since it checks for NULL before de-referencing pr_exe_loadmap

Comment 9

[Kai Engert (:KaiE:)]

hduston, IIUC Wan-Teh's comment 6 means: Unconditionally destroying pr_exe_loadmap, like suggested in your patch, shouldn't be done. Rather, you'd have to verify the conditions described in comment 6 are false, and only then destroy. If true, you'd have to allow it to leak.

Comment 10

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

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.