ComputeImplicitThis leaks VarEnvironmentObjects to script

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: tcampbell, Assigned: tcampbell)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

a year 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

a year ago
Assignee: nobody → tcampbell
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Blocks: 1385429
Comment hidden (mozreview-request)

Updated

a year ago
Duplicate of this bug: 1385429

Comment 3

a year ago
Could we include the test case in bug 1385429 comment 5 in this patch?

Comment 4

a year 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

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

Comment 11

a year 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

a year 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

a year 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

a year ago
Attachment #8905768 - Flags: review?(jdemooij)
Attachment #8905231 - Flags: review?(jdemooij)
Attachment #8905769 - Flags: review?(jdemooij)
(Assignee)

Updated

a year ago
Attachment #8905231 - Flags: review?(jdemooij)
(Assignee)

Comment 17

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 28

a year 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

a year 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

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

Updated

a year ago
Attachment #8912907 - Attachment is obsolete: true

Comment 31

a year 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

a year 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

a year 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

a year ago
Keywords: checkin-needed

Comment 34

a year 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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/17a2791683e8

Seemed like a good idea anyway.
You need to log in before you can comment on or make changes to this bug.