Last Comment Bug 364836 - Assert fail in JS_ArenaRealloc (alignment issues)
: Assert fail in JS_ArenaRealloc (alignment issues)
Status: RESOLVED FIXED
[need testcase]
: fixed1.8.0.10, fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Gavin Reaney
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 239721
  Show dependency treegraph
 
Reported: 2006-12-23 12:52 PST by Gavin Reaney
Modified: 2007-01-17 04:45 PST (History)
6 users (show)
jaymoz: blocking1.8.1.2+
jaymoz: blocking1.8.0.10+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
JS_ArenaRealloc assert fail (for windows/linux js shell) (1.24 KB, patch)
2006-12-23 12:59 PST, Gavin Reaney
no flags Details | Diff | Splinter Review
Stack trace and some debug (2.51 KB, text/plain)
2006-12-23 13:24 PST, Gavin Reaney
no flags Details
Fix for assert fail (1.59 KB, patch)
2006-12-23 15:55 PST, Gavin Reaney
no flags Details | Diff | Splinter Review
Fix for assert fail and header padding (5.41 KB, patch)
2006-12-31 14:41 PST, Gavin Reaney
brendan: review+
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Splinter Review

Description Gavin Reaney 2006-12-23 12:52:16 PST
I've encountered the following assert fail in JS_ArenaRealloc when using picsel's embedding of spidermonkey:

    JS_ASSERT(a->base <= a->avail && a->avail <= a->limit);

The scenario that leads to the failure depends on a couple of factors:

- the alignment requirement of the arena pool
- the target alignment of the addresses returned from malloc

In the failing case, the arena has 8 byte alignment but malloc has returned an address that is 4 byte aligned (but not on an 8 byte boundary). I don't think this particular scenario can arise with firefox/mozilla because windows/wince/linux implementations of malloc always return 8 byte aligned addresses.

In order to reproduce the bug on windows/wince/linux we need to use a pool with an alignment of 16 (any value greater than the malloc target alignment will do).

Stack trace and code to reproduce the problem will follow shortly.
Comment 1 Gavin Reaney 2006-12-23 12:59:29 PST
Created attachment 249527 [details] [diff] [review]
JS_ArenaRealloc assert fail (for windows/linux js shell)

Here's some example code that initialises a few arenas and reallocs them in such a way as to show the problem.

I've created three arenas to make the problem more likely to happen (it only happens when realloc returns an address that isn't a multiple of 16 for this example).
Comment 2 Gavin Reaney 2006-12-23 13:24:47 PST
Created attachment 249531 [details]
Stack trace and some debug

Stack trace for assert fail.

The fix for this particular issue seems relatively easy. The relevant part of JS_ArenaRealloc is here:

    a->base = ((jsuword)a + hdrsz) & ~HEADER_BASE_MASK(pool);
    a->limit = (jsuword)a + gross;
    a->avail = JS_ARENA_ALIGN(pool, a->base + aoff);

The issue is that when we align 'a->base + aoff' this can take it beyond a->limit. To prevent this from happening we must add some alignment slop onto gross.

Apart from this, a couple of related issues are:

1) we should perhaps modify jsopcode to grow the arena exponentially since many reallocs are bad for performance. Or modify JS_ArenaRealloc to do this instead.

2) for the given stack trace, the header has 8 bytes of unused space (prior to the back pointer) that appear not to be necessary (the alignment requirements would still be met without this padding). I'll consider whether there is another/simpler way to derive the correct offset for a->base.
Comment 3 Gavin Reaney 2006-12-23 15:55:08 PST
Created attachment 249538 [details] [diff] [review]
Fix for assert fail

Proposed fix. Brendan, feel free to deflect the reviewing duties elsewhere. I'll probably not be around until next Wednesday or so.

I'll consider some simplifications to the arena header calculations later, but just in case it's of interest, the basic idea I had in mind was to calculate the (overallocation) header size as follows:

hdrsz   = sizeof(JSArena) + sizeof(JSArena**) + HEADER_BASE_MASK(pool);
a->base = ((jsuword)a + hdrsz) & ~HEADER_BASE_MASK(pool);

