ComputeImplicitThis leaks VarEnvironmentObjects to script

RESOLVED FIXED in Firefox 57

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

3 months ago
ComputeImplicitThis does not handle VarEnvironmentObjects (and ModuleEnvironmentObjects) correctly.

For example, |(function (a = eval("function g() { return this; }; with({}) { g() }")) { return a })()| will return the environment that the function default parameter is expanded in.

Since the holder of |g| is the VarEnvironmentObject, it gets passed to ComputeImplicitThis which returns the env instead of undefined.
(Assignee)

Updated

3 months ago
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
(Assignee)

Updated

3 months ago
Blocks: 1385429
Comment hidden (mozreview-request)

Updated

3 months ago
Duplicate of this bug: 1385429

Comment 3

3 months ago
Could we include the test case in bug 1385429 comment 5 in this patch?

Comment 4

3 months ago
mozreview-review
Comment on attachment 8905231 [details]
Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments

https://reviewboard.mozilla.org/r/177020/#review182266

Good find! This is much nicer.

::: js/src/tests/ecma_6/Function/implicit-this-in-parameter-expression.js:15
(Diff revision 1)
> +    }
> +    `)) {
> +    return a
> +};
> +
> +assertEq(f(), undefined);

Unlike jit-tests, jstests need to report success:

if (typeof reportCompare === "function")
    reportCompare(true, true);
Attachment #8905231 - Flags: review?(jdemooij) → review+
(In reply to Jim Blandy :jimb from comment #3)
> Could we include the test case in bug 1385429 comment 5 in this patch?

Agreed a test in jit-test/tests/debug/ would be nice. Should probably remove the non-standard uneval and add a correctness check.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

3 months ago
Fixed test cases as requested. Also did same whitelist for GetThisValue as discussed with :jandem on IRC.
Comment hidden (mozreview-request)

Comment 11

3 months ago
mozreview-review
Comment on attachment 8905231 [details]
Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments

https://reviewboard.mozilla.org/r/177020/#review182404

Clearing review as discussed on IRC.
Attachment #8905231 - Flags: review+

Comment 12

3 months ago
The rewrite of ComputeImplicitThis makes the problem unlikely, but it might be good to test calling a function found in a module, so that we exercise the case where env is an EnvironmentModuleObject.

Comment 13

3 months ago
Created attachment 8905684 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS.

Would this comment be good?
Attachment #8905684 - Flags: review?(jdemooij)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8905768 - Flags: review?(jdemooij)
Attachment #8905231 - Flags: review?(jdemooij)
Attachment #8905769 - Flags: review?(jdemooij)
(Assignee)

Updated

3 months ago
Attachment #8905231 - Flags: review?(jdemooij)
(Assignee)

Comment 17

3 months ago
Alright, did some refactoring so it is a less magic. This fixes the VarEnvironmentObject leak, but otherwise is just pushing code around. I tried to document some of the oddities expected of our embedding now that they can't hide as well in the code.

One large source of ugliness is the SubscriptLoader. It has silly compatibility requirements:
  GlobalThis -> with target
  ImplicitThis -> with target
  FunctionThis(null) -> global

This is why https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/js/src/vm/Interpreter.cpp#140 doesn't match https://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/js/src/vm/Interpreter.cpp#163

I'd like to do more investigation on DebugEnvironmentProxies and see if Bug 1397049 is related as a separate bug.
Comment on attachment 8905684 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS.

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

Forwarding to Ted.
Attachment #8905684 - Flags: review?(jdemooij) → review?(tcampbell)

Comment 19

3 months ago
mozreview-review
Comment on attachment 8905231 [details]
Bug 1397385 - Fixup js::ComputeImplicitThis to not leak environments

https://reviewboard.mozilla.org/r/177020/#review182750

::: js/src/vm/Interpreter.cpp:1487
(Diff revision 6)
> -        return UndefinedValue();
> +    if (env->is<GlobalObject>())
> -
> -    if (obj->is<NonSyntacticVariablesObject>())
>          return UndefinedValue();
>  
> -    if (obj->is<WithEnvironmentObject>())
> +    // WithEnvironmentObject have an actual implicit |this|

Nit: s/have/has/ or s/Object/Objects/ ? Also add a period at the end while you're here.

::: js/src/vm/Interpreter.cpp:1497
(Diff revision 6)
>      // non-syntactic, they wrap syntactic environments and should not be
>      // treated like other embedding-specific non-syntactic environments.
> -    if (obj->is<DebugEnvironmentProxy>())
> -        return ComputeImplicitThis(&obj->as<DebugEnvironmentProxy>().environment());
> +    if (env->is<DebugEnvironmentProxy>())
> +        return ComputeImplicitThis(&env->as<DebugEnvironmentProxy>().environment());
>  
> -    return GetThisValue(obj);
> +    MOZ_ASSERT(env->is<EnvironmentObject>());

Invariants! \o/
Attachment #8905231 - Flags: review?(jdemooij) → review+

Comment 20

3 months ago
mozreview-review
Comment on attachment 8905768 [details]
Bug 1397385 - Refactor js::GetThisValue

https://reviewboard.mozilla.org/r/177570/#review182752

::: js/src/vm/EnvironmentObject.cpp:1074
(Diff revision 1)
>      MOZ_ASSERT(isExtensible());
>      Value v = getReservedSlot(THIS_VALUE_OR_SCOPE_SLOT);
>      if (v.isObject()) {
> -        // If `v` is a Window, return the WindowProxy instead. We called
> -        // GetThisValue (which also does ToWindowProxyIfWindow) when storing
> -        // the value in THIS_VALUE_OR_SCOPE_SLOT, but it's possible the
> +        // A WindowProxy may have been attached after this environment was
> +        // created so check ToWindowProxyIfWindow again. For example,
> +        // GlobalObject::createInternal will construct it's lexical environment

Nit: s/it's/its/
Attachment #8905768 - Flags: review?(jdemooij) → review+

Comment 21

3 months ago
mozreview-review
Comment on attachment 8905769 [details]
Bug 1397385 - Simplify js::GetThisValue

https://reviewboard.mozilla.org/r/177572/#review182756

Much simpler! Very nice.

::: js/src/jsobj.cpp:3259
(Diff revision 1)
>  }
>  
>  Value
>  js::GetThisValue(JSObject* obj)
>  {
> +    // Outerize globals

Nit: I'd prefer sticking to Window/WindowProxy inside spidermonkey. Inner/outer windows are more of a Gecko-ism (SpiderMonkey used to have innerObject/outerObject Class hooks but I removed them a few years ago). Maybe say something like: Use the WindowProxy if the global is a Window, as the Window must never be exposed to JS.
Attachment #8905769 - Flags: review?(jdemooij) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

3 months ago
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e2a1d9ffd7a
Refactor js::GetThisValue r=jandem
https://hg.mozilla.org/integration/autoland/rev/aac13bc10a72
Fixup js::ComputeImplicitThis to not leak environments r=jandem
https://hg.mozilla.org/integration/autoland/rev/0d384af83d69
Simplify js::GetThisValue r=jandem
(Assignee)

Comment 26

3 months ago
Comment on attachment 8905684 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS.

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

::: js/src/vm/Opcodes.h
@@ +1626,5 @@
>      macro(JSOP_STRICTSETGNAME, 156, "strict-setgname",  NULL,       5,  2,  1, JOF_ATOM|JOF_NAME|JOF_PROPSET|JOF_DETECTING|JOF_GNAME|JOF_CHECKSTRICT) \
>      /*
>       * Pushes the implicit 'this' value for calls to the associated name onto
> +     * the stack; only used when we know the implicit this, if any, will be our
> +     * first non-syntactic scope.

This was never quite right in first place. The implicit this can be found on any non-syntactic scope (or the global). Should be something like 'only used when the implicit this might be found on a non-syntactic scope outside this script'.

@@ +1633,5 @@
> +     * objects on its scope chain, which are non-syntactic environments that
> +     * refer to syntactic environments. So this opcode does not imply that the
> +     * function being called was found on some ordinary JS object that is safe
> +     * to supply as a 'this` value; it may have been found on a CallObject or
> +     * VarEnvironmentObject referred to by a DebugEnvironmentProxy.

