482861 - Guaranteed use-after-free in js_FinishJSONParse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Look six lines up.

Comment 1

[Robert Sayre]

lame, sorry. OS X debug didn't catch this. Valgrind does.

Comment 2

[Robert Sayre]

Attachment: fix

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 366984 [details] [diff] [review]
fix

>diff --git a/js/src/json.cpp b/js/src/json.cpp
>--- a/js/src/json.cpp
>+++ b/js/src/json.cpp
>@@ -586,23 +586,24 @@ js_FinishJSONParse(JSContext *cx, JSONPa
> 
>     if (jp->buffer)
>         js_FinishStringBuffer(jp->buffer);
>     JS_free(cx, jp->buffer);
> 
>     if (!js_RemoveRoot(cx->runtime, &jp->objectStack))
>         return JS_FALSE;
>     JSBool ok = *jp->statep == JSON_PARSE_STATE_FINISHED;
>+    jsval *v = jp->rootVal;
>     JS_free(cx, jp);
> 
>     if (!ok)
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_JSON_BAD_PARSE);
> 
>     if (!JSVAL_IS_PRIMITIVE(reviver) && js_IsCallable(JSVAL_TO_OBJECT(reviver), cx))
>-        ok = Revive(cx, reviver, jp->rootVal);
>+        ok = Revive(cx, reviver, v);

jp->rootVal isn't a rooted value, is it?  In the case it's called from js_json_parse jp->rootVal is the address of the return location for the function, but in the JSAPI case it could be anything.  It looks to me like the rootVal field needs to be a root, and the value you grab here has to be rooted while revive does its work.

Comment 4

[Robert Sayre]

(In reply to comment #3)
> 
> jp->rootVal isn't a rooted value, is it?  

It's designed to be rooted by the caller.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 366984 [details] [diff] [review]
fix

The current way things are set up smells funny, but yeah, as discussed on IRC this is a separate bug -- please file a followup.

Comment 6

[Robert Sayre]

(In reply to comment #5)
> (From update of attachment 366984 [details] [diff] [review])
> The current way things are set up smells funny, but yeah, as discussed on IRC
> this is a separate bug -- please file a followup.

482895

Comment 7

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/cf269afe3d21

Comment 8

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/cf269afe3d21

Comment 9

[Brendan Eich [:brendan]]

Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

/be

Comment 10

[Robert Sayre]

(In reply to comment #9)
> Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

fixed in bug 482899

Comment 11

[Bob Clary [:bc] (inactive)]

hint for a test?

Comment 12

[Jeff Walden [:Waldo]]

Every call to JSON.parse that doesn't hit a rare error condition like OOM will hit this; I found this by running xpcshell tests, in particular <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>, in a Windows debug build.

Comment 13

[Jeff Walden [:Waldo]]

If this problem wasn't introduced in time for 3.1b3 and hadn't yet made its way into 191 (or as soon as this fix does make it in there), I think we should open it, otherwise wait for b4 to open it.  I don't know the checkin/release timelines well enough to confidently and deliberately open or not open it, tho.

Comment 14

[Robert Sayre]

(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; 

Only calls to JSON.parse with the optional reviver argument will hit this.

>I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.

Doesn't seem to bite in opt somehow, but we should wait till b4 to open this bug, imho.

Comment 16

[[e-mail]]

(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.
Can we get this on 1.9.1? Our unit test box is red :-(
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1238422048.1238425884.10078.gz

Comment 17

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5a3f6ade94