283234 - jsarena.c arena reuse + reallocation can lose other allocations.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Dylan Salisbury]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

In JS_ArenaAllocate(), if the allocation is oversized, the function may select
an arena from the free list that is larger than necessary, as noted in this comment:

/*
 * Insist on exact arenasize match if nb is not greater than
 * arenasize.  Otherwise take any arena big enough, but not by
 * more than gross + arenasize.
 */

But this leaves room for more (small) allocations to be added after this
allocation in the same arena, which violates the “every oversized allocation is
the only allocation in its arena” rule.  When the oversized allocation is
subsequently grown, other allocations will be lost.

This bug doesn’t get hit when all oversized allocations are powers of two, but
the Sprinter code does seem to be something that can create unusually-sized
allocations.

One way to fix this would be to push avail all the way out to limit for any
arena with an oversized allocation, and change the grow/realloc functions to
simply leave the arena alone if the caller wants to grow the allocation into
space that the arena already has.


Reproducible: Always

Steps to Reproduce:

Comment 1

[Dylan Salisbury]

Attachment: Sample program to reproduce the bug

Comments in this file show where the smaller allocation will get lost.

Comment 2

[Brendan Eich [:brendan]]

This could explain talkback from crashes to-do with NewOrRecycledNode and other
arena-based allocators.  I'd like to fix it ASAP for impending releases.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: simplest fix

I don't see the point in messing around here.

/be

Comment 4

[John Bandhauer]

Don't you need to fix the comment right above that?

Comment 5

[Brendan Eich [:brendan]]

Jband, yeah: I was rushing to get into Firefox 1.0.1, but that train has left.
Here's the revised comment I'll check into the trunk:

                 /*
-                 * Insist on exact arenasize match if nb is not greater than
-                 * arenasize.  Otherwise take any arena big enough, but not by
-                 * more than gross + arenasize.
+                 * Insist on exact arenasize match to avoid leaving alloc'able
+                 * space after an oversized allocation as it grows.
                  */

/be

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 175232 [details] [diff] [review]
simplest fix

r=shaver, thanks for the IRC explanations as well.

Comment 7

[Brendan Eich [:brendan]]

Fixed on trunk.  Candidate for future dot releases off branches, but not gonna
hold up 1.0.1 (therefore ditto 1.7.6).

/be