This feels more intuitive to me and I think it works (when the assert fix is also applied), but I may have missed something.
Comment 4 Gavin Reaney 2006-12-31 09:52:34 PST
This is most likely a dup of bug 239721 (the fix here is still required though).

The suggestions for changes to header padding in comment 3 are equivalent to attachment 149872 [details] [diff] [review] from bug 239721.
Comment 5 Brendan Eich [:brendan] 2006-12-31 11:41:09 PST
Thanks, I should have kept working on that bug, it was mouldering on my list. Marking it blocked by this bug -- we can DUP it if appropriate, or just verify it as FIXED when this is fixed.

/be
Comment 6 Brendan Eich [:brendan] 2006-12-31 11:48:58 PST
(In reply to comment #2)
> Apart from this, a couple of related issues are:
> 
> 1) we should perhaps modify jsopcode to grow the arena exponentially since many
> reallocs are bad for performance. Or modify JS_ArenaRealloc to do this instead.

Could you file a separate bug on this?  It hasn't been reported as a problem, but it's an O(n^2) hazard and we tend to get burned by those at the worst times, after years of getting by on luck.  Note that only expressions require the sprint stack, so unless you make very large expressions and decompile them, you'll tend not to use more than a buffer or two.

> 2) for the given stack trace, the header has 8 bytes of unused space (prior to
> the back pointer) that appear not to be necessary (the alignment requirements
> would still be met without this padding). I'll consider whether there is
> another/simpler way to derive the correct offset for a->base.

This was what I remember trying to finesse when I last looked at bug 239721, but we can fix it in a separate bug too, if you are willing to file one.  Thanks,

/be
Comment 7 Brendan Eich [:brendan] 2006-12-31 11:51:02 PST
(In reply to comment #4)
> This is most likely a dup of bug 239721 (the fix here is still required
> though).
> 
> The suggestions for changes to header padding in comment 3 are equivalent to
> attachment 149872 [details] [diff] [review] from bug 239721.

Or, if you are game, feel free to merge that patch into the one for this bug.

/be
Comment 8 Gavin Reaney 2006-12-31 14:41:33 PST
Created attachment 250067 [details] [diff] [review]
Fix for assert fail and header padding

Merged the patches as suggested and retested. I'll file a new bug for the sprint stack issue soon.
Comment 9 Brendan Eich [:brendan] 2006-12-31 14:53:59 PST
Comment on attachment 250067 [details] [diff] [review]
Fix for assert fail and header padding

r=me, thanks.

/be
Comment 10 Brendan Eich [:brendan] 2007-01-02 12:57:30 PST
Fixed on trunk:

Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.32; previous revision: 3.31
done

Checking branch applicability of patch....

/be
Comment 11 Brendan Eich [:brendan] 2007-01-02 12:58:50 PST
Comment on attachment 250067 [details] [diff] [review]
Fix for assert fail and header padding

Applies (with hunk skew for 1.8.0) to both 1.8* branches without problems.

/be
Comment 12 Daniel Veditz [:dveditz] 2007-01-03 14:48:13 PST
Comment on attachment 250067 [details] [diff] [review]
Fix for assert fail and header padding

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Comment 13 Brendan Eich [:brendan] 2007-01-04 14:45:01 PST
Fixed on the branches:

1.8:   new revision: 3.28.8.4; previous revision: 3.28.8.3
1.8.0: new revision: 3.28.16.3; previous revision: 3.28.16.2

/be
Comment 14 Tony Chung [:tchung] 2007-01-11 10:16:21 PST
Please add testcases to this bug so QA can verify the bug.  Thanks.
Comment 15 Brendan Eich [:brendan] 2007-01-11 10:58:19 PST
(In reply to comment #14)
> Please add testcases to this bug so QA can verify the bug.  Thanks.

The first attachment is a patch to the js shell, but you have to apply it, rebuild the shell using js/src/Makefile.ref, and run it.  This test requires C coding, it can't be written using just JS or other web content.

/be

Note You need to log in before you can comment on or make changes to this bug.