906040 - add back the trusted JS buffer zone in the interpreter

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

testChromeBuffer is supposed to be testing that, even when untrusted JS (JS running with privileges != rt->trustedPrincipals) has exhausted the stack, there is still enough left for trusted JS to run code.  (This is useful for allowing browser chrome JS to run in an error handler or slow script dialog when content has exhausted the stack.)

Unfortunately, the test I wrote in testChromeBuffer was silly and passes even if the call to trusted code throws over-recursed.  Fixing the test (wrapping the trusted() call with a try {} catch) will reveal that the test has been broken (with the righteous vm/Stack refactoring).  Fortunately, this should be a simple fix in InterpreterStack::allocateFrame.  The JITs now use JS_CHECK_RECURSION which will is getting fixed right now in bug 732665.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

I changed the test so that it fails without the changes to allocateFrame, passes with them.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 791308 [details] [diff] [review]
Patch

Review of attachment 791308 [details] [diff] [review]:
-----------------------------------------------------------------

Great!

::: js/src/jsapi-tests/testChromeBuffer.cpp
@@ +83,5 @@
> +                            "        return -1;                         "
> +                            "    try {                                  "
> +                            "        return trusted(100);               "
> +                            "    } catch(e) {                           "
> +                            "        throw 'fail';                      "

I think you can just 'return -1' here, if you'd like.

Comment 3

[Jan de Mooij [:jandem]]

D'oh, you're right. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c5f867eb56

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 791308 [details] [diff] [review]
Patch

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Bug 881902.

> User impact if declined: 
Problems with the slow script dialog not working.

> Testing completed (on m-c, etc.): 
On m-i.

> Risk to taking this patch (and alternatives if risky):
Very low. Raises the recursion limit a bit for trusted code, like we did before.

> String or IDL/UUID changes made by this patch:
None.

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a2c5f867eb56

Comment 6

[Luke Wagner [:luke]]

This patch is causing link errors for me with gcc 4.7 in debug builds:

build/unix/gold/ld: error: /moz/mi/js/src/d/Interpreter.o: requires dynamic R_X86_64_PC32 reloc against '_ZN2js16InterpreterStack18MAX_FRAMES_TRUSTEDE' which may overflow at runtime; recompile with -fPIC

I'm pretty sure the problem is this long-standing GCC quirk that it emits references to static const varaibles when they are used in a ternary operator (which you write "static const X = 1", this is a declaration and initialization, but not a *definition* which means, should a symbol ever be emitted for X, it'll be a link error, but mostly symbols never get emitted since the whole point is that they're inlined).  The usual fix is either to add a definition or take out the ternary operator.

If you can't repro, I'll make the patch, but it'd be good not to uplift yet :)

Comment 7

[Yves Gwerder]

I had this link error too and fixed it this way (based on Luke's information):
---------
diff -r a71cedddadd1 js/src/vm/Stack-inl.h
--- a/js/src/vm/Stack-inl.h	Sat Aug 17 19:50:37 2013 -0700
+++ b/js/src/vm/Stack-inl.h	Sun Aug 18 22:08:50 2013 +0200
@@ -228,9 +228,12 @@
 uint8_t *
 InterpreterStack::allocateFrame(JSContext *cx, size_t size)
 {
-    size_t maxFrames = cx->compartment()->principals == cx->runtime()->trustedPrincipals()
-                       ? MAX_FRAMES_TRUSTED
-                       : MAX_FRAMES;
+    size_t maxFrames;
+    if (cx->compartment()->principals == cx->runtime()->trustedPrincipals())
+        maxFrames = MAX_FRAMES_TRUSTED;
+    else
+        maxFrames = MAX_FRAMES;
+
     if (JS_UNLIKELY(frameCount_ >= maxFrames)) {
         js_ReportOverRecursed(cx);
         return NULL;

Comment 8

[Robert Bindar]

I get these errors too:

mozilla-central/js/src/vm/Stack-inl.h:233: error: undefined reference to 'js::InterpreterStack::MAX_FRAMES_TRUSTED'
 
mozilla-central/js/src/vm/Stack-inl.h:233: error: undefined reference to 'js::InterpreterStack::MAX_FRAMES'

collect2: error: ld returned 1 exit status

Comment 9

[Robert Bindar]

Yves's solution worked for me too.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Yves' WFM as well

Comment 11

[Jan de Mooij [:jandem]]

Pushed a followup fix to use if-else instead of a ternary:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd6afa556c9

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch + follow-up fix for aurora/beta

[Approval Request Comment]
> Bug caused by (feature/regressing bug #):
Bug 881902.

> User impact if declined: 
Problems with the slow script dialog not working.

> Testing completed (on m-c, etc.): 
On m-i.

> Risk to taking this patch (and alternatives if risky):
Very low. Raises the recursion limit a bit for trusted code, like we did before.

> String or IDL/UUID changes made by this patch:
None.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd6afa556c9

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61fbfa26390c
https://hg.mozilla.org/releases/mozilla-beta/rev/2d0439c5138f

Comment 15

[u279076]

Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.