Closed Bug 375183 Opened 17 years ago Closed 17 years ago

__noSuchMethod__ allocates beyond fp->script->depth

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(5 keywords, Whiteboard: [sg:critical?] maybe GC problem)

Attachments

(5 files)

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.
Flags: blocking1.8.1.4?
Summary: nuSuchMethod + getter => assertion → noSuchMethod + getter => assertion
Keywords: crash, testcase
Is __noSuchMethod__() reachable from web content (doesn't seem to be)? if not this is perhaps more a "bug" than a security concern.
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.
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.
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 ;)
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.
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.
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
Summary: noSuchMethod + getter => assertion → non-function as __noSuchMethod__ => "Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff"
Flags: blocking1.8.1.4? → blocking1.8.1.4+
Keywords: crashassertion
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.
Summary: non-function as __noSuchMethod__ => "Assertion failure: JS_UPTRDIFF(fp->sp, fp->spbase) <= depthdiff" → __noSuchMethod__ allocates beyond fp->script->depth
Attached patch Fix v1Splinter Review
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.
Attachment #260635 - Flags: review?(brendan)
Comment on attachment 260635 [details] [diff] [review]
Fix v1

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

/be
Attachment #260635 - Flags: review?(brendan) → review+
Blocks: js1.7src
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
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Nominating to 1.8.0 since AFAICS the GC hazard exists there as well.
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12? → blocking1.8.0.12+
Attachment #260856 - Attachment description: 1.8 version → Fix v1 for 1.8 branch
Attachment #260856 - Attachment is patch: true
Attachment #260856 - Attachment mime type: text/x-patch → text/plain
Attachment #260856 - Attachment description: Fix v1 for 1.8 branch → Fix v1 for 1.8 brunch
Attachment #260856 - Flags: review?(brendan)
Attachment #260856 - Flags: approval1.8.1.4?
Comment on attachment 260857 [details]
diff between trunk and 1.8 versions of the patch

Argh, text/x-patch strikes again.

/be
Attachment #260857 - Attachment is patch: true
Attachment #260857 - Attachment mime type: text/x-patch → text/plain
Attachment #260856 - Flags: review?(brendan) → review+
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
Keywords: crash
(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:"
(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 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.
Attachment #260857 - Attachment is patch: false
This is a backport of the idea to call js_InternalInvoke and root all the temporaries in between.
Attachment #260862 - Flags: review?(brendan)
Attachment #260862 - Flags: approval1.8.0.12?
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
Attachment #260862 - Flags: review?(brendan) → review+
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
Attachment #260862 - Flags: approval1.8.0.12? → approval1.8.0.12+
Attachment #260856 - Flags: approval1.8.1.4? → approval1.8.1.4+
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
Keywords: fixed1.8.1.4
 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
Keywords: fixed1.8.0.12
Whiteboard: [sg:critical?] maybe GC problem
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?
Flags: in-testsuite+
Group: security
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-375183.js,v  <--  regress-375183.js
initial revision: 1.1
no failures in 1.8.1, 1.9.0 in 2008-02 to date. going ahead an verifying.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.