Summary: | Invalid numeric and named references should be early syntax errors | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Alexey Shvayka <ashvayka> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Trivial | CC: | ashvayka, darin, ews-watchlist, keith_miller, mark.lam, ross.kirsling, saam, tzagallo, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=178174 https://bugs.webkit.org/show_bug.cgi?id=167067 |
||||||||||||||||
Bug Depends on: | 189407 | ||||||||||||||||
Bug Blocks: | |||||||||||||||||
Attachments: |
|
Description
Michael Saboff
2017-10-11 11:24:31 PDT
Created attachment 394228 [details]
Patch
I will add ChangeLog entry in a minute; just making sure we are not tackling the same issue separately. Created attachment 394256 [details]
Patch
Add ChangeLog and fix build.
Created attachment 394257 [details]
Patch
Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.
(In reply to Alexey Shvayka from comment #5) > Created attachment 394257 [details] > Patch > > Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT. I think we intentionally avoid this? I think for a release assert you may need to use NO_RETURN_DUE_TO_CRASH. > > Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.
>
> I think we intentionally avoid this? I think for a release assert you may
> need to use NO_RETURN_DUE_TO_CRASH.
Thank you, there are indeed no usages of [[noreturn]] in WebKit code base.
API tests fail because named and numeric references should be disabled for URLFilterParser. I might have need to bring back isValidNamedForwardReference().
Created attachment 394400 [details]
Patch
Use NO_RETURN_DUE_TO_CRASH, fix API tests, improve error messages, and tweak naming.
Comment on attachment 394400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394400&action=review Nice work! (Looks like you need a rebase though.) > Source/JavaScriptCore/yarr/YarrErrorCode.h:55 > + InvalidNamedBackreference, Interesting how it's `Backreference` here and `BackReference` elsewhere...though this isn't your fault. :P Created attachment 394553 [details]
Patch
Rebase patch, set reviewer, add second m_kIdentityEscapeSeen, check and add a comment explaining m_isUnicode check in handleIllegalReferences().
Created attachment 394555 [details]
Patch
Camelcase "backreference".
Thank you for taking time to review this patch, Ross. It is quite tricky. Comment on attachment 394555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=394555&action=review > Source/JavaScriptCore/yarr/YarrParser.h:919 > + if (m_kIdentityEscapeSeen && !m_captureGroupNames.isEmpty()) { > + m_errorCode = ErrorCode::InvalidNamedBackReference; > + return; > + } Do we need a test for this path, if nothing was failing without it? (In reply to Ross Kirsling from comment #13) > Do we need a test for this path, if nothing was failing without it? We have this covered by test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js I just didn't run the tests properly. (In reply to Alexey Shvayka from comment #14) > (In reply to Ross Kirsling from comment #13) > > Do we need a test for this path, if nothing was failing without it? > > We have this covered by > test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js > > test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js > > test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js > > I just didn't run the tests properly. Oh okay, thanks for confirming. Committed r259026: <https://trac.webkit.org/changeset/259026> All reviewed patches have been landed. Closing bug and clearing flags on attachment 394555 [details]. |