239721 - Bogus code and assertion in jsarena.c

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

From Jens' posting at the bug URL (someone feel free to update with a google
groups permalink):

getting towards a testcase:

/*
  test case

  1) compile spidermonkey with -DDEBUG
  2) run program with valgrind or link to efence - to get realloc in
jsarena.c:250 to move memory

*/
#include <jsapi.h>
#include <string.h>

JSBool
foo(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval) {
  return JS_TRUE;
}

int
main(int argc, char** argv)
{
  JSRuntime *rt;
  JSContext *cx;
  JSObject  *glob;
  JSClass global_class = {
    "global",0,
    JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,JS_PropertyStub,
    JS_EnumerateStub,JS_ResolveStub,JS_ConvertStub,JS_FinalizeStub,
    0,0,0,0,0,0,0,0
  };

  if (!(rt = JS_NewRuntime(8L * 1024L * 1024L))) return 1;
  if (!(cx = JS_NewContext(rt, 2<<13))) return 1;
  if (!(glob = JS_NewObject(cx, &global_class, NULL, NULL))) return 1;
  if (!JS_InitStandardClasses(cx, glob)) return 1;
  JS_DefineFunction(cx, glob, "foo", foo, 1, 0);

  char
*script="x='------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
--------------------------------';foo(x.toSource());";

  jsval rval;
  JS_EvaluateScript(cx, glob, script, strlen(script), "script", 1, &rval);
  if (cx) {
    JS_DestroyContext(cx);
    cx = NULL;
  }
  if (rt) {
    JS_DestroyRuntime(rt);
    rt = NULL;
  }
  JS_ShutDown();
  return 0;
}

sample debug session:
r
[New Thread 16384 (LWP 29733)]

Program received signal SIGABRT, Aborted.
[Switching to Thread 16384 (LWP 29733)]
0x40079571 in kill () from /lib/libc.so.6
bt
#0  0x40079571 in kill () from /lib/libc.so.6
#1  0x4018b761 in pthread_kill () from /lib/libpthread.so.0
#2  0x4018ba6b in raise () from /lib/libpthread.so.0
#3  0x40079324 in raise () from /lib/libc.so.6
#4  0x4007a838 in abort () from /lib/libc.so.6
#5  0x080cc97a in JS_Assert (s=0x80d2d60 "a->base <= a->avail &&
a->avail <= a->limit", file=0x80d2ca0 "jsarena.c",
    ln=274) at jsutil.c:155
#6  0x08053fc8 in JS_ArenaRealloc (pool=0x401e2ef8, p=0x40a16bf8,
size=2, incr=2000) at jsarena.c:274
#7  0x080a12ea in SprintAlloc (sp=0xbffff310, nb=2000) at jsopcode.c:321
#8  0x080a168d in QuoteString (sp=0xbffff310, str=0x401ebc28, quote=34)
at jsopcode.c:412
#9  0x080a1852 in js_QuoteString (cx=0x401e2ebc, str=0x401ebc28,
quote=34) at jsopcode.c:450
#10 0x080c5276 in str_toSource (cx=0x401e2ebc, obj=0x401ebc30, argc=0,
argv=0x40a0401c, rval=0xbffff4b0) at jsstr.c:618
#11 0x080811bd in js_Invoke (cx=0x401e2ebc, argc=0, flags=0) at
jsinterp.c:941
#12 0x0808d8af in js_Interpret (cx=0x401e2ebc, result=0xbffffaa8) at
jsinterp.c:2962
#13 0x08081a68 in js_Execute (cx=0x401e2ebc, chain=0x401eaf48,
script=0x40a0cfac, down=0x0, special=0, result=0xbffffaa8)
    at jsinterp.c:1155
#14 0x08052abe in JS_EvaluateUCScriptForPrincipals (cx=0x401e2ebc,
obj=0x401eaf48, principals=0x0, chars=0x409f7030,
    length=2023, filename=0x80cf968 "script", lineno=1, rval=0xbffffaa8)
at jsapi.c:3530
#15 0x08052a2f in JS_EvaluateUCScript (cx=0x401e2ebc, obj=0x401eaf48,
chars=0x409f7030, length=2023,
    filename=0x80cf968 "script", lineno=1, rval=0xbffffaa8) at jsapi.c:3511
#16 0x0805291f in JS_EvaluateScript (cx=0x401e2ebc, obj=0x401eaf48,
bytes=0x80cf180 "x='", '-' <repeats 197 times>...,
    length=2023, filename=0x80cf968 "script", lineno=1, rval=0xbffffaa8)
