375183 - __noSuchMethod__ allocates beyond fp->script->depth

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

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]]

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]

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]

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]

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]

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]

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]

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]

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]

Attachment: 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]]

Comment on attachment 260635 [details] [diff] [review]
Fix v1

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

/be

Comment 11

[Igor Bukanov]

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]

Nominating to 1.8.0 since AFAICS the GC hazard exists there as well.

Comment 13

[Igor Bukanov]

Attachment: Fix v1 for 1.8 brunch

Comment 14

[Igor Bukanov]

Attachment: diff between trunk and 1.8 versions of the patch

Comment 15

[Brendan Eich [:brendan]]

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]

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]

(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]

(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]

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]

Attachment: 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]]

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]]

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]

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]

 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] (inactive)]

Attachment: 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] (inactive)]

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

Comment 27

[Bob Clary [:bc] (inactive)]

no failures in 1.8.1, 1.9.0 in 2008-02 to date. going ahead an verifying.