Closed
Bug 281763
Opened 19 years ago
Closed 19 years ago
PL Arena Free List leaks one arena when second arena is taken
Categories
(NSPR :: NSPR, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
4.6
People
(Reporter: nelson, Assigned: wtc)
Details
(Keywords: memory-leak)
Attachments
(3 files, 1 obsolete file)
1.26 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
3.82 KB,
application/zip
|
Details | |
798 bytes,
patch
|
caillon
:
approval1.8b+
|
Details | Diff | Splinter Review |
The function PL_ArenaAllocate, which was rewritten for NSPR 4.2, has a bug which causes it to leak the first arena in the free list every time that it is able to satisfy the allocation request with the second arena in the free list. In that case, the free list head is set to point to the third arena in the list, the second arena is returned, and the first arena is leaked. The bug has been present since NSPR 4.2, and may also be present in some NSPR 4.1.x builds. I will attach to this bug the source for a small test program that reproduces the leak, leaking tens of megabytes per second. I will also attach to this bug a patch that fixes it. A 1 character change. NSS users may workaround this bug by setting the environment variable NSS_DISABLE_ARENA_FREE_LIST to the value 1. This causes NSS to force arenas to be actually freed, and not put into the arena free list, when the arenapool is freed. Julien Pierre and I spent most of the last 2 days finding this bug, and about an hour to fix it and write a test program to reproduce it.
Reporter | ||
Comment 1•19 years ago
|
||
Julien says this bug is also in NSPR 4.1.6. We *may* need an NSPR 4.5.2 release to fix this. That's not sure yet, so I'm targeting this at NSPR 4.6 for now.
Priority: -- → P1
Target Milestone: --- → 4.6
Reporter | ||
Comment 2•19 years ago
|
||
Wan-Teh, Please review this patch.
Attachment #173937 -
Flags: review?(wtchang)
Reporter | ||
Comment 3•19 years ago
|
||
I created my test program as a new subdirectory under security/nss/cmd. This zip file contains the Makefile, manifest and one .c file. The test program uses NSS's PORT_Arena functions to allocate and free. It does a million loops, allocating arenapools, arenas, and freeing them. When run with NSS_DISABLE_ARENA_FREE_LIST=1 in the environment, it does not grow as it runs. Likewise, with my NSPR patch in place, it does not grow. But with NSPR 4.1.6 and later, the programs leaks many megabytes per second and runs out of memory. BTW, in NSS, this leak occurs when softoken tries to allocate a very large arena to hold a CRL, shortly after it has previously done so. Along the way to finding this bug, we also found bug 281761 in that same code path.
Assignee | ||
Comment 4•19 years ago
|
||
Added Brendan to the cc list because he knows what other Mozilla components have copies of the PLArena code.
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 173937 [details] [diff] [review] Patch, 1 charfacter fix r=wtc. I also highly recommend that in the for loop below > for ( a = p = arena_freelist; a != NULL ; p = a, a = a->next ) { we initialize p to NULL instead of arena_freelist. This means sense because if a points to the head of the arena freelist, there is no previous node, hence p should be NULL. If we do this, we can also change the following if statement + if ( a == arena_freelist ) arena_freelist = a->next; else p->next = a->next; to + if ( p == NULL ) arena_freelist = a->next; else p->next = a->next; I reviewed the copy of PLArena I know about: jsarena.c. The equivalent function JS_ArenaAllocate has been rewritten and it doesn't have this bug. (Its equivalent of our p variable is bp, which has one more level of indirection than our p variable and is initialized to &arena_freelist. That's why the jsarena.c code doesn't have this bug. I seem to recall David Hyatt copied plarena.{h,c} to Apple's Safari browser (bug 179828), so they should examine their sources for this bug.
Attachment #173937 -
Flags: review?(wtchang) → review+
Assignee | ||
Comment 6•19 years ago
|
||
This bug was introduced in NSPR 4.1.1, when we rewrote PL_ArenaAllocate (plarena.c, cvs rev. 3.7 or rev. 3.6.2.1; see attachment 29646 [details] [diff] [review] in bug 45343). My comment 5 about JS_ArenaAllocate having been rewritten was incorrect. I observed a big difference between PL_ArenaAllocate and JS_ArenaAllocate but made the wrong conclusion; it was actually PL_ArenaAllocate that was rewritten.
Version: 4.2 → 4.1.1
Reporter | ||
Comment 7•19 years ago
|
||
I believe this patch embodies Wan-Teh's suggested alternative fix. I like this better, too. Looks like it should work. Haven't tested it. Wan-Teh, please check in whatever you prefer.
Assignee | ||
Comment 8•19 years ago
|
||
Nelson, your patch is exactly what I described. I checked in your patch with a minor change (I like to see related variables initialized together).
Attachment #174011 -
Attachment is obsolete: true
Assignee | ||
Comment 9•19 years ago
|
||
Alternative patch v2.1 has been checked in on the NSPR tip (NSPR 4.6).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 10•19 years ago
|
||
Comment on attachment 174060 [details] [diff] [review] alternative patch v2.1 (checked in) Requesting Mozilla 1.8 Beta approval. This patch fixes a memory leak in PLArena code. Nelson Bolyard wrote the patch. I reviewed and edited it. The risk of this patch is low.
Attachment #174060 -
Flags: approval1.8b?
Comment 11•19 years ago
|
||
Comment on attachment 174060 [details] [diff] [review] alternative patch v2.1 (checked in) a=caillon for 1.8beta.
Attachment #174060 -
Flags: approval1.8b? → approval1.8b+
Assignee | ||
Comment 12•19 years ago
|
||
Comment on attachment 174060 [details] [diff] [review] alternative patch v2.1 (checked in) I checked in this patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.8 Beta).
You need to log in
before you can comment on or make changes to this bug.
Description
•