Closed Bug 279273 Opened 15 years ago Closed 15 years ago

Broken pointer arithmetic in jsarena code

Categories

(Core :: JavaScript Engine, defect, P3, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: jk, Assigned: brendan)

Details

(Keywords: js1.5, Whiteboard: [have patch])

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux ppc64; en-US; rv:1.7.5) Gecko/20050119 Galeon/1.3.19 (Debian package 1.3.19-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux ppc64; en-US; rv:1.7.5) Gecko/20050119 Galeon/1.3.19 (Debian package 1.3.19-1)

The code in jsarena.h and jsarena.c does some bad pointer arithmetic.  Here's
one example:

for (a = pool->current; a->avail + nb > a->limit; pool->current = a) {

The problem with "a->avail + nb > a->limit" is that the addition might overflow
if a->avail is close to the upper memory boundary (e.g. 0xffffe184 + 8000 ==
0xc4).  The consequence is that the comparision gives an unexpected result and a
crash later on. 
The solution is to use "a->avail > a->limit - nb" instead.

I've seen crashes caused by this on Linux with an ppc64 kernel running a ppc32
mozilla build.  The problem might also be reproducible with AMD64 kernels and
32-bit mozilla builds.  The reason is that a->avail happens to be close the
upper 32-bit memory boundary in these situations.  It probably isn't easily
reproducibe on pure 32-bit and 64-bit systems.

I'm attaching a patch which fixes the problem for me.  It just touches three
instances of bad pointer arithmetic.  You might want to check if there are more.
 (Only the changes in JS_ARENA_ALLOCATE_CAST and JS_ArenaAllocate were needed to
fix the crash I saw.  I just modified JS_ARENA_GROW_CAST because it looked wrong
too.)

(I've originally reported this bug agaist Galeon but it turned out be a problem
in Mozilla. See http://bugzilla.gnome.org/show_bug.cgi?id=164262)


Reproducible: Always
Attached patch JS Arena Fix (obsolete) — Splinter Review
Assignee: general → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: js1.5
Priority: -- → P3
Target Milestone: --- → mozilla1.8beta
Attached patch revised fixSplinter Review
There's no need for the first change to JS_ARENA_GROW_CAST in attachment
172007 [details] [diff] [review], since the allocation at p is JS_ARENA_ALIGN(pool, size) bytes long, and
its last byte is at most at 0xffffffff.  With 32-bit jsuword type, even if p is
up against the end of the 32-bit address space, _a->avail will be zero, the
outer if's condition will be true, and the inner if/else chain will find that
the allocation cannot be grown in place.

/be
Attachment #172007 - Attachment is obsolete: true
Attachment #172299 - Flags: review?(shaver)
Status: NEW → ASSIGNED
Whiteboard: [have patch]
Target Milestone: mozilla1.8beta1 → mozilla1.8beta2
Comment on attachment 172299 [details] [diff] [review]
revised fix

r=shaver
Attachment #172299 - Flags: review?(shaver) → review+
Fixed, thanks to all.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.