Closed Bug 629653 Opened 9 years ago Closed 9 years ago

PL_ArenaRelease can fail to "free" some marks

Categories

(NSPR :: NSPR, defect, P2, major)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: javakit, Assigned: cjones)

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19
Build Identifier: a memory leak for [NSPR arena] if I use PL_ARENA_MARK and PL_ARENA_RELEASE

In the c source file of ${MOZ_SRC}/nsprpub/lib/ds/plarena.c, the function
PL_ArenaRelease(PLArenaPool *pool, char *mark) may cause memory leaks for
the wanted-to-be-free arena blocks. The reason is the code 'if':
 if (PR_UPTRDIFF(mark, a->base) < PR_UPTRDIFF(a->avail, a->base)) 
should be:
 if (PR_UPTRDIFF(mark, a->base) <= PR_UPTRDIFF(a->avail, a->base)) 

I build a block list: A-->B-->C and want to free the B block and C block. At the A block I use PL_ARENA_MARK. Then, after the B and C is created and used, I use PL_ARENA_RELEASE in-order-to free the B and C block. But the B and C are memory leaks!


Reproducible: Always

Steps to Reproduce:
[1]、I build an arena pool for long time used:
{
PLArenaPool mPool;
PL_InitArenaPool(&mPool, "myArenaPool", 1024, sizeof(void*)-1);
}
[2]、class MyObject new operator: 
//the aSize is 700 bytes
void * operator MyObject::new(size_t aSize){
    void * tmp;
    PL_ARENA_ALLOCATE(tmp, &mPool, aSize);
    return tmp;
}
void doMyFunc(){
        //the A block is may be the pool header or (A->limit - A->avail) is 
        //less than 700 bytes
	const PLArena * A = mPool.current;        
	void * mark = PL_ARENA_MARK(&temp.mPool);
        //want to get 700 bytes memory, it may create another B
	MyObject * p1 = new MyObject();//need 700 bytes
	const PLArena * B = mPool.current;
        MyObject * p2 = new MyObject();//need 700 bytes
	const PLArena * C = mPool.current;

        //key: I want to free B and C, but it may does not work!
	PL_ARENA_RELEASE(&temp.mPool,mark);
}
The doMyFunc() is frequently invoked and each time there must be 2 blocks is memory leaks! If I don't free mPool, the leak memory will increase higher and higher!



Actual Results:  
memory block leaks increase higher and higher. If I modify the code in
${MOZ_SRC}/nsprpub/lib/ds/plarena.c from:
 if (PR_UPTRDIFF(mark, a->base) < PR_UPTRDIFF(a->avail, a->base)) 
to:
 if (PR_UPTRDIFF(mark, a->base) <= PR_UPTRDIFF(a->avail, a->base)) 

the memory is OK!

Expected Results:  
no memory leaks

The above code is simply abstracted from my real code. But I think the problem is descripted obviously.
Chris, could you take a look at this?
Assignee: nobody → wtc
Status: UNCONFIRMED → NEW
Component: General → NSPR
Ever confirmed: true
Product: Core → NSPR
QA Contact: general → nspr
Version: unspecified → other
Assignee: wtc → jones.chris.g
Status: NEW → ASSIGNED
OS: Windows XP → All
Priority: -- → P2
Hardware: x86 → All
Target Milestone: --- → 4.8.8
In fact, the function PL_ArenaRelease in ${MOZ_SRC}/nsprpub/lib/ds/plarena.c should be:
PR_IMPLEMENT(void) PL_ArenaRelease(PLArenaPool *pool, char *mark)
{
    PLArena *a;

    //for (a = pool->first.next; a; a = a->next) {
      for (a = &pool->first; a; a = a->next) {// see diff from above line
        //if (PR_UPTRDIFF(mark, a->base) < PR_UPTRDIFF(a->avail, a->base)) {
          if (PR_UPTRDIFF(mark, a->base) <= PR_UPTRDIFF(a->avail, a->base)) {
            a->avail = (PRUword)PL_ARENA_ALIGN(pool, mark);
            FreeArenaList(pool, a, PR_FALSE);
            return;
        }
    }
}

This means:
(1) the 'a' pointer should begin from the pool's header. Because we may make a mark from the pool's header just before the pool is used to allocat. 
(2) The 'if' should also contain the condition of mark == a->avail. So I use the 
"<=" instead of the "<".
Thanks, "shameiguoren"!  You're absolutely right.

The two bugs in the logic are exposed by this code

  PLAreaPool pool;
  PL_InitArenaPool(&pool, 1024, 7);
  void* mark = PL_ARENA_MARK(&pool);
  void* dummy;
  PL_ARENA_ALLOCATE(dummy, &pool, 512);
  PL_ARENA_RELEASE(&pool, mark);

Because PL_ArenaRelease() starts searching at the second PLArena |first.next|, not its |first|, it misses the mark above that points to |first| and fails to "free" the arena it should have.  But if that bug were fixed, it would still fail to free the arena because the mark of |first| would fail the |PR_UPTRDIFF(mark, a->base) < PR_UPTRDIFF(a->avail, a->base)| test.  Starting from the header arena and checking <= are the right fixes.

The </<= bug would also cause failure to free if a PLArenaPool were marked, then the next PL_ArenaAllocate() couldn't be satisfied by the marked PLArena and a new one had to be allocated.

