1670124 - \8 and \9 should be forbidden in strict mode strings and in template literals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Walden [:Waldo]]

Specs used to be maybe ambiguous on this, or permissive if it was allowed, but that got changed recently.

Comment 1

[Ganesh Sundar C [:obsu]]

Hi, can I take this up?

I am a beginner and have come here after watching compiler compiler series.

I tried to understand the code and am guessing the following should be done for fix

1. Create new error string - JSMSG_DEPRECATED_NON_OCTAL_DECIMAL - 'non octal decimal sequences are deprecated' (in ErrorNumbers.msg)
2. Create new InvalidEscapeToken type NonOctalDecimal (https://searchfox.org/mozilla-central/source/js/src/frontend/TokenStream.h#528)
3. Handle NonOctalDecimal case in reportInvalidEscapeError (https://searchfox.org/mozilla-central/source/js/src/frontend/TokenStream.h#2548)
4. Handle it at getStringOrTemplateToken (https://searchfox.org/mozilla-central/source/js/src/frontend/TokenStream.cpp#3647)
   • If template literal, setInvalidTemplateEscape to NonOctalDecimal
   • call strictModeError with JSMSG_DEPRECATED_NON_OCTAL_DECIMAL and return false if in strict mode(i.e. strictModeError() returns 0)
5. Remove skipped tests in jstests.list (https://searchfox.org/mozilla-central/source/js/src/tests/jstests.list#142)

If anyone can check and tell if the approach is correct, I would be happy to submit a patch. TIA

Comment 2

[Yulia Startsev [:yulia]]

Just repeating what I mentioned in element chat. I am not the expert in this but the general approach looks good. Happy to review.

By the way, since our searchfox source directory points to the head of the tree, links that are not permalinks might get out of date. You can make sure links are always pointing where you intend by clicking on the "permalink" in the right menu in searchfox.

Thanks for taking this on!

Comment 3

[Ganesh Sundar C [:obsu]]

Attachment: Bug 1670124 - Forbid \8 and \9 in strict mode strings and template literals. r?jwalden

Comment 4

[Ganesh Sundar C [:obsu]]

Thanks for your help.

I have submitted a patch and I was able to pass jit-test and jstests on my machine.

A couple of points

1. Changed strings from NON_OCTAL_DECIMAL to LEGACY_OCTAL as test262 test case refers this case as 'invalid legacy octal'
2. Whitespace changes in TokenStream.h & TokenStream.cpp got autoadded by "hg commit". Not sure what is the expected behaviour, so left it as it is.
3. Tried adding r=yulia,waldo & r=yulia,Waldo, but got error saying "waldo is not a valid reviewer's name"

Comment 5

[Jeff Walden [:Waldo]]

I'm pretty sure you'd want "jwalden" for reviewing, if you wanted to tag me.

I'm taking a look now because I've got a few (a few) minutes immediately. Not sure whether it'll blossom far enough to be a full review. (And my queue is backed up with Intl stuff, so it's a question of propriety just how much I ought let a short patch like this jump over other entries. :-) )

Comment 6

[Jeff Walden [:Waldo]]

Looks like it did blossom into a full review. Thanks for the patch, it's 90% there and 90% good, just needs a slight bit more to push it across the finish line. :-)

Comment 7

[Jeff Walden [:Waldo]]

Oh, for "Changed strings from NON_OCTAL_DECIMAL to LEGACY_OCTAL as test262 test case refers this case as 'invalid legacy octal'" -- "NonOctalDecimalEscapeSequence" is I suppose technically the bestest term for this, because it's what the spec uses for the relevant terminal. (The test262 case is likely some sort of copypasta, maybe misleading. Or it could be a generated testcase that as consequence just happens to have skew comments in it in some cases.) But I think the spec language is probably less readable than literally just saying the two possible characters, here, from the point of view of the developer reading this code. (And certainly for any web developer who inadvertently invokes this.)

Comment 8

[Ganesh Sundar C [:obsu]]

I have raised issue at test262 - (link) for the preceding strict case. I have made the changes requested and have submitted again. Thanks for taking time out to review despite a busy schedule :)

Comment 9

[Ganesh Sundar C [:obsu]]

Attachment: Bug 1670124 - Forbid \8 and \9 in strict mode strings and template literals. r=jwalden,yulia

Comment 10

[Tom S [:evilpie]]

Hey! This patch failed to land, because it didn't apply cleanly. Would you mind rebasing?

Comment 11

[Tom S [:evilpie]]

I rebased and tried to land the patch now.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b7df5f4ad70
Forbid \8 and \9 in strict mode strings and template literals. r=jwalden

Comment 13

[Natalia Csoregi [:nataliaCs]]

Backed out for bustage on TokenStream.h

backout: https://hg.mozilla.org/integration/autoland/rev/6ef0888cb89aa1daf428ceb1d0e5c4a088482339

push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&revision=2b7df5f4ad7043e722a888e4f0b2fc01f95b7eb4&selectedTaskRun=Mp3PSg-iRr6CG8WiecVRBw.0

failure log: https://treeherder.mozilla.org/logviewer?job_id=331115208&repo=autoland&lineNumber=1232

[task 2021-02-25T00:08:12.049Z] In file included from /builds/worker/checkouts/gecko/js/src/frontend/SyntaxParseHandler.h:19:0,
[task 2021-02-25T00:08:12.049Z] from /builds/worker/checkouts/gecko/js/src/frontend/Parser.h:190,
[task 2021-02-25T00:08:12.049Z] from /builds/worker/checkouts/gecko/js/src/debugger/Debugger.cpp:44,
[task 2021-02-25T00:08:12.049Z] from Unified_cpp_js_src_debugger0.cpp:11:
[task 2021-02-25T00:08:12.049Z] /builds/worker/checkouts/gecko/js/src/frontend/TokenStream.h:271:44: error: 'js::frontend::TokenStreamFlags::sawDeprecatedContent' is too small to hold all values of 'enum class js::frontend::DeprecatedContent' [-Werror]
[task 2021-02-25T00:08:12.049Z] DeprecatedContent sawDeprecatedContent : 2;
[task 2021-02-25T00:08:12.049Z] ^

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b200811e596b
Forbid \8 and \9 in strict mode strings and template literals. r=jwalden

Comment 15

[Ganesh Sundar C [:obsu]]

Hi evilpie, I got to see your comment too late. Apologies and thanks for landing it!

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b200811e596b

Comment 17

[Tom S [:evilpie]]

No problems, thanks for providing a patch!