__noSuchMethod__ allocates beyond fp->script->depth

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

(5 keywords)

Trunk
assertion, crash, fixed1.8.0.12, fixed1.8.1.4, testcase
Points:
---
Bug Flags:
blocking1.8.1.4 +
blocking1.8.0.12 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(5 attachments)

(Assignee)

Description

11 years ago
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?
(Assignee)

Updated

11 years ago
Summary: nuSuchMethod + getter => assertion → noSuchMethod + getter => assertion

Updated

11 years ago
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.

Comment 2

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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

11 years ago
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: crash → assertion
(Assignee)

Comment 8

11 years ago
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
(Assignee)

Comment 9

11 years ago
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.
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+

Updated

11 years ago
Blocks: 355044
(Assignee)

Comment 11

11 years ago
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
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

11 years ago
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+
(Assignee)

Comment 13

11 years ago
Created attachment 260856 [details] [diff] [review]
Fix v1 for 1.8 brunch
(Assignee)

Updated

11 years ago
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
(Assignee)

Updated

11 years ago
Attachment #260856 - Attachment description: Fix v1 for 1.8 branch → Fix v1 for 1.8 brunch
(Assignee)

Updated

11 years ago
Attachment #260856 - Flags: review?(brendan)
Attachment #260856 - Flags: approval1.8.1.4?
(Assignee)

Comment 14

11 years ago
Created attachment 260857 [details]
diff between trunk and 1.8 versions of the patch
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

Updated

11 years ago
Attachment #260856 - Flags: review?(brendan) → review+
(Assignee)

Comment 16

11 years ago
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
(Assignee)

Comment 17

11 years ago
(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:"
(Assignee)

Comment 18

11 years ago
(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.  
(Assignee)

Comment 19

11 years ago
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
(Assignee)

Comment 20

11 years ago
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.
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+
(Assignee)

Comment 23

11 years ago
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
(Assignee)

Comment 24

11 years ago
 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

Comment 25

11 years ago
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?

Updated

11 years ago
Flags: in-testsuite+
Group: security

Comment 26

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

Comment 27

10 years ago
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.