at jsapi.c:3479
#17 0x0804a044 in main (argc=1, argv=0xbffffb64) at bug.c:38
up 6
#6  0x08053fc8 in JS_ArenaRealloc (pool=0x401e2ef8, p=0x40a16bf8,
size=2, incr=2000) at jsarena.c:274
274	    JS_ASSERT(a->base <= a->avail && a->avail <= a->limit);
print a
$1 = (JSArena *) 0x40a1480c
print *a
$2 = {next = 0x0, base = 1084311592, limit = 1084313597, avail = 1084313600}
print pool
$3 = (JSArenaPool *) 0x401e2ef8
print p
$4 = (void *) 0x40a16bf8
print size
$5 = 2
print incr
$6 = 2000
print ap
$7 = (JSArena **) 0x401e2ef8
print *ap
$8 = (JSArena *) 0x40a1480c
print b
$9 = (JSArena *) 0x0
print boff
$10 = 16
print aoff
$11 = 2002
print extra
$12 = 8
print hdrsz
$13 = 31
up
#7  0x080a12ea in SprintAlloc (sp=0xbffff310, nb=2000) at jsopcode.c:321
321	        JS_ARENA_GROW_CAST(sp->base, char *, sp->pool, sp->size, nb);
l
316	SprintAlloc(Sprinter *sp, size_t nb)
317	{
318	    if (!sp->base) {
319	        JS_ARENA_ALLOCATE_CAST(sp->base, char *, sp->pool, nb);
320	    } else {
321	        JS_ARENA_GROW_CAST(sp->base, char *, sp->pool, sp->size, nb);
322	    }
323	    if (!sp->base) {
324	        JS_ReportOutOfMemory(sp->context);
325	        return JS_FALSE;

My summary:

boff = a->base - a;						  16
aoff = size + incr;						2002/7d2
extra = sizeof(JSArena **)
        + ((pool->mask < POINTER_MASK)
           ? POINTER_MASK - pool->mask
           : pool->mask - POINTER_MASK);			   8
hdrsz = sizeof(JSArena) + extra + pool->mask;			  31
gross = hdrsz + aoff;						2033
a = realloc(a, gross);					  0x40a1480c
a->base = (a + hdrsz) & ~(pool->mask | POINTER_MASK);     0x40a14828
a->limit = a + gross;					  0x40a14ffd
a->avail = (a->base + aoff + pool->mask) & ~pool->mask;   0x40a15001

/be

Comment 1

[Martijn Wargers (dead)]

This is the google groups permalink:
http://groups.google.com/groups?dq=&hl=en&lr=&ie=UTF-8&oe=UTF-8&threadm=peqr4c.3m2.ln%40karme.myfqdn.de&prev=/groups%3Fhl%3Den%26lr%3D%26ie%3DUTF-8%26oe%3DUTF-8%26group%3Dnetscape.public.mozilla.jseng

Comment 2

[Brendan Eich [:brendan]]

Two bugs here:

1.  HEADER_SIZE adds too much space in the (pool)->mask > POINTER_MASK case --
it should add 0, not (pool)->mask - POINTER_MASK.

2.  jsopcode.c's js_QuoteString and js_DecompileCode pass &cx->tempPool to
INIT_SPRINTER, which wants to keep track of the current allocation size on a
byte-by-byte basis, without rounding up to the alignment modulus for the pool.
But tempPool double-aligns, for portable struct allocation, so JS_ArenaRealloc
will add too much alignment slop (1), and then round avail to end up > limit,
for some calls.

I still want to fix this for 1.7final, but I'm gonna have to patch tonight.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: patch for the first bug

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix for second bug

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 149875 [details] [diff] [review]
proposed fix for second bug

Oops, ignore this.

/be

Comment 6

[chris hofmann]

doesn't look like this is making 1.7

Comment 7

[Daphne H. Luong]

Brendan, we are also hitting this assert during our testing recently while
running with dbmalloc turned on.  Are there any bad side effects we should worry
about?

Comment 8

[John Bandhauer]

Ok. I'm scared. We are clearly sometimes exiting JS_ArenaRealloc with a->avail
being greater than a->limit. I can't convince myself that the callers will never
end up writing on memory the arena doesn't own. I'd be happier if we could
either fix this or if you could convince me that this is benign.

Comment 9

[Robin Bate Boerop]

I think that this bug is fixed now.  Bug 364836 fixed it, no?

Comment 10

[Brian Crowder]

Robin:  Have you proven that with a testcase?

Comment 11

[Robin Bate Boerop]

Nope.  :)  I misspoke.  I should have said "did bug 364836 also fix this bug?"

Comment 12

[Brian Crowder]

It does seem likely that your work will either morph or invalidate or fix this bug, Robin.  This one should probably be resolved anyway, I'm not sure there's a lot more to do here; the code in these patches seems either to have been incorporated elsewhere or else become invalid.  I'll mark as WFM, since I've never managed to reproduce the assertion without changing the code to introduce another bug (not even with what I thought was bogus client code in C).