Last Comment Bug 367120 - Potential memory corruption in script_toSource and script_toString
: Potential memory corruption in script_toSource and script_toString
Status: VERIFIED FIXED
[sg:critical]
: verified1.8.0.10, verified1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Brian Crowder
:
Mentors:
Depends on: 368534
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-16 06:18 PST by moz_bug_r_a4
Modified: 2007-05-06 01:42 PDT (History)
6 users (show)
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (529 bytes, text/html)
2007-01-16 06:18 PST, moz_bug_r_a4
no flags Details
Moving the value conversions earlier. (2.35 KB, patch)
2007-01-26 15:56 PST, Brian Crowder
brendan: review+
Details | Diff | Review
moved index decls to match use-order as suggested (2.71 KB, patch)
2007-01-26 20:34 PST, Brian Crowder
crowderbt: review+
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Review
js1_5/extensions/regress-367120-01.js (2.61 KB, text/plain)
2007-02-06 20:27 PST, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-367120-02.js (2.58 KB, text/plain)
2007-02-06 20:28 PST, Bob Clary [:bc:]
no flags Details

Description moz_bug_r_a4 2007-01-16 06:18:49 PST
Created attachment 251634 [details]
testcase

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.129&mark=84,95-98#72
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.129&mark=142,149-152#133

In script_toSource and script_toString, we retrieve |script| before calling
js_ValueToECMAUint32(cx, argv[0], &indent)).  During the call to
js_ValueToECMAUint32, |script| can be freed by calling script.compile().

  var s = new Script("");
  var o = {
    valueOf : function() {
      s.compile("");
    }
  };
  s.toSource(o);
or
  s.toString(o);

---
js> var s = new Script(""), o = {
  valueOf : function() {
    s.compile("");
    Array(11).join(Array(11).join(Array(101).join("aaaaa")));
    return;
  }
}; s.toString(o); void 0;

Breakpoint 2, script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:149
149         if (argc && !js_ValueToECMAUint32(cx, argv[0], &indent))
(gdb) p script
$1 = (JSScript *) 0x8163af0
(gdb) p/x *script
$2 = {code = 0x8163b20, length = 0x1, main = 0x8163b20, version = 0x0,
  numGlobalVars = 0x0, atomMap = {vector = 0x0, length = 0x0},
  filename = 0x8162481, lineno = 0x1, depth = 0x0, trynotes = 0x0,
  principals = 0x0, object = 0x815b1c0}
(gdb) c
Continuing.

Breakpoint 3, script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:151
151         str = JS_DecompileScript(cx, script, "Script.prototype.toString",
(gdb) p/x *script
$3 = {code = 0x610061, length = 0x610061, main = 0x610061, version = 0x61,
  numGlobalVars = 0x61, atomMap = {vector = 0x610061, length = 0x610061},
  filename = 0x610061, lineno = 0x610061, depth = 0x610061,
  trynotes = 0x610061, principals = 0x610061, object = 0x610061}
(gdb) c
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x080c85ec in Decompile (ss=0xbf91ec58,
    pc=0x610061 <Address 0x610061 out of bounds>, nb=6357089)
    at jsopcode.c:1614
1614            op = (JSOp) *pc;
(gdb) bt
#0  0x080c85ec in Decompile (ss=0xbf91ec58,
    pc=0x610061 <Address 0x610061 out of bounds>, nb=6357089)
    at jsopcode.c:1614
#1  0x080d2513 in js_DecompileCode (jp=0x81632e0, script=0x8163af0,
    pc=0x610061 <Address 0x610061 out of bounds>, len=6357089, pcdepth=0)
    at jsopcode.c:4192
#2  0x080d26a1 in js_DecompileScript (jp=0x81632e0, script=0x8163af0)
    at jsopcode.c:4212
#3  0x0805fc74 in JS_DecompileScript (cx=0x81556a8, script=0x8163af0,
    name=0x81304c2 "Script.prototype.toString", indent=0) at jsapi.c:4158
#4  0x080f7293 in script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:151
#5  0x080990e5 in js_Invoke (cx=0x81556a8, argc=1, flags=0) at jsinterp.c:1348
#6  0x080a93a4 in js_Interpret (cx=0x81556a8, pc=0x816485c ":",
    result=0xbf91f354) at jsinterp.c:4070
#7  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815a020, script=0x81647f8,
    down=0x0, flags=0, result=0xbf920424) at jsinterp.c:1607
#8  0x0805fe17 in JS_ExecuteScript (cx=0x81556a8, obj=0x815a020,
    script=0x81647f8, rval=0xbf920424) at jsapi.c:4212
#9  0x08049760 in Process (cx=0x81556a8, obj=0x815a020, filename=0x0,
    forceTTY=0) at js.c:268