It looks like our only in-tree user of PL_ARENA_MARK() is NSS.  I'm not sure how badly these bugs affect NSS.
Attachment #509689 - Flags: review?(brendan)
Summary: c++ : a memory leak for [NSPR arena] if I use PL_ARENA_MARK and PL_ARENA_RELEASE → PL_ArenaRelease can fail to "free" some marks
Comment on attachment 509689 [details] [diff] [review]
Fix two cases where PL_ArenaRelease fails to "free" memory it should (checked in)

r=wtc.

Here are the NSS calls to PL_ArenaRelease:
http://mxr.mozilla.org/mozilla-central/ident?i=PORT_ArenaRelease
http://mxr.mozilla.org/mozilla-central/ident?i=PORT_ArenaZRelease

There are a lot of calls.  Since you need to hit the mark == a->avail
condition, and the memory will still be freed eventually, I don't
think this is a serious bug for NSS.  But it is intellectually very
interesting.  Good job, javakit!
Attachment #509689 - Flags: review+
Wan-Teh, I forget the process here ... looking back over hg blame, it looks like these changes land in NSPR's CVS repo and are imported back to mozilla-central.  Is that correct?
Chris, that is correct.
Yikes. The forked jsarena.{h,cpp} bug was fixed in 2007 for bug 385729. Sorry we did not check plarena and file a bug then!

/be
Attachment #509689 - Flags: review?(brendan) → review+
Brendan: thanks for the JS bug number.  The correct JS bug is
bug 41381 comment 5, attachment 11183 [details] [diff] [review]:
  pre-patch, fixes a bug found by tellme folks in jsarena.[ch]
which was fixed in 2000 as part of a huge patch.

jsarena.h, rev. 3.10:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=jsarena.h&branch=3.29&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&rev1=3.9&rev2=3.10

jsarena.c, rev. 3.11:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/js/src&command=DIFF_FRAMESET&file=jsarena.c&rev2=3.11&rev1=3.10

And you did report this bug (along with other bugs) to NSPR,
in bug 45343 comment 0:

  1.  PL_ARENA_RELEASE will fail to free arenas if called with
  mark == pool->first.avail (== pool->first.limit).

For some reason, we only tried to fix the other bugs, which were
very complicated, and didn't finish the patch.

Let's fix this bug first.  We also need to merge the change to
JS_ARENA_RELEASE in jsarena.h, rev. 3.10.
Can someone explain the change to JS_ARENA_RELEASE in jsarena.h,
rev. 3.10?  I have a link to that change in comment 8 above.
Comment on attachment 509689 [details] [diff] [review]
Fix two cases where PL_ArenaRelease fails to "free" memory it should (checked in)

Patch checked in on the NSPR trunk (NSPR 4.8.8).

Checking in plarena.c;
/cvsroot/mozilla/nsprpub/lib/ds/plarena.c,v  <--  plarena.c
new revision: 3.18; previous revision: 3.17
done
Attachment #509689 - Attachment description: Fix two cases where PL_ArenaRelease fails to "free" memory it should → Fix two cases where PL_ArenaRelease fails to "free" memory it should (checked in)
(In reply to comment #9)
> Can someone explain the change to JS_ARENA_RELEASE in jsarena.h,
> rev. 3.10?  I have a link to that change in comment 8 above.

The change has to exclude the empty interior-allocated first arena in the pool, since the <= condition will be true for it. Hope that helps. Was something else unclear?

/be
Good point.  I think the right fix is for FreeArenaList() to skip the 0-sized PLArenas.
... which the code already does.  OK.
Brendan,

It seems that the change to JS_ARENA_RELEASE is essentially
a no-op.  The "if" block in the JS_ARENA_RELEASE macro contains
three statements:

    _a->avail = (jsuword)JS_ARENA_ALIGN(pool, _m);
    JS_CLEAR_UNUSED(_a);  
    JS_ArenaCountRetract(pool, _m);

The first two are no-op for the empty interior-allocated first
arena in the pool.  So the only difference is that
JS_ArenaCountRetract() won't be called.  But strictly speaking,
this is a retract/fast release, right?

Sorry to belabor a minor point.  I just wanted to make sure
I understand the code.
Igor did the patch way back in 2007 (at least, I found the cvs log entry from him; obviously it had the wrong bug number, per comment 8), so he should weigh in on comment 14.

/be
Hah! How long ago that was. I didn't peer through cvs blame with x-ray vision.

Ok, having looked at the diff, I recall wanting a full release to the zero-width mark in pool->first to call JS_ArenaRelease, in order to free all the separately allocated arenas in the pool.

/be
But if _a, which is (pool)->current, is equal to &(pool)->first,
then there should be no separately allocated arenas in the pool.
This is why I think that change is essentially a no-op; only
the retract/fast release stat counter is affected.
(In reply to comment #18)
> But if _a, which is (pool)->current, is equal to &(pool)->first,
> then there should be no separately allocated arenas in the pool.
> This is why I think that change is essentially a no-op; only
> the retract/fast release stat counter is affected.

Yes, that's right.

/be
I decided to not merge the change to JS_ARENA_RELEASE
in jsarena.h, rev. 3.10.  (See comment 8 and comment 19.)

Marked the bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.