338121 - probably something badly wrong with JS_ARENA_ALLOCATE_CAST

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[georgi - hopefully not receiving bugspam]

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]

Attachment: potential exploit

Comment 2

[georgi - hopefully not receiving bugspam]

on macosx ppc i get something like SIGILL (after i continue with gdb)

Comment 3

[Brendan Eich [:brendan]]

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) (inactive)]

Attachment: 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]

+        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 emeritus)]

JS_ASSERT is fatal in debug builds, and compiled out entirely in release builds.

Comment 7

[georgi - hopefully not receiving bugspam]

(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) (inactive)]

(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]

managed to execute code on linux, but under gdb.

Comment 10

[Brendan Eich [:brendan]]

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]

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]]

Attachment: 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) (inactive)]

Comment on attachment 222499 [details] [diff] [review]
how about this?

This is *much* better.

Comment 14

[Brendan Eich [:brendan]]

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]]

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]

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]

Attachment: arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes

Comment 18

[georgi - hopefully not receiving bugspam]

Attachment: argument a little less than 4GB, fast, no error

Comment 19

[georgi - hopefully not receiving bugspam]

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]]

Please land on trunk and for 1.8.1

Comment 21

[Daniel Veditz [:dveditz]]

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) (inactive)]

The fix already was checked into the trunk.

Comment 23

[Jay Patel [:jay]]

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]]

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]]

reassigning to mrbkap for branch check-in while brendan's out.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8.1 branch.

Comment 27

[Bob Clary [:bc] (inactive)]

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] (inactive)]

Attachment: js1_5/Function/regress-338121-01.js

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Function/regress-338121-02.js

Comment 30

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Function/regress-338121-03.js

Comment 31

[Bob Clary [:bc] (inactive)]

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) (inactive)]

(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) (inactive)]

Fix checked into the 1.8.0 branch.

Comment 34

[georgi - hopefully not receiving bugspam]

(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]

(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]]

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] (inactive)]

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]

big strings still don't give function arguments, only the body.

Comment 39

[Bob Clary [:bc] (inactive)]

(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]

(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] (inactive)]

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]

Attachment: 1.0.x clean

Comment 43

[Bob Clary [:bc] (inactive)]

see Bug 358975

Comment 44

[Bob Clary [:bc] (inactive)]

/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]]

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]

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]

The math I am referring to is in jsfun.c:1901 or thereabouts.

Comment 48

[georgi - hopefully not receiving bugspam]

on macosx get the same malloc error on trunk

Comment 49

[georgi - hopefully not receiving bugspam]

(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]

(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) (inactive)]

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]

(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] (inactive)]

(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) (inactive)]

Yeah, nothing is wrong here except for Mac OSX being whiny when we ask too much of it.

Comment 55

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows modulo out of memory known failures.