First sentence helps. The second sentence confuses me because the holder could already be a CallObject so the filtering is already required. Instead point out that 'As a result, the binding we want may be held by a syntactic environments such as CallObject / VarEnvrionmentObject.'
Attachment #8905684 - Flags: review?(tcampbell)

Comment 27

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7e2a1d9ffd7a
https://hg.mozilla.org/mozilla-central/rev/aac13bc10a72
https://hg.mozilla.org/mozilla-central/rev/0d384af83d69
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 28

3 months ago
Created attachment 8912907 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD

Revised per comments.
Attachment #8905684 - Attachment is obsolete: true
Attachment #8912907 - Flags: review?(tcampbell)
(Assignee)

Comment 29

3 months ago
Comment on attachment 8912907 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD

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

Thanks for updating this doc.
Attachment #8912907 - Flags: review?(tcampbell) → review+

Comment 30

3 months ago
Created attachment 8912908 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell

Updated

3 months ago
Attachment #8912907 - Attachment is obsolete: true

Comment 31

3 months ago
Comment on attachment 8912908 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell

Forgot to update patch per last suggestion on IRC. Carrying over r+.
Attachment #8912908 - Attachment description: Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD → Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell
Attachment #8912908 - Flags: review+

Comment 32

3 months ago
Comment on attachment 8912908 [details] [diff] [review]
Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell

># HG changeset patch
># User Jim Blandy <jimb@mozilla.com>
># Date 1504820250 25200
>#      Thu Sep 07 14:37:30 2017 -0700
># Node ID 62c6fed3a473a19e2820f88ae796bef12be2d74c
># Parent  756e10aa8bbd416cbc49b7739f78fb81d5525477
>Bug 1397385: Clarify use of JSOP_GIMPLICITTHIS. DONTBUILD r=tcampbell
>
>diff --git a/js/src/vm/Opcodes.h b/js/src/vm/Opcodes.h
>--- a/js/src/vm/Opcodes.h
>+++ b/js/src/vm/Opcodes.h
>@@ -1621,18 +1621,24 @@ 1234567890123456789012345678901234567890
>      *   Category: Variables and Scopes
>      *   Type: Free Variables
>      *   Operands: uint32_t nameIndex
>      *   Stack: env, val => val
>      */ \
>     macro(JSOP_STRICTSETGNAME, 156, "strict-setgname",  NULL,       5,  2,  1, JOF_ATOM|JOF_NAME|JOF_PROPSET|JOF_DETECTING|JOF_GNAME|JOF_CHECKSTRICT) \
>     /*
>      * Pushes the implicit 'this' value for calls to the associated name onto
>-     * the stack; only used when we know this implicit this will be our first
>-     * non-syntactic scope.
>+     * the stack; only used when the implicit this might be derived from a
>+     * non-syntactic scope (instead of the global itself).
>+     *
>+     * Note that code evaluated via the Debugger API uses DebugEnvironmentProxy
>+     * objects on its scope chain, which are non-syntactic environments that
>+     * refer to syntactic environments. As a result, the binding we want may be
>+     * held by a syntactic environments such as CallObject or
>+     * VarEnvrionmentObject.
>      *
>      *   Category: Variables and Scopes
>      *   Type: This
>      *   Operands: uint32_t nameIndex
>      *   Stack: => this
>      */                                                                 \
>     macro(JSOP_GIMPLICITTHIS, 157, "gimplicitthis", "",      5,  0,  1,  JOF_ATOM) \
>     /*

Comment 33

3 months ago
Created attachment 8912910 [details] [diff] [review]
implicitthis-comment.patch

I should not be allowed to operate heavy machinery today.
Attachment #8912908 - Attachment is obsolete: true
Attachment #8912910 - Flags: review+

Updated

3 months ago
Keywords: checkin-needed

Comment 34

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5b9b4480c4
Clarify use of JSOP_GIMPLICITTHIS. r=tcampbell
Keywords: checkin-needed

Comment 35

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/17a2791683e8

Seemed like a good idea anyway.

Comment 36

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5d5b9b4480c4
You need to log in before you can comment on or make changes to this bug.