853417 - Prevent self-hosted scripts from ever being visible to client scripts and browser code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Attachment: v1

The patch in bug 787927 prevents self-hosted scripts from showing up in the debugger and most places where stack introspection is done in the browser.

Unfortunately, it left a few holes, with most of them in the old debug API.

This patch plugs those holes. Additionally, it adds (de-)serialization of the `selfHosted` flag to js::XDRScript. I doubt that self-hosted scripts will ever be XDRed, but wouldn't entirely bet on it.

With the introduction of a `selfHosted` flag on JSScript, the equivalent flag in JSFunction should be removed. I'll do that in a follow-up bug, however, as I want this one to be uplifted to Aurora.

Comment 1

[Jim Blandy :jimb]

Comment on attachment 727652 [details] [diff] [review]
v1

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

Just two notes; one we discussed on IRC, the other possibly new.

::: js/src/jsdbgapi.cpp
@@ +73,5 @@
>  
>  static bool
>  IsTopFrameConstructing(JSContext *cx, AbstractFramePtr frame)
>  {
> +    NonBuiltinScriptFrameIter iter(cx);

I wonder if this is correct. It doesn't seem to me that we take any steps to prevent self-hosted frames from reaching cx->runtime->debugHooks->executeHook and ...->callHook. If we don't take such steps, but we use NonBuiltinScriptFrameIter here in IsTopFrameConstructing, then I would expect executeHook and callHook to be passed 'isConstructing' values that don't match the 'frame' they're passed.

::: js/src/jsscript.cpp
@@ +405,5 @@
>          IsGenerator,
>          IsGeneratorExp,
>          OwnSource,
> +        ExplicitUseStrict,
> +        SelfHosted

Whenever you change the format of XDR'd scripts, be sure to bump the value of XDR_BYTECODE_VERSION in js/src/vm/Xdr.h, according to the comments there. (Unless this is a backwards-compatible change, which it might be...)

Comment 2

[Till Schneidereit [:till]]

Attachment: v2

Thanks for the clarification on IRC!

Turns out I don't need to guard in js::ScriptDebugEpilogue: frame.maybeHookData() will always return NULL there.

Oh and no: the XDR format change isn't compatible, thanks.

Comment 3

[Alex Keybl [:akeybl]]

Without a more explicit user impact, we can't track for release. Please re-nominate if we know of an important user scenario affected by this bug.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 727978 [details] [diff] [review]
v2

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

Looks good to me!

Comment 5

[Till Schneidereit [:till]]

Thanks for the review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/495bf367cca3

Comment 6

[Jim Blandy :jimb]

(In reply to Till Schneidereit [:till] from comment #2)
> Turns out I don't need to guard in js::ScriptDebugEpilogue:
> frame.maybeHookData() will always return NULL there.

Is this because we know the ScriptDebugPrologue call didn't set any hook data, because we did guard there?

That sounds correct, but it sure would be nice to have an assertion and a comment in ScriptDebugEpilogue.

Comment 7

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/495bf367cca3

Comment 8

[Till Schneidereit [:till]]

(In reply to Jim Blandy :jimb from comment #6)
> (In reply to Till Schneidereit [:till] from comment #2)
> > Turns out I don't need to guard in js::ScriptDebugEpilogue:
> > frame.maybeHookData() will always return NULL there.
> 
> Is this because we know the ScriptDebugPrologue call didn't set any hook
> data, because we did guard there?
> 
> That sounds correct, but it sure would be nice to have an assertion and a
> comment in ScriptDebugEpilogue.

Yes, that's the reasoning. You're right, though: a comment about that seems in order. Added in
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c2c54c2b5da

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0c2c54c2b5da

Comment 10

[Till Schneidereit [:till]]

Comment on attachment 727978 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784294, bug 787927
User impact if declined: Some addons (especially GreaseMonkey) fail, plus some top-crashers caused by the attempted fix in bug 787927 (see bug 851788)
Testing completed (on m-c, etc.): yes, patch has landed 1.5 weeks ago
Risk to taking this patch (and alternatives if risky): slim to none.
String or IDL/UUID changes made by this patch: none

I dropped the ball on requesting Aurora approval before the uplift, but this should definitely go into beta, now. Sorry.

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 727978 [details] [diff] [review]
v2

Approving the low risk fix to help fix a Fx21 top crasher 851788.

Comment 12

[Till Schneidereit [:till]]

Comment on attachment 727978 [details] [diff] [review]
v2

https://hg.mozilla.org/releases/mozilla-beta/rev/dfd9d55369da