Last Comment Bug 619255 - GC hazard in JO() from json.cpp due to reuse of root (1.9.[12]-only)
: GC hazard in JO() from json.cpp due to reuse of root (1.9.[12]-only)
Status: RESOLVED DUPLICATE of bug 616009
: [sg:dupe 616009]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: JavaScript Engine
: 1.9.2 Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Igor Bukanov
: general
:
:
:
  Show dependency treegraph
 
Reported: 2010-12-14 18:26 PST by Igor Bukanov
Modified: 2011-03-29 19:27 PDT (History)
5 users (show)
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  -
  unaffected
  .14+
  .14-fixed
  .17+
  .17-fixed


Attachments
fix (841 bytes, patch)
2010-12-14 18:50 PST, Igor Bukanov
gal: review+
clegnitto: approval1.9.2.14+
Details | Diff | Splinter Review
fix for 191 (880 bytes, patch)
2010-12-20 14:52 PST, Igor Bukanov
clegnitto: approval1.9.1.17+
Details | Diff | Splinter Review

Summon comment box

Description Igor Bukanov 2010-12-14 18:26:27 PST
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 2010-12-14 18:50:54 PST
Created attachment 497715 [details] [review]
fix

The patch copies the initial value of *vp into an extra root.
Comment 2 Andreas Gal :gal 2010-12-14 20:08:27 PST
Comment on attachment 497715 [details] [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 2010-12-16 08:59:58 PST
Nominating for 1.9.2 due to the critical status.
Comment 4 Christian Legnitto [:LegNeato] 2010-12-20 10:15:11 PST
Does this affect 1.9.1 as well?
Comment 5 Christian Legnitto [:LegNeato] 2010-12-20 10:16:04 PST
(waiting to approve until we know 1.9.1 is (un)affected.
Comment 6 Igor Bukanov 2010-12-20 14:47:14 PST
(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 2010-12-20 14:52:58 PST
Created attachment 498868 [details] [review]
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 Christian Legnitto [:LegNeato] 2010-12-27 10:16:10 PST
Comment on attachment 497715 [details] [review]
fix

a=LegNeato for 1.9.2.14
Comment 9 Christian Legnitto [:LegNeato] 2010-12-27 10:16:38 PST
Comment on attachment 498868 [details] [review]
fix for 191

a=LegNeato for 1.9.1.17
Comment 10 Robert Sayre 2011-01-05 16:10:38 PST

*** This bug has been marked as a duplicate of bug 616009 ***
Comment 11 Al Billings [:abillings] 2011-01-25 17:33:09 PST
Verified for branches by verifying bug 616009.

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