Bug 154144 – The parser doesn't properly protect against global variable references in builtins

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

...

[Saam Barati]

Created attachment 271119 [details]
patch

[Oliver Hunt]

Comment on attachment 271119 [details]
patch

IIRC correctly the purpose of this logic wasn't to be correct wrt language semantics, just correct-enough to ensure that we didn't accidentally try to capture from the global object. It was essentially a safety thing for builtins.

[Saam Barati]

(In reply to comment #2)
> Comment on attachment 271119 [details]
> patch
> 
> IIRC correctly the purpose of this logic wasn't to be correct wrt language
> semantics, just correct-enough to ensure that we didn't accidentally try to
> capture from the global object. It was essentially a safety thing for
> builtins.

Which logic are you talking about?

[Saam Barati]

I think you're probably talking about this loop:
-        if (!capturedVariables.isEmpty()) {
-            for (const auto& capturedVariable : capturedVariables) {
-                if (scope->hasDeclaredVariable(capturedVariable))
-                    continue;
-
-                if (scope->hasDeclaredParameter(capturedVariable))
-                    continue;
-
-                RELEASE_ASSERT_NOT_REACHED();
-            }
-        }

And if so, we want the logic to be something like:
```
if (m_parsingBuiltin)
    RELEASE_ASSERT(capturedVariableCandidates().isEmpty());
```

Out of curiosity, why do we enforce this? Is it just that
global properties can be changed by user code?

[Oliver Hunt]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 271119 [details]
> > patch
> > 
> > IIRC correctly the purpose of this logic wasn't to be correct wrt language
> > semantics, just correct-enough to ensure that we didn't accidentally try to
> > capture from the global object. It was essentially a safety thing for
> > builtins.
> 
> Which logic are you talking about?

Literally the entirety of what you just removed.

Again, this is very much an "IIRC", and all I can recall is that the particular set of rules it ended up enforcing were those necessary to disallow capturing global variables. I agree the mismatched naming vs. semantics warrant fixing, but with this change it becomes possible to accidentally capture unprivileged global variables, e.g. Object vs @Object.

Given the testing we could also change the logic that achieves this to be at bytecode generation time to fail out on any non-local resolve (which is probably a more sensible way to do it).

[Oliver Hunt]

(In reply to comment #4)
> I think you're probably talking about this loop:
> -        if (!capturedVariables.isEmpty()) {
> -            for (const auto& capturedVariable : capturedVariables) {
> -                if (scope->hasDeclaredVariable(capturedVariable))
> -                    continue;
> -
> -                if (scope->hasDeclaredParameter(capturedVariable))
> -                    continue;
> -
> -                RELEASE_ASSERT_NOT_REACHED();
> -            }
> -        }
> 
> And if so, we want the logic to be something like:
> ```
> if (m_parsingBuiltin)
>     RELEASE_ASSERT(capturedVariableCandidates().isEmpty());
> ```
> 
> Out of curiosity, why do we enforce this? Is it just that
> global properties can be changed by user code?

Yup, which (among other things) leads to sadness in language semantics, as well as potential security issues.

[Saam Barati]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 271119 [details]
> > > patch
> > > 
> > > IIRC correctly the purpose of this logic wasn't to be correct wrt language
> > > semantics, just correct-enough to ensure that we didn't accidentally try to
> > > capture from the global object. It was essentially a safety thing for
> > > builtins.
> > 
> > Which logic are you talking about?
> 
> Literally the entirety of what you just removed.
> 
> Again, this is very much an "IIRC", and all I can recall is that the
> particular set of rules it ended up enforcing were those necessary to
> disallow capturing global variables. I agree the mismatched naming vs.
> semantics warrant fixing, but with this change it becomes possible to
> accidentally capture unprivileged global variables, e.g. Object vs @Object.
> 
> Given the testing we could also change the logic that achieves this to be at
> bytecode generation time to fail out on any non-local resolve (which is
> probably a more sensible way to do it).

Okay. I agree that we should enforce this. I was just observing
when reading the code that the code doesn't do anything.

We need to come up with some rules that work. I don't think
this can be done from the BytecodeGenerator. It would be possible
to do during Bytecode linking, though.

I think solving this in the parser still might be easiest. I think
the logic would go something like this.
```
after parsing is complete, do:
if (weAreParsingAProgram()) {
    if (captureVariableCandidates() \ definedVariables() != emptySet)
         crash()
}
```

[Saam Barati]

Comment on attachment 271119 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271119&action=review

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:-115
> -    for (const auto& closedVariable : program->closedVariables()) {
> -        if (closedVariable == vm.propertyNames->arguments.impl())
> -            continue;
> -        
> -        if (closedVariable == vm.propertyNames->undefinedKeyword.impl())
> -            continue;
> -    }

Oliver, do you remember why we had this here?
Instead of strictly being in the parser?
This is a very curious loop, it looks like it's probably been edited to death.

[Saam Barati]

I wonder if we have any errors that have slipped through here.

[Saam Barati]

(In reply to comment #9)
> I wonder if we have any errors that have slipped through here.

Yup. We totally do!

[Saam Barati]

(In reply to comment #10)
> (In reply to comment #9)
> > I wonder if we have any errors that have slipped through here.
> 
> Yup. We totally do!

Oh man, we get it wrong in so many places :/

[Saam Barati]

Created attachment 271238 [details]
patch

[WebKit Commit Bot]

Attachment 271238 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Parser.cpp:304:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/parser/Parser.cpp:305:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Created attachment 271239 [details]
patch

[WebKit Commit Bot]

Attachment 271239 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 271239 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review

r=me

> Source/JavaScriptCore/parser/Parser.cpp:334
> +            if (!lexicalVariables.contains(candidate) && !varDeclarations.contains(candidate) && !builtinNames.isPrivateName(*candidate.get())) {
> +                dataLog("Bad global capture in builtin: '", candidate, "'\n");
> +                dataLog(m_source->view());
> +                CRASH();
>              }

Can we make this an ASSERT instead?

[Mark Lam]

Comment on attachment 271239 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review

> Source/JavaScriptCore/parser/Parser.cpp:302
> +    scope->getCapturedVars(capturedVariables, modifiedParameter, modifiedArguments); // FIXME: make this just find out modifedParamerer/modifiedArguments

Maybe you should file a bug for this (and include the bug url here)?

[Saam Barati]

Comment on attachment 271239 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:302
>> +    scope->getCapturedVars(capturedVariables, modifiedParameter, modifiedArguments); // FIXME: make this just find out modifedParamerer/modifiedArguments
> 
> Maybe you should file a bug for this (and include the bug url here)?

oops, this FIXME should just be removed. I was anticipating rewriting this function but I didn't

[Saam Barati]

Comment on attachment 271239 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271239&action=review

>> Source/JavaScriptCore/parser/Parser.cpp:334
>>              }
> 
> Can we make this an ASSERT instead?

I think this reporting is actually very helpful. I'm going to keep it.
I'm going to put the whole loop under #ifndef NDEBUG

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/196525