Last Comment Bug 338121 - probably something badly wrong with JS_ARENA_ALLOCATE_CAST
: probably something badly wrong with JS_ARENA_ALLOCATE_CAST
Status: VERIFIED FIXED
[sg:critical?]
: verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-05-16 03:55 PDT by georgi - hopefully not receiving bugspam
Modified: 2007-09-18 15:07 PDT (History)
10 users (show)
brendan: blocking1.8.1+
dveditz: blocking1.8.0.5+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
potential exploit (503 bytes, text/html)
2006-05-16 03:55 PDT, georgi - hopefully not receiving bugspam
no flags Details
Something like this (4.14 KB, patch)
2006-05-16 07:13 PDT, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
how about this? (3.76 KB, patch)
2006-05-18 08:15 PDT, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes (551 bytes, text/html)
2006-05-19 03:45 PDT, georgi - hopefully not receiving bugspam
no flags Details
argument a little less than 4GB, fast, no error (554 bytes, text/html)
2006-05-19 03:45 PDT, georgi - hopefully not receiving bugspam
no flags Details
js1_5/Function/regress-338121-01.js (2.29 KB, text/plain)
2006-06-15 10:33 PDT, Bob Clary [:bc:]
no flags Details
js1_5/Function/regress-338121-02.js (2.37 KB, text/plain)
2006-06-15 10:34 PDT, Bob Clary [:bc:]
no flags Details
js1_5/Function/regress-338121-03.js (2.46 KB, text/plain)
2006-06-15 10:35 PDT, Bob Clary [:bc:]
no flags Details
1.0.x clean (2.48 KB, patch)
2006-08-08 08:21 PDT, Alexander Sack
no flags Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2006-05-16 03:55:04 PDT
probably something badly wrong with JS_ARENA_ALLOCATE_CAST

[Switching to Thread -1222125344 (LWP 5210)]
0xb7e243c3 in Function (cx=0x869cbd8, obj=0x88688f0, argc=39, argv=0x88d9a70, 
    rval=0xbf8b50d8) at /opt/joro/firefox/mozilla/js/src/jsfun.c:1854
1854                (void) js_strncpy(cp, JSSTRING_CHARS(arg), arg_length);
Current language:  auto; currently c
(gdb) x/i $eip
0xb7e243c3 <Function+1654>:     repz movsb %ds:(%esi),%es:(%edi)
(gdb) x/4x $edi
0x88fc000:      Cannot access memory at address 0x88fc000
(gdb) x/4x $edi-16
0x88fbff0:      0x00760076      0x00760076      0x00760076      0x00760076
(gdb) p n
$1 = 38
(gdb) p arg_length
$2 = 33554432
(gdb) p args_length
$3 = 1275068453
(gdb)
(gdb) p/x args_length
$4 = 0x4c000025
(gdb)

bc:
(2^32-(2*38*33554432))
1744830464

((38*33554432))
1275068416
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1304429648 (LWP 5230)]
0xb7c92bf3 in PR_SetThreadPrivate (index=3, priv=0x0)
    at /opt/joro/firefox/mozilla/nsprpub/pr/src/threads/prtpd.c:208
208         else if (self->privateData[index] && _pr_tpd_destructors[index])
(gdb) x/i $eip
0xb7c92bf3 <PR_SetThreadPrivate+340>:   mov    (%eax),%eax
(gdb) p/x $eax
$5 = 0x760082
(gdb)


on virgin firefox 1503 from ftp.mozilla.org

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1222170400 (LWP 5249)]
0xb7eab88f in ?? ()
(gdb) x/i $eip
0xb7eab88f:     repz movsb %ds:(%esi),%es:(%edi)
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread -1233278032 (LWP 5257)]
0xb7e49597 in ?? ()
(gdb) x/i $eip
0xb7e49597:     call   *0x4(%eax)
(gdb) p/x $eax
$1 = 0x760076
(gdb)
Comment 1 georgi - hopefully not receiving bugspam 2006-05-16 03:55:51 PDT
Created attachment 222159 [details]
potential exploit
Comment 2 georgi - hopefully not receiving bugspam 2006-05-16 05:18:14 PDT
on macosx ppc i get something like SIGILL (after i continue with gdb)
Comment 3 Brendan Eich [:brendan] 2006-05-16 05:52:36 PDT
Old bug in JS_ARENA_ALLOCATE_CAST, not caught till now -- the fix for bug 279273 just rearranged terms.

