Bug 178175 - Invalid numeric and named references should be early syntax errors
Summary: Invalid numeric and named references should be early syntax errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Trivial
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on: 189407
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-11 11:24 PDT by Michael Saboff
Modified: 2020-03-25 18:24 PDT (History)
9 users (show)

See Also:


Attachments
Patch (27.06 KB, patch)
2020-03-22 13:56 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (28.57 KB, patch)
2020-03-23 05:41 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (28.54 KB, patch)
2020-03-23 06:00 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (36.86 KB, patch)
2020-03-24 13:04 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (38.13 KB, patch)
2020-03-25 15:45 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (38.13 KB, patch)
2020-03-25 15:52 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2017-10-11 11:24:31 PDT
This Test262 test is failing - JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js
Exception: SyntaxError: Invalid regular expression: invalid backreference for unicode pattern
at JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js:33

The line in question is:
  assert(compareArray(["bab", "b"], "bab".match(/\k<a>(?<a>b)\w\k<a>/u)));

Here the named group “a” is being referenced before if is defined.  Seems like we’re doing the right thing, but we need to check.
Comment 1 Michael Saboff 2017-10-11 11:25:28 PDT
<rdar://problem/34844068>
Comment 2 Alexey Shvayka 2020-03-22 13:56:08 PDT
Created attachment 394228 [details]
Patch
Comment 3 Alexey Shvayka 2020-03-22 14:01:12 PDT
I will add ChangeLog entry in a minute; just making sure we are not tackling the same issue separately.
Comment 4 Alexey Shvayka 2020-03-23 05:41:42 PDT
Created attachment 394256 [details]
Patch

Add ChangeLog and fix build.
Comment 5 Alexey Shvayka 2020-03-23 06:00:06 PDT
Created attachment 394257 [details]
Patch

Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.
Comment 6 Ross Kirsling 2020-03-23 12:45:37 PDT
(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.
Comment 7 Alexey Shvayka 2020-03-23 12:53:02 PDT
> > 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().
Comment 8 Alexey Shvayka 2020-03-24 13:04:04 PDT
Created attachment 394400 [details]
Patch

Use NO_RETURN_DUE_TO_CRASH, fix API tests, improve error messages, and tweak naming.
Comment 9 Ross Kirsling 2020-03-24 20:10:42 PDT
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
Comment 10 Alexey Shvayka 2020-03-25 15:45:10 PDT
Created attachment 394553 [details]
Patch

Rebase patch, set reviewer, add second m_kIdentityEscapeSeen,  check and add a comment explaining m_isUnicode check in handleIllegalReferences().
Comment 11 Alexey Shvayka 2020-03-25 15:52:20 PDT
Created attachment 394555 [details]
Patch

Camelcase "backreference".
Comment 12 Alexey Shvayka 2020-03-25 15:59:29 PDT
Thank you for taking time to review this patch, Ross.
It is quite tricky.
Comment 13 Ross Kirsling 2020-03-25 16:01:17 PDT
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?
Comment 14 Alexey Shvayka 2020-03-25 16:05:56 PDT
(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.
Comment 15 Ross Kirsling 2020-03-25 16:08:00 PDT
(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.
Comment 16 EWS 2020-03-25 18:24:11 PDT
Committed r259026: <https://trac.webkit.org/changeset/259026>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394555 [details].