732665 - Chrome scripts should have a higher stack (recursion) limit than content scripts

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Jesse Ruderman]

Content can probe for the maximum recursion depth and then cause chrome code to run (or spin the event loop). This can make chrome scripts fail at unexpected points.

Comment 1

[Jesse Ruderman]

Luke -- billm thinks you might have already fixed this?

Comment 2

[Jesse Ruderman]

(But maybe only for the script stack, not the native stack?)

Comment 3

[Luke Wagner [:luke]]

That's right, only for the script stack.  It would make sense to do it for the native stack.  mrbkap was talking about this also.

Comment 4

[Bobby Holley (:bholley)]

Luke, is this something that's relatively easy to do? If so, are you the one to work on it?

Comment 5

[Luke Wagner [:luke]]

Probably easy; seems like you'd just change JS_CHECK_RECURSION to use a different limit when cx->runningWithTrustedPrincipals().  This doesn't involve any details of vm/Stack so anyone could do it.  More important would be making a good set of mochitests that fail before the patch and are fixed by the patch.

Comment 6

[Johnny Stenback (:jst)]

Bobby, could you hack up a fix here? Sounds easy, would knock off a few bugs with one fix!

Comment 7

[Bobby Holley (:bholley)]

We should also look at fixing bug 813646 here.

Comment 8

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=740216d845b6

Comment 9

[Bobby Holley (:bholley)]

Looks like this was good for everything bug linux-debug. Pushing again for that config:

https://tbpl.mozilla.org/?tree=Try&rev=9bbfa060ab48

Comment 10

[Bobby Holley (:bholley)]

Oh darn - I just realized that I wrote a mochitest-chrome test, not a mochitest-plain, and so I need to do mochitest-o to get those test runs (rather than mochitest-4).

Redoing this pushes from comment 8 and comment 9 together:

https://tbpl.mozilla.org/?tree=Try&rev=29ab1dfc929c

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce a mechanism for specifying different stack limits for system, trusted, and untrusted code. v1

I'm going to get started by flagging for review on the JS bits.

We kind of cop out for ion and PJS stuff. We should at least have a good story
for ion, but it probably needs to be done with someone with more jit familiarity
than I have.

The uninlined function call in JS_CHECK_RECURSION is unfortunate. We could fix
this either by separating the jsfriendapi JS_CHECK_RECURSION from the internal
one, or moving the appropriate stuff into friend fields.

Comment 12

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d1084c016d5b

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 791116 [details] [diff] [review]
Part 1 - Introduce a mechanism for specifying different stack limits for system, trusted, and untrusted code. v1

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

Nice work!

Could you add a JSAPI test for this?  There is already jsapi-tests/testChromeBuffer.cpp that I added to test the JS stack which hopefully you can extend w/o too much trouble (however, see bug 906040).

::: js/src/jit/ParallelFunctions.cpp
@@ +133,5 @@
>      // flag is set, so we still need to double check.
>  
>      uintptr_t realStackLimit;
>      if (slice->isMainThread())
> +        realStackLimit = slice->runtime()->mainThread.nativeStackLimit[StackForTrustedScript];

I think you could continue to use GetNativeStackLimit if you changed GetNativeStackLimit to take a ThreadSafeContext (instead of ExclusiveContext) which is a public base of ForkJoinSlice (which you have here).

::: js/src/jsapi.cpp
@@ +2605,3 @@
>  
> +void
> +js::RecomputeStackLimit(JSRuntime *rt, StackKind kind)

This is declared in Runtime.h, can you put it in Runtime.cpp?

@@ +2627,5 @@
>  
>      // If there's no pending interrupt request set on the runtime's main thread's
>      // ionStackLimit, then update it so that it reflects the new nativeStacklimit.
> +    //
> +    // Note that, for now, we use the untrusted limit for ion.

This comment made me worry that if trusted code accidentally entered Ion that it would get an error, but this is not the case because ionStackLimit is a conservative bound and, when the check fails in jit code, we fall back to C++ code which does a real JS_CHECK_RECURSION.  Could you extend the comment to explain this.