Blake kindly offered to fix this.

/be
Comment 4 Blake Kaplan (:mrbkap) 2006-05-16 07:13:57 PDT
Created attachment 222178 [details] [diff] [review]
Something like this

I haven't vetted all of the other users of JS_ARENA_ALLOCATE_CAST yet. That might take a bunch of work, since some functions take nb and pass that directly, so a simple grep-and-check won't work.

I also didn't really calculate the maximum size. This uses 0x4000000 which is probably a little on the conservative size.
Comment 5 georgi - hopefully not receiving bugspam 2006-05-16 07:54:03 PDT
+        JS_ASSERT(_nb < JS_ARENA_MAX_SIZE);

isn't JS_ASSERT noop in non debug builds and just spamming thing in debug builds?
Comment 6 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-16 08:37:40 PDT
JS_ASSERT is fatal in debug builds, and compiled out entirely in release builds.
Comment 7 georgi - hopefully not receiving bugspam 2006-05-16 12:38:49 PDT
(In reply to comment #6)
> JS_ASSERT is fatal in debug builds, and compiled out entirely in release
> builds.
> 

can you explain in technical terms what do you mean by "compiled out entirely in release builds" for the non native indianz?
Comment 8 Blake Kaplan (:mrbkap) 2006-05-16 14:32:49 PDT
(In reply to comment #7)
> can you explain in technical terms what do you mean by "compiled out entirely
> in release builds" for the non native indianz?

In a release build, whenever the preprocessor sees JS_ASSERT(foo), it replaces it with |(void) 0|. The compiler never sees any calls to JS_Assert.
Comment 9 georgi - hopefully not receiving bugspam 2006-05-18 02:28:00 PDT
managed to execute code on linux, but under gdb.
Comment 10 Brendan Eich [:brendan] 2006-05-18 03:17:56 PDT
Comment on attachment 222178 [details] [diff] [review]
Something like this


> /*
>- *
>+ * The JS arena allocator is not a general-purpose allocator. In order for the
>+ * code in this file to work properly, all sizes passed in must be smaller
>+ * than the size described by this macro.

Nits: "less than" instead of "smaller than" and "given" instead of "described".

r=me with those picked.

/be
Comment 11 georgi - hopefully not receiving bugspam 2006-05-18 05:10:26 PDT
so since JS_ASSERT seems a noop in release builds, this patch actually fixes only jsfun.c and not the underlying problem?

doesn't seem very reasonable to be carefull in every arena usage instead of the arena to check the limits.
Comment 12 Brendan Eich [:brendan] 2006-05-18 08:15:05 PDT
Created attachment 222499 [details] [diff] [review]
how about this?

The data flow is less amenable to inspection than I had hoped, and I could see things getting big for callers who don't use JS_ARENA_ALLOCATE_TYPE (where the type is guaranteed to be small).  So this refactors the macros to avoid penalizing the _TYPE variant.

/be
Comment 13 Blake Kaplan (:mrbkap) 2006-05-18 08:34:37 PDT
Comment on attachment 222499 [details] [diff] [review]
how about this?

This is *much* better.
Comment 14 Brendan Eich [:brendan] 2006-05-18 10:13:23 PDT
Fixed on trunk:

Checking in jsarena.c;
/cvsroot/mozilla/js/src/jsarena.c,v  <--  jsarena.c
new revision: 3.29; previous revision: 3.28
done
Checking in jsarena.h;
/cvsroot/mozilla/js/src/jsarena.h,v  <--  jsarena.h
new revision: 3.22; previous revision: 3.21
done

/be
Comment 15 Brendan Eich [:brendan] 2006-05-18 10:15:12 PDT
Comment on attachment 222499 [details] [diff] [review]
how about this?

I haven't checked into the 1.8 branch yet, note well - not sure it is open, busy with other stuph.  Blake, save me!

/be
Comment 16 georgi - hopefully not receiving bugspam 2006-05-19 03:43:33 PDT
notice the following:

1. when trying to make Function with cumulative arguments a little over 2GB, Function.toString() doesn't show the arguments, only the body.

2. when trying to make Function with cumulative arguments a little less than 4GB, the script stops, but there is no error of any kind.

is this expected?

testcase to follow.
Comment 17 georgi - hopefully not receiving bugspam 2006-05-19 03:45:05 PDT
Created attachment 222607 [details]
arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes
Comment 18 georgi - hopefully not receiving bugspam 2006-05-19 03:45:59 PDT
Created attachment 222608 [details]
argument a little less than 4GB, fast, no error
Comment 19 georgi - hopefully not receiving bugspam 2006-05-19 03:53:11 PDT
Comment on attachment 222607 [details]
arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes

s/ram/vm
Comment 20 Daniel Veditz [:dveditz] 2006-05-26 11:25:46 PDT
Please land on trunk and for 1.8.1
Comment 21 Daniel Veditz [:dveditz] 2006-06-05 11:23:51 PDT
Please land on trunk and for 1.8.1 for baking before we approve it for 1.8.0.5


Comment 22 Blake Kaplan (:mrbkap) 2006-06-06 17:04:58 PDT
The fix already was checked into the trunk.
Comment 23 Jay Patel [:jay] 2006-06-08 14:10:51 PDT
Blake:  Can you get this checked in on the 1.8.1 branch ASAP.  Once we have some bake time there, we can approve it for 1.8.0.5 next week.
Comment 24 Daniel Veditz [:dveditz] 2006-06-12 11:31:14 PDT
Comment on attachment 222499 [details] [diff] [review]
how about this?

approved for 1.8.0 branch, a=dveditz for drivers
Comment 25 Daniel Veditz [:dveditz] 2006-06-12 11:32:19 PDT
reassigning to mrbkap for branch check-in while brendan's out.
Comment 26 Blake Kaplan (:mrbkap) 2006-06-12 17:35:12 PDT
Fix checked into the 1.8.1 branch.
Comment 27 Bob Clary [:bc:] 2006-06-15 10:32:18 PDT
Georgi, how important are the alerts in the second two testcases? Are they required? Can they be replaced with anything that would just convert the function to a string?
Comment 28 Bob Clary [:bc:] 2006-06-15 10:33:48 PDT
Created attachment 225721 [details]
js1_5/Function/regress-338121-01.js
Comment 29 Bob Clary [:bc:] 2006-06-15 10:34:26 PDT
Created attachment 225723 [details]
js1_5/Function/regress-338121-02.js
Comment 30 Bob Clary [:bc:] 2006-06-15 10:35:04 PDT
Created attachment 225724 [details]
js1_5/Function/regress-338121-03.js
Comment 31 Bob Clary [:bc:] 2006-06-15 10:40:36 PDT
I am not entirely sure that these tests match exactly what Georgi was testing.

Note these tests do not complete in the trunk browser on winxp. The final 'Done' is not written out. I think I will end up moving these into the js1_5/GC/ directory (ditto 338001) since if they ever do complete they would be a pita to run with the other tests. Maybe /GC/ wasn't such a good name for that dir after all. Maybe it should have been pita.

I also get a crash in a recent trunk winxp build when attempting to step through these tests in venkman.

+		this	0x00000030 {mRawPtr=??? }	const nsCOMPtr<nsIScriptTimeoutHandler> * const

      nsDerivedSafe<T>*
      operator->() const
        {
          NS_PRECONDITION(mRawPtr != 0, "You can't dereference a NULL nsCOMPtr with operator->().");
          return get();
        }

>	gklayout.dll!nsCOMPtr<nsIScriptTimeoutHandler>::operator->()  Line 849 + 0x3 bytes	C++
 	gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x00000000)  Line 6289 + 0xb bytes	C++
 	gklayout.dll!nsGlobalWindow::SetScriptsEnabled(int aEnabled=0x00000001, int aFireTimeouts=0x00000001)  Line 1850	C++
 	gklayout.dll!nsGlobalWindow::SetScriptsEnabled(int aEnabled=0x00000001, int aFireTimeouts=0x00000001)  Line 1842 + 0x5c bytes	C++
 	gklayout.dll!nsJSContext::SetScriptsEnabled(int aEnabled=0x00000001, int aFireTimeouts=0x00000001)  Line 2878	C++
 	jsd3250.dll!jsdContext::SetScriptsEnabled(int _rval=0x00000001)  Line 1654	C++
 	xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x0000000f, unsigned int methodIndex=0x00000001, unsigned int paramCount=0x0012d7e0, nsXPTCVariant * params=0x30028b96)  Line 102	C++
...
yada yada yada
...
Comment 32 Blake Kaplan (:mrbkap) 2006-06-15 11:28:47 PDT
(In reply to comment #31)
> I also get a crash in a recent trunk winxp build when attempting to step
> through these tests in venkman.

That was bug 341535.
Comment 33 Blake Kaplan (:mrbkap) 2006-06-15 18:10:16 PDT
Fix checked into the 1.8.0 branch.
Comment 34 georgi - hopefully not receiving bugspam 2006-06-15 23:48:27 PDT
(In reply to comment #27)
> Georgi, how important are the alerts in the second two testcases? Are they
> required? Can they be replaced with anything that would just convert the
> function to a string?
> 

the alerts are not necessary.
Comment 35 georgi - hopefully not receiving bugspam 2006-06-16 00:57:00 PDT
(In reply to comment #31)
> I am not entirely sure that these tests match exactly what Georgi was testing.
> 

don't crash on any of the testcases on trunk, but also don't get js error of any kind. don't know if this is expected.
Comment 36 Jay Patel [:jay] 2006-06-26 16:26:35 PDT
v.fixed on 1.8.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US;
rv:1.8.0.5) Gecko/20060626 Firefox/1.5.0.5, no crashes, but same experience as G30rgi, no JS erros.
Comment 37 Bob Clary [:bc:] 2006-07-02 14:34:50 PDT
on 1.8.0.5, 1.8.1, 1.9a1 browser|shell 20060630 linux the tests do not crash and the tests do complete. 

On windows/mac(ppc|tel) I don't see crashes with 1.8.0.5, 1.8.1, 1.9a1 browser|shell but they do not appear to complete. That is the line writeLineToLog('Done'); never appears  in the browser and the reportCompare call does not appear to be made. 

I can't verify that this is really fixed since I don't understand this behavior.
Comment 38 georgi - hopefully not receiving bugspam 2006-07-03 03:34:34 PDT
big strings still don't give function arguments, only the body.
Comment 39 Bob Clary [:bc:] 2006-07-05 23:35:21 PDT
(In reply to comment #38)
> big strings still don't give function arguments, only the body.
> 

I don't understand what you mean, but verifying fixed no crashes on 20060703 builds.

Comment 40 georgi - hopefully not receiving bugspam 2006-07-06 03:25:59 PDT
(In reply to comment #39)
> (In reply to comment #38)
> > big strings still don't give function arguments, only the body.
> > 
> 
> I don't understand what you mean, but verifying fixed no crashes on 20060703
> builds.
> 

mean that when constructing a function with big arguments Function.toString() shows only the body without the arguments. if the arguments are small, they are shown.
Comment 41 Bob Clary [:bc:] 2006-07-06 23:18:04 PDT
note to self:js1_5/Function/regress-338121-01.js TIMED OUT dbg-1.9a1_2006070604-plum, dbg-1.8.0.5_2006070604-plum
Comment 42 Alexander Sack 2006-08-08 08:21:32 PDT
Created attachment 232731 [details] [diff] [review]
1.0.x clean
Comment 43 Bob Clary [:bc:] 2006-10-31 17:16:30 PST
see Bug 358975
Comment 44 Bob Clary [:bc:] 2007-02-08 15:02:37 PST
/cvsroot/mozilla/js/tests/js1_5/Function/regress-338121-01.js,v  <--  regress-338121-01.js

/cvsroot/mozilla/js/tests/js1_5/Function/regress-338121-02.js,v  <--  regress-338121-02.js

/cvsroot/mozilla/js/tests/js1_5/Function/regress-338121-03.js,v  <--  regress-338121-03.js
Comment 45 Brendan Eich [:brendan] 2007-05-04 22:12:51 PDT
I'm getting:

Testcase js1_5/Function/regress-338121-03.js failed Bug Number 338121
[ Top of Page ]
Expected exit code 0, got 5
Testcase terminated with signal 0
Complete testcase output was:
js(12721) malloc: *** vm_allocate(size=4026535936) failed (error code=3)
js(12721) malloc: *** error: can't allocate region
js(12721) malloc: *** set a breakpoint in szone_error to debug
./js1_5/Function/regress-338121-03.js:56: out of memory
BUGNUMBER: 338121
STATUS: Issues with JS_ARENA_ALLOCATE_CAST

Is anyone else seeing this?

/be
Comment 46 Brian Crowder 2007-05-04 22:46:24 PDT
I get the same bug.  It looks like the OOM math above the allocate here needs to leave more room for header data and such (ie., ~0 / sizeof(jschar) is not conservative enough), but I'm not sure what the right math is.  Reopening.  If someone wants to provide a hint on what math to use, I'll submit a patch.
Comment 47 Brian Crowder 2007-05-04 22:51:36 PDT
The math I am referring to is in jsfun.c:1901 or thereabouts.
Comment 48 georgi - hopefully not receiving bugspam 2007-05-04 23:42:44 PDT
on macosx get the same malloc error on trunk
Comment 49 georgi - hopefully not receiving bugspam 2007-05-08 00:07:20 PDT
(In reply to comment #47)
> The math I am referring to is in jsfun.c:1901 or thereabouts.
> 

what wrong do you see in the math?

a potential problem on 64 bit systems is this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&rev=3.184#2027
2027 brendan      3.78          argv[(intN)(argc-1)] = STRING_TO_JSVAL(str);

using *signed type* as array index is not nice in the negative case.
Comment 50 georgi - hopefully not receiving bugspam 2007-05-08 04:05:53 PDT
(In reply to comment #49)
> (In reply to comment #47)
> > The math I am referring to is in jsfun.c:1901 or thereabouts.
> > 
> 
> what wrong do you see in the math?
> 
> a potential problem on 64 bit systems is this:
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfun.c&rev=3.184#2027
> 2027 brendan      3.78          argv[(intN)(argc-1)] = STRING_TO_JSVAL(str);
> 
> using *signed type* as array index is not nice in the negative case.
> 

this case seems caught by another check
Comment 51 Blake Kaplan (:mrbkap) 2007-05-08 10:40:48 PDT
So, is this an actual problem? The only math we have right now is to prevent integer overflow on the multiplication. Is the error output here actually a sign of bad things to come? I'd expect malloc to simply return NULL if we passed in a size that was too large for it to deal with (which it appears to do, as well as warn us).
Comment 52 georgi - hopefully not receiving bugspam 2007-05-09 00:57:37 PDT
(In reply to comment #51)
> The only math we have right now is to prevent
> integer overflow on the multiplication. 

looks like addition, not multiplication (short of sizeof(jschar))

so i don't see a problem in firefox and the math in jsfun.c:1901

macosx just whines it can't allocate memory.
Comment 53 Bob Clary [:bc:] 2007-05-09 13:07:07 PDT
(In reply to comment #45)

macosx does that when it runs out of memory. All platforms show out of memory in the shell on this testcase.

Can we reclose this bug?
Comment 54 Blake Kaplan (:mrbkap) 2007-05-09 13:20:03 PDT
Yeah, nothing is wrong here except for Mac OSX being whiny when we ask too much of it.
Comment 55 Bob Clary [:bc:] 2007-09-18 15:07:01 PDT
verified fixed 1.9.0 linux/mac*/windows modulo out of memory known failures.

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