#10 0x08049ecb in ProcessArgs (cx=0x81556a8, obj=0x815a020, argv=0xbf9205b8,
    argc=0) at js.c:489
---Type <return> to continue, or q <return> to quit---
#11 0x0804e5b7 in main (argc=0, argv=0xbf9205b8, envp=0xbf9205bc) at js.c:3167
(gdb)
Comment 1 Daniel Veditz [:dveditz] 2007-01-23 15:31:36 PST
Critical security bugs must have owners. If you can't work on this bug help us find another active owner for it.
Comment 2 Brian Crowder 2007-01-26 15:56:02 PST
Created attachment 252964 [details] [diff] [review]
Moving the value conversions earlier.

Moving the value conversions earlier.
Comment 3 Brian Crowder 2007-01-26 15:57:17 PST
Sorry for spam; I meant to add:  This is a patch against the trunk.  To test it, since the Script removal is checked in, you'll need to manually turn it back on in jsconfig.h.  I will port to branches if moz_bug doesn't defeat this.
Comment 4 Brendan Eich [:brendan] 2007-01-26 17:33:18 PST
Comment on attachment 252964 [details] [diff] [review]
Moving the value conversions earlier.

>Index: jsscript.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsscript.c,v
>retrieving revision 3.130
>diff -u -p -8 -r3.130 jsscript.c
>--- jsscript.c	25 Jan 2007 23:08:52 -0000	3.130
>+++ jsscript.c	26 Jan 2007 23:53:20 -0000
>@@ -74,31 +74,32 @@ script_toSource(JSContext *cx, JSObject 
> {
>     JSScript *script;
>     size_t i, j, k, n;
>     char buf[16];
>     jschar *s, *t;
>     uint32 indent;
>     JSString *str;
> 
>+    indent = 0;
>+    if (argc && !js_ValueToECMAUint32(cx, argv[0], &indent))
>+        return JS_FALSE;

Nit: move indent's decl up to first place to match use order, same for toString.

Great spot-fix, thanks.

/be
Comment 5 Brian Crowder 2007-01-26 20:34:00 PST
Created attachment 252995 [details] [diff] [review]
moved index decls to match use-order as suggested

This is the patch I am landing on the main trunk.  Branch patches coming.
Comment 6 Brian Crowder 2007-01-26 21:17:45 PST
Went to go check-in; but this is one of my first interesting security-bugs, so I stopped myself.  Seems like it might be a good idea for this one to have some sort of cover-bug?
Comment 7 Brian Crowder 2007-01-26 21:44:32 PST
Patch applies cleanly on both branches as-is.  Please advice with regard to landing strategy.
Comment 8 Daniel Veditz [:dveditz] 2007-01-27 18:13:32 PST
Comment on attachment 252995 [details] [diff] [review]
moved index decls to match use-order as suggested

a=dveditz for 1.8/1.8.0 branches.

You could use a public cover bug, but what would it say? You could also check in under the attachment number rather than bug number (without saying it's an attachment number -- we'll know). The checkin comment should be vague in any case, like "bail earlier if we can't get the value"
Comment 9 Brian Crowder 2007-01-28 20:55:39 PST
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.133; previous revision: 3.132
done
Comment 10 Brian Crowder 2007-01-28 21:18:59 PST
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.21; previous revision: 3.79.2.20
done
Comment 11 Brian Crowder 2007-01-28 21:56:47 PST
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.5.2.4; previous revision: 3.79.2.5.2.3
done
Comment 12 Bob Clary [:bc:] 2007-02-06 20:27:31 PST
Created attachment 254262 [details]
js1_5/extensions/regress-367120-01.js
Comment 13 Bob Clary [:bc:] 2007-02-06 20:28:09 PST
Created attachment 254263 [details]
js1_5/extensions/regress-367120-02.js
Comment 14 juan becerra [:juanb] 2007-02-08 16:17:26 PST
Verified using testcase in comment #0 on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.10pre) Gecko/20070206 Firefox/1.5.0.10pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070206 BonEcho/2.0.0.2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.10pre) Gecko/20070208 Firefox/1.5.0.10pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070207 BonEcho/2.0.0.2pre

Also on latest linux trunk, and latest Mac 2002pre nightly.
Comment 15 Brian Crowder 2007-02-09 13:15:01 PST
Another one Taras might find interesting.  Similar bad pattern with pointer-usage.
Comment 16 Bob Clary [:bc:] 2007-02-19 13:33:52 PST
verified fixed 2007-02-17 1.9.0 windows/mac*/linux
Comment 17 Bob Clary [:bc:] 2007-05-06 01:42:07 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367120-01.js,v  <--  regress-367120-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367120-02.js,v  <--  regress-367120-02.js
initial revision: 1.1

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