Last Comment Bug 367119 - Potential memory corruption in script_exec
: Potential memory corruption in script_exec
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:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-16 06:11 PST by moz_bug_r_a4
Modified: 2007-05-06 01:40 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 (477 bytes, text/html)
2007-01-16 06:11 PST, moz_bug_r_a4
no flags Details
reordering again (2.83 KB, patch)
2007-01-26 22:00 PST, Brian Crowder
no flags Details | Diff | Splinter Review
same reordering concept (2.01 KB, patch)
2007-01-28 09:52 PST, Brian Crowder
brendan: review+
dveditz: approval1.8.1.2+
dveditz: approval1.8.0.10+
Details | Diff | Splinter Review
Moved instanceof check up (1.93 KB, patch)
2007-01-28 20:06 PST, Brian Crowder
crowderbt: review+
dveditz: approval1.8.1.2+
Details | Diff | Splinter Review
Moz 1.8.0 branch port (1.87 KB, patch)
2007-01-28 21:46 PST, Brian Crowder
crowderbt: review+
dveditz: approval1.8.0.10+
Details | Diff | Splinter Review
js1_5/extensions/regress-367119-01.js (2.58 KB, text/plain)
2007-02-06 20:25 PST, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-367119-02.js (2.54 KB, application/javascript)
2007-02-06 20:26 PST, Bob Clary [:bc:]
no flags Details

Description moz_bug_r_a4 2007-01-16 06:11:25 PST
Created attachment 251633 [details]
testcase

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.129&mark=271,277,340#262

In script_exec, we retrieve |script| before calling js_ValueToObject(cx,
argv[0], &scopeobj).  During the call to js_ValueToObject, |script| can be
freed by calling script.compile().

  var s = new Script("");
  var o = {
    valueOf : function() {
      s.compile("");
      return {};
    }
  };
  s.exec(o);

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


Breakpoint 2, script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:277
277             if (!js_ValueToObject(cx, argv[0], &scopeobj))
(gdb) p script
$1 = (JSScript *) 0x8163ae8
(gdb) p/x *script
$2 = {code = 0x8163b18, length = 0x1, main = 0x8163b18, version = 0x0,
  numGlobalVars = 0x0, atomMap = {vector = 0x0, length = 0x0},
  filename = 0x81624c1, lineno = 0x1, depth = 0x0, trynotes = 0x0,
  principals = 0x0, object = 0x815b1c0}
(gdb) c
Continuing.

Breakpoint 3, script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:279
279             argv[0] = OBJECT_TO_JSVAL(scopeobj);
(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 SIGTRAP, Trace/breakpoint trap.
JS_Assert (
    s=0x812310c "version == JSVERSION_DEFAULT || version >= JSVERSION_ECMA_3",
    file=0x8123100 "jscntxt.c", ln=180) at jsutil.c:63
63          abort();
(gdb) bt
#0  JS_Assert (
    s=0x812310c "version == JSVERSION_DEFAULT || version >= JSVERSION_ECMA_3",
    file=0x8123100 "jscntxt.c", ln=180) at jsutil.c:63
#1  0x08068bee in js_OnVersionChange (cx=0x81556a8) at jscntxt.c:180
#2  0x08068c0d in js_SetVersion (cx=0x81556a8, version=97) at jscntxt.c:188
#3  0x0809a809 in js_Interpret (cx=0x81556a8,
    pc=0x610061 <Address 0x610061 out of bounds>, result=0xbfbcdca4)
    at jsinterp.c:2272
#4  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815b260, script=0x8163ae8,
    down=0xbfbce344, flags=32, result=0xbfbcddb0) at jsinterp.c:1607
#5  0x080f78f0 in script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:340
#6  0x080990e5 in js_Invoke (cx=0x81556a8, argc=1, flags=0) at jsinterp.c:1348
#7  0x080a93a4 in js_Interpret (cx=0x81556a8, pc=0x8164854 ":",
    result=0xbfbce364) at jsinterp.c:4070
#8  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815a020, script=0x81647f0,
    down=0x0, flags=0, result=0xbfbcf434) at jsinterp.c:1607
#9  0x0805fe17 in JS_ExecuteScript (cx=0x81556a8, obj=0x815a020,
    script=0x81647f0, rval=0xbfbcf434) at jsapi.c:4212
#10 0x08049760 in Process (cx=0x81556a8, obj=0x815a020, filename=0x0,
    forceTTY=0) at js.c:268
#11 0x08049ecb in ProcessArgs (cx=0x81556a8, obj=0x815a020, argv=0xbfbcf5c8,
    argc=0) at js.c:489
---Type <return> to continue, or q <return> to quit---
#12 0x0804e5b7 in main (argc=0, argv=0xbfbcf5c8, envp=0xbfbcf5cc) at js.c:3167
(gdb)
Comment 1 Daniel Veditz [:dveditz] 2007-01-23 15:30:31 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-23 16:06:41 PST
Is this fixed by the removal of the script object?  Brendan have you looked at that patch?  That's in bug 355820, btw.
Comment 3 Brendan Eich [:brendan] 2007-01-23 17:16:14 PST
Yes, removing (unifdef'ing for JS_HAS_SCRIPT_OBJECT == 0, or just #define'ing that jsconfig.h macro to 0) will fix this and other bugs. But that's an API change, and we try to avoid those on the stable branches.

I'll go look at that bug now.

/be
Comment 4 Brian Crowder 2007-01-26 12:16:03 PST
For branches, I'm wondering if a reasonable strategy for this would be to put a flag on the script object when it is executing, such that it cannot be compiled over, and throws an exception instead?
Comment 5 Brian Crowder 2007-01-26 22:00:58 PST
Created attachment 253000 [details] [diff] [review]
reordering again

Same strategy as fixes in bug 367120
Comment 6 Brian Crowder 2007-01-26 22:11:10 PST
The patch from comment 5 also applies cleanly on both branches.
Comment 7 Brian Crowder 2007-01-26 23:06:51 PST
heh.  I THOUGHT I had already put this patch on a bug somewhere.  Turns out I had.  Pretty sure these are the same bug.

*** This bug has been marked as a duplicate of bug 367118 ***
Comment 8 moz_bug_r_a4 2007-01-27 23:30:10 PST
The patch in bug 367118 does not fix this.  script_exec needs to be fixed, too.
Comment 9 Brian Crowder 2007-01-28 09:52:33 PST
Created attachment 253102 [details] [diff] [review]
same reordering concept

Weird, in my testing with the compile fix, this didn't crash.  Can you please try this patch out, moz_bug?  Same reordering concept as the other two.  Asking for branch approval now so I can land all three of these together, pending r+ from brendan.
Comment 10 Daniel Veditz [:dveditz] 2007-01-28 14:23:26 PST
Comment on attachment 253102 [details] [diff] [review]
same reordering concept

a=dveditz for 1.8/1.8.0 branches pending brendan's r+
Comment 11 Brendan Eich [:brendan] 2007-01-28 15:24:26 PST
Comment on attachment 253102 [details] [diff] [review]
same reordering concept

> static JSBool
> script_exec(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>-    JSScript *script;
>     JSObject *scopeobj, *parent;
>     JSStackFrame *fp, *caller;
>     JSPrincipals *principals;
>-
>-    if (!JS_InstanceOf(cx, obj, &js_ScriptClass, argv))
>-        return JS_FALSE;

The instanceof check should always come first. r=me with that, post a patch-to-commit and no need to re-solicit review. Thanks,

/be
Comment 12 moz_bug_r_a4 2007-01-28 16:02:06 PST
(In reply to comment #9)
> Weird, in my testing with the compile fix, this didn't crash.  Can you please
> try this patch out, moz_bug?

I've tested with jsshell. script_exec patch fixes this. (script_compile patch
does not.)
Comment 13 Brian Crowder 2007-01-28 20:06:15 PST
Created attachment 253125 [details] [diff] [review]
Moved instanceof check up

I'm going to hope my branch-approval is grandfathered in, regardless of this tweak, and just land this tonight with the other two.  dveditz or whoever, please re-stamp when it's convenient, or let me know if it's not for some reason and I will back out.
Comment 14 Brian Crowder 2007-01-28 20:19:57 PST
Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.131; previous revision: 3.130
done

(leaving open until I land on branches.. soon)
Comment 15 Brian Crowder 2007-01-28 21:09:25 PST
Moz1.8:

Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.20; previous revision: 3.79.2.19
done
Comment 16 Brian Crowder 2007-01-28 21:46:09 PST
Created attachment 253134 [details] [diff] [review]
Moz 1.8.0 branch port

This one I think is okay to land, the diffs are pretty minor:

<      if (!js_CheckPrincipalsAccess(cx, scopeobj, principals,
<                                    CLASS_ATOM(cx, Script))) {
---
>      if (!js_CheckPrincipalsAccess(cx, scopeobj, principals, js_script_exec))
72d63
<      }
Comment 17 Brian Crowder 2007-01-28 21:51:33 PST
Moz 1.8.0:

Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.5.2.3; previous revision: 3.79.2.5.2.2
done
Comment 18 Daniel Veditz [:dveditz] 2007-01-28 23:23:47 PST
Comment on attachment 253125 [details] [diff] [review]
Moved instanceof check up

a=dveditz for 1.8/1.8.0 branches
Comment 19 Bob Clary [:bc:] 2007-02-06 20:25:45 PST
Created attachment 254260 [details]
js1_5/extensions/regress-367119-01.js
Comment 20 Bob Clary [:bc:] 2007-02-06 20:26:20 PST
Created attachment 254261 [details]
js1_5/extensions/regress-367119-02.js
Comment 21 juan becerra [:juanb] 2007-02-08 16:15:40 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.
Comment 22 Bob Clary [:bc:] 2007-02-19 13:35:06 PST
verified fixed 2007-02-17 1.9.0 windows/mac*/linux
Comment 23 Bob Clary [:bc:] 2007-05-06 01:40:03 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367119-01.js,v  <--  regress-367119-01.js
initial revision: 1.1

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

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