Bug 163930

Summary: [JSC] forbid lexical redeclaration of generator formal parameters
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: New BugsAssignee: Caitlin Potter (:caitp) <caitp>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, keith_miller, mark.lam, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Caitlin Potter (:caitp) 2016-10-24 17:45:05 PDT
[JSC] forbid lexical redeclaration of generator formal parameters
Comment 1 Caitlin Potter (:caitp) 2016-10-24 17:46:05 PDT
Created attachment 292686 [details]
Patch
Comment 2 Caitlin Potter (:caitp) 2016-10-24 17:49:55 PDT
Created attachment 292687 [details]
Patch
Comment 3 Caitlin Potter (:caitp) 2016-10-24 18:02:46 PDT
couple of test262 failures (for both generators and async functions) fixed here, and some new tests added for previously untested things that were actually working correctly. Good to have coverage :>
Comment 4 Caitlin Potter (:caitp) 2016-10-26 14:47:07 PDT
Created attachment 292963 [details]
Patch

Add comments describing why things are done in isValidStrictMode() and hasDeclaredParameter()
Comment 5 Yusuke Suzuki 2016-10-26 15:14:50 PDT
Comment on attachment 292963 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Also forbids "arguments" and "eval" as generator argument names in strict mode.

Nice!

> Source/JavaScriptCore/parser/Parser.h:1238
>          return m_scopeStack[i].hasDeclaredParameter(ident);

This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
So we never call this function during reparsing. It returns the incorrect result.

It is better to rename this function to the other name that represents this function is not valid during the reparsing.

> Source/JavaScriptCore/parser/Parser.h:1403
> +    }

I think this name is also misleading. Can you rename it like, `isValidStatusForStrictMode()`? (And changing m_validStrictMode to the renamed one).
Comment 6 Caitlin Potter (:caitp) 2016-10-26 15:29:19 PDT
Comment on attachment 292963 [details]
Patch

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

>> Source/JavaScriptCore/parser/Parser.h:1238
>>          return m_scopeStack[i].hasDeclaredParameter(ident);
> 
> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
> So we never call this function during reparsing. It returns the incorrect result.
> 
> It is better to rename this function to the other name that represents this function is not valid during the reparsing.

From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.

I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.

>> Source/JavaScriptCore/parser/Parser.h:1403
>> +    }
> 
> I think this name is also misleading. Can you rename it like, `isValidStatusForStrictMode()`? (And changing m_validStrictMode to the renamed one).

"isValidStrictMode" seems ok to me, I read it answering the question "does this pass strict mode static restrictions?". Maybe Saam has an opinion?

I'll do it if folks prefer that, but it's sort of outside of the scope of what this patch is meant to fix.
Comment 7 Caitlin Potter (:caitp) 2016-10-26 15:32:03 PDT
Comment on attachment 292963 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>> 
>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>> So we never call this function during reparsing. It returns the incorrect result.
>> 
>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
> 
> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
> 
> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.

Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).
Comment 8 Yusuke Suzuki 2016-10-26 16:13:03 PDT
Comment on attachment 292963 [details]
Patch

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

>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>> 
>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>> So we never call this function during reparsing. It returns the incorrect result.
>>> 
>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>> 
>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>> 
>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
> 
> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).

Oops, I was considering about generator wrapper function case.

Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).
Comment 9 Caitlin Potter (:caitp) 2016-10-26 16:28:13 PDT
Comment on attachment 292963 [details]
Patch

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

>>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>>> 
>>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>>> So we never call this function during reparsing. It returns the incorrect result.
>>>> 
>>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>>> 
>>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>>> 
>>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
>> 
>> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).
> 
> Oops, I was considering about generator wrapper function case.
> 
> Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).

Would you prefer a `gif (not a wrapper function) return false;` after decrementing i?
Comment 10 Yusuke Suzuki 2016-10-26 17:17:10 PDT
Comment on attachment 292963 [details]
Patch

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

>>>>>> Source/JavaScriptCore/parser/Parser.h:1238
>>>>>>          return m_scopeStack[i].hasDeclaredParameter(ident);
>>>>> 
>>>>> This `hasDeclaredParameter` function is dangerous I think. When performing the syntax checking onto the code, it is OK.
>>>>> But when actually parsing the generator body function to create the AST, the scope in this line(m_scopeStack[i]) becomes the global scope, right?
>>>>> So we never call this function during reparsing. It returns the incorrect result.
>>>>> 
>>>>> It is better to rename this function to the other name that represents this function is not valid during the reparsing.
>>>> 
>>>> From local testing, during reparsing, there is no outer scope object (which is why it checks if `i` is non-zero). Otherwise there is an OOB problem.
>>>> 
>>>> I could add an assertion that the outer scope is an async function wrapper or generator function wrapper and see if it still passes everything, I guess, but it seems that there is no global scope when reparsing closures.
>>> 
>>> Also note, even if it is the global scope during reparsing, it doesn't matter, because `valid-strict-mode-ness` doesn't matter during reparsing (if it's invalid during strict mode, it should be a SyntaxError and never get re-parsed).
>> 
>> Oops, I was considering about generator wrapper function case.
>> 
>> Yeah, but if you call `hasDeclaredParameter(...)` in the reparsing phase for the other purpose, we have a bad time (this function returns the incorrect answer).
> 
> Would you prefer a `gif (not a wrapper function) return false;` after decrementing i?

Since this function (Parser::hasDeclaredParameter) is only used for this error checking purpose currently, I suggest renaming this function to avoid incorrect use and inserting assertion for isReparsing.
Comment 11 Caitlin Potter (:caitp) 2016-10-26 18:07:35 PDT
Created attachment 292982 [details]
Patch

Rename Parser::hasDeclaredParameter() and Parser::hasDeclaredVariable(), don't call Parser::hasDeclaredParameter() when reparsing, and assert that not reparsing when it is called
Comment 12 Caitlin Potter (:caitp) 2016-10-27 15:41:19 PDT
Created attachment 293070 [details]
Patch

Updated per offline discussions
Comment 13 Yusuke Suzuki 2016-10-27 15:43:32 PDT
Comment on attachment 293070 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2016-10-27 16:23:28 PDT
Comment on attachment 293070 [details]
Patch

Clearing flags on attachment: 293070

Committed r208016: <http://trac.webkit.org/changeset/208016>
Comment 15 WebKit Commit Bot 2016-10-27 16:23:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Saam Barati 2016-10-28 01:43:19 PDT
Comment on attachment 293070 [details]
Patch

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

Bugs like these make me think that it's a bit ineligant that our parser so closely understands the implementation details of generator functions. In some ways, it'd be nicer if the parser didn't know that we lower generators to an outer and an inner function. It could just be worried with parsing generators at a higher level, and we could take the parser's output and generate the outer and the inner function at a later stage. I'm not sure if it's worth doing anything like this now, but the current design makes it harder to reason about properly "syntax checking" generator programs.

> Source/JavaScriptCore/parser/Parser.h:1400
> +        int i = m_scopeStack.size() - 1;

Nit: I'd use size_t here and ASSERT that scopeStack is non-empty