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)

4.1.1

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: wtc)

Details

(Keywords: memory-leak)

Attachments

(3 files, 1 obsolete file)

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.
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
Wan-Teh, Please review this patch.
Attachment #173937 - Flags: review?(wtchang)
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.
Added Brendan to the cc list because he
knows what other Mozilla components have
copies of the PLArena code.
Status: NEW → ASSIGNED
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+
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
Attached patch alternative patch v2 (obsolete) — Splinter Review
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.
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
Alternative patch v2.1 has been checked in
on the NSPR tip (NSPR 4.6).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
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?
Keywords: mlk
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+
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.

Attachment

General

Created:
Updated:
Size: