Last Comment Bug 375183 - __noSuchMethod__ allocates beyond fp->script->depth
: __noSuchMethod__ allocates beyond fp->script->depth
Status: VERIFIED FIXED
[sg:critical?] maybe GC problem
: assertion, crash, fixed1.8.0.12, fixed1.8.1.4, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2007-03-24 04:27 PDT by Igor Bukanov
Modified: 2008-02-26 09:04 PST (History)
6 users (show)
dveditz: blocking1.8.1.4+
dveditz: blocking1.8.0.12+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (6.44 KB, patch)
2007-04-04 14:38 PDT, Igor Bukanov
brendan: review+
Details | Diff | Splinter Review
Fix v1 for 1.8 brunch (6.53 KB, patch)
2007-04-06 14:37 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.1.4+
Details | Diff | Splinter Review
diff between trunk and 1.8 versions of the patch (1.26 KB, text/plain)
2007-04-06 14:41 PDT, Igor Bukanov
no flags Details
Fix v1 for 1.8.0 brunch (5.29 KB, patch)
2007-04-06 16:11 PDT, Igor Bukanov
brendan: review+
dveditz: approval1.8.0.12+
Details | Diff | Splinter Review
js1_5/extensions/regress-375183.js (2.71 KB, text/plain)
2007-05-02 11:43 PDT, Bob Clary [:bc:]
no flags Details

Description Igor Bukanov 2007-03-24 04:27:39 PDT
The following test case for the jsshell demonstrates the problem:

~/m/trunk/mozilla/js/src $ cat ~/s/y.js
var obj = { get __noSuchMethod__() {
              print("Executed");
              return new Object();
          }};

obj.x();
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
Executed
Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff, at jsinterp.c:347
trace/breakpoint trap


The bug exists in 1.8.1 as well, but not in 1.8.0.
Comment 1 Daniel Veditz [:dveditz] 2007-03-29 10:38:03 PDT
Is __noSuchMethod__() reachable from web content (doesn't seem to be)? if not this is perhaps more a "bug" than a security concern.
Comment 2 Jesse Ruderman 2007-03-30 05:51:49 PDT
I think web content can define __noSuchMethod__.  The example here makes a debug build of Firefox assert (fatally) when I paste it into http://www.squarefree.com/shell/shell.html.
Comment 3 William J. Edney 2007-03-30 09:40:22 PDT
Jesse -

Thanks for adding me to this bug. We *absolutely* use this mechanism from 'web content'. It is a core part of being able to build a polymorphic system.

We don't use Spidermonkey getters and setters though, which is why you probably didn't see me screaming earlier.

This isn't a show-stopper for us, but anytime points out problems with this hook, I get really nervous.

Maybe you should get Brendan involved? He wrote the original code here. He's probably stunningly busy these days, though ;-).

Thanks again,

- Bill

P.S. I'm adding Scott Shattuck, architect and lead developer of TIBET, to this bug.
Comment 4 Jesse Ruderman 2007-03-30 10:23:26 PDT
Oops, I thought you had written the patch in bug 196097.  Now I see that you weren't the author of the patch, I can understand you not wanting to dive in and fix this potential security hole ;)
Comment 5 Scott Shattuck 2007-03-30 10:38:43 PDT
A general comment on "security holes" such as this is that we seem to often run up against the fact that there are, in some sense, two webs: the public web (aka the open internet) where concerns about security are clearly important; and the business web where companies are building mission critical applications that just happen to use Moz/FF as a virtual machine. I have no issue with patching security holes, but I'd like to make sure that when patches are made we don't cripple the browser as an application platform in trusted enterprise environments.
Comment 6 Jesse Ruderman 2007-03-30 19:20:34 PDT
If you're doing clever things with __noSuchMethod__ beyond what you trust http://lxr.mozilla.org/mozilla/source/js/tests/js1_5/extensions/no-such-method.js to test, please file a bug for adding more tests and upload your tests to it.  Maybe you'd like to turn bug 196097 comment 54 into an automated regression test, for example?  Then someone will notice quickly if this patch (or any other patch) breaks it in a way that's likely to affect your application.
Comment 7 Jesse Ruderman 2007-03-30 19:25:31 PDT
You don't need a getter to trigger this bug.  Simply having __noSuchMethod__ be a non-function object is sufficient to trigger it.

  var obj = { __noSuchMethod__: {} };
  obj.x();

Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff, at jsinterp.c:347
Comment 8 Igor Bukanov 2007-04-03 13:06:16 PDT
The real problem here is that __noSuchMethod__ implementation forces the stack to grow beyond fp->script->depth when it pushes 2 parameters to pass to the __noSuchMethod__ implementation. This is visible with much simpler test case: 

~/m/trunk/mozilla/js/src $ cat ~/s/y.js
var obj = { }
obj.__noSuchMethod__ = {};
obj.x();
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js 
Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff, at jsinterp.c:347
trace/breakpoint trap
~/m/trunk/mozilla/js/src $

AFAICS such overallocation by noSushMethod is wrong given that the code in other places (like frame tracing during GC) assumes that fp->script->depth is a hard limit. But on the other hand given the explicit coding for that overallocation in NoSuchMethod from jsinterp.c I am not sure that this 100% wrong.

In any case, I suggest to change noSushMethod to simply call the method instead of delegating that to the caller.
Comment 9 Igor Bukanov 2007-04-04 14:38:28 PDT
Created attachment 260635 [details] [diff] [review]
Fix v1

The patch always uses js_InternalInvoke to implement the call to noSuchMethod. 

In addition the patch fixes GC hazard in the current code in NoSuchMethod from jsinterp.c There the code loads a value of __noSuchMethod__ property into unrooted  variable and call js_NewArrayObject later. Since for a sufficiently long argument list the last function will call the branch callback and GC, a script should be able via unclosed generator to unroot the value.
Comment 10 Brendan Eich [:brendan] 2007-04-04 14:53:11 PDT
Comment on attachment 260635 [details] [diff] [review]
Fix v1

Looks good, thanks. TIBET guys should test it hard.

/be
Comment 11 Igor Bukanov 2007-04-04 15:19:49 PDT
I committed the patch from comment 9 to the trunk:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.343; previous revision: 3.342
done
Comment 12 Igor Bukanov 2007-04-04 15:21:24 PDT
Nominating to 1.8.0 since AFAICS the GC hazard exists there as well.
Comment 13 Igor Bukanov 2007-04-06 14:37:23 PDT
Created attachment 260856 [details] [diff] [review]
Fix v1 for 1.8 brunch
Comment 14 Igor Bukanov 2007-04-06 14:41:42 PDT
Created attachment 260857 [details]
diff between trunk and 1.8 versions of the patch
Comment 15 Brendan Eich [:brendan] 2007-04-06 14:49:26 PDT
Comment on attachment 260857 [details]
diff between trunk and 1.8 versions of the patch

Argh, text/x-patch strikes again.

/be
Comment 16 Igor Bukanov 2007-04-06 15:09:48 PDT
Here is a test case to expose GC hazard with noSuchMethod implementation that existed on trunk/1.8 and that continue to exist on 1.8.1 brunch:

~/m/1.8.0/mozilla/js/src $ cat ~/s/y.js
var obj = { }
obj.__noSuchMethod__ = {};

Array.prototype.__defineSetter__(0, function() {
  obj.__noSuchMethod__ = {};
  gc();
});

obj.x(1);
~/m/1.8.0/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js 
before 2826, after 2799, break 087f5000
segmentation fault
Comment 17 Igor Bukanov 2007-04-06 15:11:37 PDT
(In reply to comment #16)
> Here is a test case to expose GC hazard with noSuchMethod implementation that
> existed on trunk/1.8 and that continue to exist on 1.8.1 brunch:
           ^^^^^^^^^^^^                             ^^^^^^^^^^^^^

I meant "existed on trunk and that continue to exist on 1.8.1/1.8.0 brunches:"
Comment 18 Igor Bukanov 2007-04-06 15:15:13 PDT
(In reply to comment #15)
> (From update of attachment 260857 [details])
> Argh, text/x-patch strikes again.

I wish bugzilla would not preselect the auto-detection of attachment's type and rather continue to complain about the wrong attachment type.  
Comment 19 Igor Bukanov 2007-04-06 15:16:07 PDT
Comment on attachment 260857 [details]
diff between trunk and 1.8 versions of the patch

This is not a patch in fact but rather a diff between patches.
Comment 20 Igor Bukanov 2007-04-06 16:11:53 PDT
Created attachment 260862 [details] [diff] [review]
Fix v1 for 1.8.0 brunch

This is a backport of the idea to call js_InternalInvoke and root all the temporaries in between.
Comment 21 Brendan Eich [:brendan] 2007-04-06 16:26:02 PDT
Comment on attachment 260862 [details] [diff] [review]
Fix v1 for 1.8.0 brunch

Looks ok -- I noodled on how to eliminate reportNotAFunction but you probably did too (alternative is another goto target with duplicated pop-temp-value-rooter call).

/be
Comment 22 Daniel Veditz [:dveditz] 2007-04-09 17:17:40 PDT
Comment on attachment 260862 [details] [diff] [review]
Fix v1 for 1.8.0 brunch

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Comment 23 Igor Bukanov 2007-04-09 23:13:27 PDT
I committed the patch from comment 13 to MOZILLA_1_8_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.87; previous revision: 3.181.2.86
done
Comment 24 Igor Bukanov 2007-04-09 23:19:16 PDT
 committed the patch from comment 20 to MOZILLA_1_8_0_BRANCH:

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.17.2.32; previous revision: 3.181.2.17.2.31
done
Comment 25 Bob Clary [:bc:] 2007-05-02 11:43:39 PDT
Created attachment 263494 [details]
js1_5/extensions/regress-375183.js

Note, I can not reproduce this bug with this testcase. Will someone who can reproduce the bug with an appropriate pull, verify this for trunk, and the branches?
Comment 26 Bob Clary [:bc:] 2007-06-14 15:34:29 PDT
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375183.js,v  <--  regress-375183.js
initial revision: 1.1
Comment 27 Bob Clary [:bc:] 2008-02-26 09:04:35 PST
no failures in 1.8.1, 1.9.0 in 2008-02 to date. going ahead an verifying.

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