Bug 171310 – We incorrectly allow escaped characters in keyword tokens

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

[Saam Barati]

e.g, we parse: "i\u006E" as "in"

[GSkachkov]

There is a link to spec https://tc39.github.io/ecma262/#sec-reserved-words

[GSkachkov]

Created attachment 312488 [details]
Patch

WIP: test is coming

[Build Bot]

Comment on attachment 312488 [details]
Patch

Attachment 312488 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3902642

New failing tests:
stress/error-messages-for-in-operator-should-not-crash.js.ftl-no-cjit-no-inline-validate
stress/error-messages-for-in-operator-should-not-crash.js.dfg-eager
stress/reserved-word-with-escape.js.no-ftl
stress/destructuring-assignment-syntax.js.dfg-eager-no-cjit-validate
stress/reserved-word-with-escape.js.ftl-no-cjit-b3o1
stress/error-messages-for-in-operator-should-not-crash.js.ftl-eager
stress/error-messages-for-in-operator-should-not-crash.js.ftl-eager-no-cjit
stress/reserved-word-with-escape.js.no-llint
stress/reserved-word-with-escape.js.ftl-eager
stress/destructuring-assignment-syntax.js.ftl-eager-no-cjit-b3o1
stress/error-messages-for-in-operator-should-not-crash.js.dfg-maximal-flush-validate-no-cjit
stress/destructuring-assignment-syntax.js.ftl-no-cjit-validate-sampling-profiler
stress/error-messages-for-in-operator-should-not-crash.js.ftl-eager-no-cjit-b3o1
stress/error-messages-for-in-operator-should-not-crash.js.default
stress/reserved-word-with-escape.js.dfg-maximal-flush-validate-no-cjit
stress/error-messages-for-in-operator-should-not-crash.js.ftl-no-cjit-no-put-stack-validate
stress/destructuring-assignment-syntax.js.no-ftl
stress/error-messages-for-in-operator-should-not-crash.js.dfg-eager-no-cjit-validate
stress/destructuring-assignment-syntax.js.no-cjit-collect-continuously
stress/destructuring-assignment-syntax.js.ftl-eager-no-cjit
stress/destructuring-assignment-syntax.js.no-cjit-validate-phases
stress/reserved-word-with-escape.js.ftl-eager-no-cjit-b3o1
stress/reserved-word-with-escape.js.ftl-no-cjit-no-put-stack-validate
stress/destructuring-assignment-syntax.js.no-llint
stress/destructuring-assignment-syntax.js.default
stress/destructuring-assignment-syntax.js.ftl-no-cjit-no-put-stack-validate
stress/reserved-word-with-escape.js.dfg-eager-no-cjit-validate
stress/error-messages-for-in-operator-should-not-crash.js.ftl-no-cjit-small-pool
stress/error-messages-for-in-operator-should-not-crash.js.no-llint
ChakraCore.yaml/ChakraCore/test/Basics/IdsWithEscapes.js.default
stress/destructuring-assignment-syntax.js.ftl-no-cjit-small-pool
stress/error-messages-for-in-operator-should-not-crash.js.no-cjit-collect-continuously
stress/reserved-word-with-escape.js.default
stress/destructuring-assignment-syntax.js.ftl-no-cjit-b3o1
stress/reserved-word-with-escape.js.no-cjit-collect-continuously
stress/error-messages-for-in-operator-should-not-crash.js.ftl-no-cjit-validate-sampling-profiler
stress/error-messages-for-in-operator-should-not-crash.js.no-cjit-validate-phases
stress/reserved-word-with-escape.js.ftl-no-cjit-validate-sampling-profiler
stress/destructuring-assignment-syntax.js.dfg-eager
stress/reserved-word-with-escape.js.dfg-eager
stress/error-messages-for-in-operator-should-not-crash.js.no-ftl
stress/destructuring-assignment-syntax.js.dfg-maximal-flush-validate-no-cjit
stress/destructuring-assignment-syntax.js.ftl-eager
stress/reserved-word-with-escape.js.ftl-eager-no-cjit
stress/reserved-word-with-escape.js.ftl-no-cjit-small-pool
stress/destructuring-assignment-syntax.js.ftl-no-cjit-no-inline-validate
stress/reserved-word-with-escape.js.ftl-no-cjit-no-inline-validate
stress/reserved-word-with-escape.js.no-cjit-validate-phases
stress/error-messages-for-in-operator-should-not-crash.js.ftl-no-cjit-b3o1

[GSkachkov]

Created attachment 312568 [details]
Patch

Patch

[Build Bot]

Comment on attachment 312568 [details]
Patch

Attachment 312568 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3906552

New failing tests:
ChakraCore.yaml/ChakraCore/test/Basics/IdsWithEscapes.js.default

[GSkachkov]

Created attachment 312579 [details]
Patch

Fix tests

[Yusuke Suzuki]

Comment on attachment 312579 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Current patch implements this requirements.

The spec text says

"The ReservedWord definitions are specified as literal sequences of specific SourceCharacter elements. A code point in a ReservedWord cannot be expressed by a \ UnicodeEscapeSequence."

Does it mean that fals\x65 is allowed?

[GSkachkov]

Comment on attachment 312579 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:10
>> +        Current patch implements this requirements.
> 
> The spec text says
> 
> "The ReservedWord definitions are specified as literal sequences of specific SourceCharacter elements. A code point in a ReservedWord cannot be expressed by a \ UnicodeEscapeSequence."
> 
> Does it mean that fals\x65 is allowed?

No, I think that it is not allowed. 
According to spec ReservedWord is "A reserved word is an `IdentifierName` that cannot be used as an Identifier.", and the IdentifierName according to the https://tc39.github.io/ecma262/#prod-IdentifierName allows to use  \UnicodeEscapeSequence and UnicodeIDStart, $, _, so this NOTE means that we remove from the list of allowed symbols `\UnicodeEscapeSequence`, but it does not mean that we can use other symbols expect UnicodeIDStart/UnicodeIDContinue, $, _

[Yusuke Suzuki]

Comment on attachment 312579 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 312579 [details]
Patch

Clearing flags on attachment: 312579

Committed r218111: <http://trac.webkit.org/changeset/218111>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.