probably something badly wrong with JS_ARENA_ALLOCATE_CAST

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: georgi - hopefully not receiving bugspam, Assigned: mrbkap)

Tracking

({verified1.8.0.5, verified1.8.1})

Trunk
x86
Linux
verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(9 attachments)

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)
Created attachment 222159 [details]
potential exploit
(Reporter)

Updated

11 years ago
Component: General → JavaScript Engine
Product: Firefox → Core
on macosx ppc i get something like SIGILL (after i continue with gdb)
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
Assignee: nobody → mrbkap
(Assignee)

Comment 4

11 years ago
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.
Attachment #222178 - Flags: review?(brendan)
+        JS_ASSERT(_nb < JS_ARENA_MAX_SIZE);

isn't JS_ASSERT noop in non debug builds and just spamming thing in debug builds?
JS_ASSERT is fatal in debug builds, and compiled out entirely in release builds.
(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?
(Assignee)

Comment 8

11 years ago
(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.
managed to execute code on linux, but under gdb.
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
Attachment #222178 - Flags: review?(brendan) → review+
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.
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
Attachment #222499 - Flags: review?(mrbkap)
(Assignee)

Comment 13

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

This is *much* better.
Attachment #222499 - Flags: review?(mrbkap) → review+

Updated

11 years ago
Flags: blocking1.8.1+
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.4?
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 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
Attachment #222499 - Flags: approval1.8.0.5?
Attachment #222499 - Flags: approval1.8.0.4?
Attachment #222499 - Flags: approval-branch-1.8.1+
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.
Created attachment 222607 [details]
arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes
Created attachment 222608 [details]
argument a little less than 4GB, fast, no error
Comment on attachment 222607 [details]
arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes

s/ram/vm
Attachment #222607 - Attachment description: arguments a little over 2GB - takes 2.6G RAM, about 5 minutes → arguments a little over 2GB - takes 2.6G VIRTUAL MEMORY, about 5 minutes
Please land on trunk and for 1.8.1
Flags: blocking1.8.0.5?
Flags: blocking1.8.0.5+
Flags: blocking1.8.0.4?
Attachment #222499 - Flags: approval1.8.0.4?
Please land on trunk and for 1.8.1 for baking before we approve it for 1.8.0.5


(Assignee)

Updated

11 years ago
Assignee: mrbkap → brendan
(Assignee)

Comment 22

11 years ago
The fix already was checked into the trunk.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 23

11 years ago
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 on attachment 222499 [details] [diff] [review]
how about this?

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #222499 - Flags: approval1.8.0.5? → approval1.8.0.5+
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
reassigning to mrbkap for branch check-in while brendan's out.
Assignee: brendan → mrbkap
Status: REOPENED → NEW
Status: NEW → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 26

11 years ago
Fix checked into the 1.8.1 branch.
Keywords: fixed1.8.1

Comment 27

11 years ago
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

11 years ago
Created attachment 225721 [details]
js1_5/Function/regress-338121-01.js

Comment 29

11 years ago
Created attachment 225723 [details]
js1_5/Function/regress-338121-02.js

Comment 30

11 years ago
Created attachment 225724 [details]
js1_5/Function/regress-338121-03.js

Comment 31

11 years ago
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
...
Flags: in-testsuite+
(Assignee)

Comment 32

11 years ago
(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.
(Assignee)

Comment 33

11 years ago
Fix checked into the 1.8.0 branch.
Keywords: fixed1.8.0.5
(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.
(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

11 years ago
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.
Keywords: fixed1.8.0.5 → verified1.8.0.5

Comment 37

11 years ago
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.
big strings still don't give function arguments, only the body.

Comment 39

11 years ago
(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.

Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
(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.

Updated

11 years ago
Keywords: fixed1.8.1

Comment 41

11 years ago
note to self:js1_5/Function/regress-338121-01.js TIMED OUT dbg-1.9a1_2006070604-plum, dbg-1.8.0.5_2006070604-plum
Keywords: fixed1.8.1
Whiteboard: [sg:critical?]

Comment 42

11 years ago
Created attachment 232731 [details] [diff] [review]
1.0.x clean

Comment 43

11 years ago
see Bug 358975
Group: security

Comment 44

11 years ago
/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
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

10 years ago
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---

Comment 47

10 years ago
The math I am referring to is in jsfun.c:1901 or thereabouts.
on macosx get the same malloc error on trunk
QA Contact: general → general
(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.
(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
(Assignee)

Comment 51

10 years ago
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).
(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

10 years ago
(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?
(Assignee)

Comment 54

10 years ago
Yeah, nothing is wrong here except for Mac OSX being whiny when we ask too much of it.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago10 years ago
Resolution: --- → FIXED

Comment 55

10 years ago
verified fixed 1.9.0 linux/mac*/windows modulo out of memory known failures.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.