619255 - GC hazard in JO() from json.cpp due to reuse of root (1.9.[12]-only)

Status: RESOLVED DUPLICATE of bug 616009

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

On 1.9.2 branch JO() from json.cpp incorrectly reuse the passed vp parameter for rooting an iterator leading to a GC hazard. The following example demonstrates this for a debug build of JS shell:

function iter() {
    ({});
    gc();
    yield 1;
}

function create_iter() {
    return iter();
}


JSON.stringify({ get o() {
    var obj = {};
    obj.__iterator__ = create_iter;
    return obj;
}});
~/m/192/js/src> ~/b/js/192dbg32/js ~/s/x.js
Segmentation fault (core dumped)

On trunk we are safe due to the conservative GC scanner.

Comment 1

[Igor Bukanov]

Attachment: fix

The patch copies the initial value of *vp into an extra root.

Comment 2

[Andreas Gal :gal]

Comment on attachment 497715 [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
>@@ -241,20 +241,20 @@ WriteIndent(JSContext *cx, StringifyCont
> 
> static JSBool
> JO(JSContext *cx, jsval *vp, StringifyContext *scx)
> {
>     JSObject *obj = JSVAL_TO_OBJECT(*vp);
> 
>     if (!scx->cb.append('{'))
>         return JS_FALSE;
> 
>-    jsval vec[3] = {JSVAL_NULL, JSVAL_NULL, JSVAL_NULL};
>-    JSAutoTempValueRooter tvr(cx, 3, vec);
>+    jsval vec[] = {JSVAL_NULL, JSVAL_NULL, JSVAL_NULL, *vp};
>+    JSAutoTempValueRooter tvr(cx, JS_ARRAY_LENGTH(vec), vec);
>     jsval& key = vec[0];
>     jsval& outputValue = vec[1];
> 
>     JSObject *iterObj = NULL;
>     jsval *keySource = vp;
>     bool usingWhitelist = false;
> 
>     // if the replacer is an array, we use the keys from it
>     if (scx->replacer && JS_IsArrayObject(cx, scx->replacer)) {

Comment 3

[Igor Bukanov]

Nominating for 1.9.2 due to the critical status.

Comment 4

[LegNeato]

Does this affect 1.9.1 as well?

Comment 5

[LegNeato]

(waiting to approve until we know 1.9.1 is (un)affected.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> Does this affect 1.9.1 as well?

Yes, this affects 1.9.1 - I forgot that the native JSON were introduced on that branch first.

Comment 7

[Igor Bukanov]

Attachment: fix for 191

191 version of the patch needed a trivial merge. Here is a plain diff between patches:

/home/igor/s/> diff json_bug619255.patch json_bug619255_191.patch
4,5c4
< @@ -241,20 +241,20 @@ WriteIndent(JSContext *cx, StringifyCont
<  
---
> @@ -261,20 +261,20 @@ WriteIndent(JSContext *cx, StringifyCont
11c10,11
<      if (!scx->cb.append('{'))
---
>      jschar c = jschar('{');
>      if (!scx->callback(&c, 1, scx->data))

Comment 8

[LegNeato]

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

a=LegNeato for 1.9.2.14

Comment 9

[LegNeato]

Comment on attachment 498868 [details] [diff] [review]
fix for 191

a=LegNeato for 1.9.1.17

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Verified for branches by verifying bug 616009.