::: js/src/jspubtd.h
@@ +273,5 @@
> +{
> +    StackForSystemCode,
> +    StackForTrustedScript,
> +    StackForUntrustedScript,
> +    StackKindCount

Could you add short comments after each enumerator?

::: js/src/vm/Runtime.cpp
@@ +461,5 @@
>      JS_ASSERT(oldCount > 0);
>  
>  #ifdef JS_THREADSAFE
>      js::TlsPerThreadData.set(NULL);
> +    PodArrayZero(mainThread.nativeStackLimit);

I don't understand the need for this if the JSRuntime is being destroyed.

Comment 14

[Bobby Holley (:bholley)]

I already have a mochitest for this stuff, I just haven't posted it yet. Addressed all other feedback, and pushed to try to see if that weird macosx64 orange went away:

https://tbpl.mozilla.org/?tree=Try&rev=ae35e464ebdd

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #14)
> I already have a mochitest for this stuff, I just haven't posted it yet.
> Addressed all other feedback, and pushed to try to see if that weird
> macosx64 orange went away:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=ae35e464ebdd

Ok, this is green modulo some slow infinite recursion that times out with the new mac stack depth. I'm going to disable a few tests and get this stuff reviewed and landed.

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 2 - Clean up and clarify the stack sizing logic. v1

The only change in behavior here is that we give the osx check higher precedence
than the DEBUG check, meaning that we end up with 7MB stacks on osx debug rather
than 4MB stacks. But this seems strictly better given the logic involved here.

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 3 - Use separate stack depths for XPConnect JS. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 19

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=69305ea18931

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 794876 [details] [diff] [review]
Part 2 - Clean up and clarify the stack sizing logic. v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2910,5 @@
> +#define DEFAULT_STACK_QUOTA (128 * sizeof(size_t) * 1024)
> +
> +// Set stack sizes for different configurations. It's probably not great for
> +// the web to base this decision primarily on the default stack size that the
> +// underlying platform makes available, but that seems to be what we do. :-(

The column of this comment and the above one looks kinda wrong.  You make the call but just pointing it out.

@@ +2914,5 @@
> +// underlying platform makes available, but that seems to be what we do. :-(
> +
> +#if defined(XP_MACOSX) || defined(DARWIN)
> +    // MacOS has a gargantuan default stack size of 8MB. Go wild with 7MB.
> +#define STACK_QUOTA (7 * 1024 * 1024)

It seems like this could be done with plain old variables instead of #defines.

@@ +2923,4 @@
>  #elif defined(XP_WIN)
> +    // 1MB is the default stack size on Windows, so the default 1MB stack quota
> +    // we'd get on win32 is slightly too large. Use 900k instead.
> +#define STACK_QUOTA (runtime, 900 * 1024)

This seems like a bug, given the usage of STACK_QUOTA below.

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 794877 [details] [diff] [review]
Part 3 - Use separate stack depths for XPConnect JS. v1

Same comment about vars vs. #defines.

Comment 22

[Luke Wagner [:luke]]

Comment on attachment 794878 [details] [diff] [review]
Part 4 - Tests. v1

Could you add a mochitest that chews up all the stack space in content and then, with all this space chewed up, calls chrome and ensures that chrome runs successfully?

Comment 23

[Bobby Holley (:bholley)]

Updated to address all review comments, and did one full try push given that this patch is likely to be pretty orange-prone:

https://tbpl.mozilla.org/?tree=Try&rev=d5af3d56492d

Comment 24

[Bobby Holley (:bholley)]

Looks green modulo some intermittent failures and an OSX JSReftest failure for whom I apparently used the wrong skip-if syntax in the first patch.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d805bda611b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/548651733b5a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/331225b5d705
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/317317005f47
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe32b93a350

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d805bda611b
https://hg.mozilla.org/mozilla-central/rev/548651733b5a
https://hg.mozilla.org/mozilla-central/rev/331225b5d705
https://hg.mozilla.org/mozilla-central/rev/317317005f47
https://hg.mozilla.org/mozilla-central/rev/bfe32b93a350