Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regex matching changed in 1.8.0 #981

Closed
autarch opened this issue Apr 21, 2023 · 1 comment · Fixed by #984
Closed

Regex matching changed in 1.8.0 #981

autarch opened this issue Apr 21, 2023 · 1 comment · Fixed by #984
Labels

Comments

@autarch
Copy link
Contributor

autarch commented Apr 21, 2023

What version of regex are you using?

This appears to be a bug in 1.8.0.

Describe the bug at a high level.

Something in the regex matching changed in 1.8.0. I suspect it might be a bug in the handling of word boundaries, \b.

What are the steps to reproduce the behavior?

use regex::Regex;

fn main() {
    let re = Regex::new(r#"(?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))"#).unwrap();
    for s in ["ubi-Darwin-x86_64.tar.gz", "ubi-Windows-x86_64.zip"] {
        println!("{s} =~ /{}/ => {}", re, re.is_match(s));
    }
}

With regex 1.8.0 the given regex will match both strings, which is surprising. The "win" in "Darwin" is not preceded by a word boundary or underscore. In 1.7.3, this matches only the second string, as I'd expect.

What is the actual behavior?

With 1.8.0:

$> cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/regex-issue`
ubi-Darwin-x86_64.tar.gz =~ /(?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))/ => true
ubi-Windows-x86_64.zip =~ /(?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))/ => true

With 1.7.3:

$> cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.00s
     Running `target/debug/regex-issue`
ubi-Darwin-x86_64.tar.gz =~ /(?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))/ => false
ubi-Windows-x86_64.zip =~ /(?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))/ => true

What is the expected behavior?

I expect this to work the way it does in 1.7.3.

autarch added a commit to houseabsolute/ubi that referenced this issue Apr 21, 2023
There appears to be a bug in regex 1.8.0 -
rust-lang/regex#981
@BurntSushi BurntSushi added the bug label Apr 21, 2023
BurntSushi added a commit that referenced this issue Apr 21, 2023
This commit fixes a bug where it was possible to report a match where
none existed. Basically, in the current regex crate, it just cannot deal
with a mixture of look-around assertions in the prefix of a pattern and
prefix literal optimizations. Before 1.8, this was handled by simply
refusing to extract literals in that case. But in 1.8, with a rewrite of
the literal extractor, literals are now extracted for patterns like
this:

    (?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))

So in 1.8, since it was still using the old engines that can't deal with
this, I added some extra logic to throw away any extracted prefix
literals if a look-around assertion occurred in the prefix of the
pattern. The problem is that the logic I used was "always occurs in the
prefix of the pattern" instead of "may occur in the prefix of the
pattern." In the pattern above, it's the latter case. So it slipped by
and the regex engine tried to use the prefix literals to accelerat the
search. This in turn caused mishandling of the `\b` and led to a false
positive match.

The specific reason why the current regex engines can't deal with this
is because they weren't designed to handle searches that took the
surrounding context into account when resolving look-around assertions.
It was a pretty big oversight on my part many years ago.

The new engines we'll be migrating to Real Soon Now don't have this
problem and can deal with the prefix literal optimizations while
correctly handling look-around assertions in the prefix.

Fixes #981
BurntSushi added a commit that referenced this issue Apr 21, 2023
This commit fixes a bug where it was possible to report a match where
none existed. Basically, in the current regex crate, it just cannot deal
with a mixture of look-around assertions in the prefix of a pattern and
prefix literal optimizations. Before 1.8, this was handled by simply
refusing to extract literals in that case. But in 1.8, with a rewrite of
the literal extractor, literals are now extracted for patterns like
this:

    (?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))

So in 1.8, since it was still using the old engines that can't deal with
this, I added some extra logic to throw away any extracted prefix
literals if a look-around assertion occurred in the prefix of the
pattern. The problem is that the logic I used was "always occurs in the
prefix of the pattern" instead of "may occur in the prefix of the
pattern." In the pattern above, it's the latter case. So it slipped by
and the regex engine tried to use the prefix literals to accelerat the
search. This in turn caused mishandling of the `\b` and led to a false
positive match.

The specific reason why the current regex engines can't deal with this
is because they weren't designed to handle searches that took the
surrounding context into account when resolving look-around assertions.
It was a pretty big oversight on my part many years ago.

The new engines we'll be migrating to Real Soon Now don't have this
problem and can deal with the prefix literal optimizations while
correctly handling look-around assertions in the prefix.

Fixes #981
BurntSushi added a commit that referenced this issue Apr 21, 2023
This commit fixes a bug where it was possible to report a match where
none existed. Basically, in the current regex crate, it just cannot deal
with a mixture of look-around assertions in the prefix of a pattern and
prefix literal optimizations. Before 1.8, this was handled by simply
refusing to extract literals in that case. But in 1.8, with a rewrite of
the literal extractor, literals are now extracted for patterns like
this:

    (?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))

So in 1.8, since it was still using the old engines that can't deal with
this, I added some extra logic to throw away any extracted prefix
literals if a look-around assertion occurred in the prefix of the
pattern. The problem is that the logic I used was "always occurs in the
prefix of the pattern" instead of "may occur in the prefix of the
pattern." In the pattern above, it's the latter case. So it slipped by
and the regex engine tried to use the prefix literals to accelerat the
search. This in turn caused mishandling of the `\b` and led to a false
positive match.

The specific reason why the current regex engines can't deal with this
is because they weren't designed to handle searches that took the
surrounding context into account when resolving look-around assertions.
It was a pretty big oversight on my part many years ago.

The new engines we'll be migrating to Real Soon Now don't have this
problem and can deal with the prefix literal optimizations while
correctly handling look-around assertions in the prefix.

Fixes #981
BurntSushi added a commit that referenced this issue Apr 21, 2023
This commit fixes a bug where it was possible to report a match where
none existed. Basically, in the current regex crate, it just cannot deal
with a mixture of look-around assertions in the prefix of a pattern and
prefix literal optimizations. Before 1.8, this was handled by simply
refusing to extract literals in that case. But in 1.8, with a rewrite of
the literal extractor, literals are now extracted for patterns like
this:

    (?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))

So in 1.8, since it was still using the old engines that can't deal with
this, I added some extra logic to throw away any extracted prefix
literals if a look-around assertion occurred in the prefix of the
pattern. The problem is that the logic I used was "always occurs in the
prefix of the pattern" instead of "may occur in the prefix of the
pattern." In the pattern above, it's the latter case. So it slipped by
and the regex engine tried to use the prefix literals to accelerat the
search. This in turn caused mishandling of the `\b` and led to a false
positive match.

The specific reason why the current regex engines can't deal with this
is because they weren't designed to handle searches that took the
surrounding context into account when resolving look-around assertions.
It was a pretty big oversight on my part many years ago.

The new engines we'll be migrating to Real Soon Now don't have this
problem and can deal with the prefix literal optimizations while
correctly handling look-around assertions in the prefix.

Fixes #981
BurntSushi added a commit that referenced this issue Apr 21, 2023
This commit fixes a bug where it was possible to report a match where
none existed. Basically, in the current regex crate, it just cannot deal
with a mixture of look-around assertions in the prefix of a pattern and
prefix literal optimizations. Before 1.8, this was handled by simply
refusing to extract literals in that case. But in 1.8, with a rewrite of
the literal extractor, literals are now extracted for patterns like
this:

    (?i:(?:\b|_)win(?:32|64|dows)?(?:\b|_))

So in 1.8, since it was still using the old engines that can't deal with
this, I added some extra logic to throw away any extracted prefix
literals if a look-around assertion occurred in the prefix of the
pattern. The problem is that the logic I used was "always occurs in the
prefix of the pattern" instead of "may occur in the prefix of the
pattern." In the pattern above, it's the latter case. So it slipped by
and the regex engine tried to use the prefix literals to accelerat the
search. This in turn caused mishandling of the `\b` and led to a false
positive match.

The specific reason why the current regex engines can't deal with this
is because they weren't designed to handle searches that took the
surrounding context into account when resolving look-around assertions.
It was a pretty big oversight on my part many years ago.

The new engines we'll be migrating to Real Soon Now don't have this
problem and can deal with the prefix literal optimizations while
correctly handling look-around assertions in the prefix.

Fixes #981
@BurntSushi
Copy link
Member

This is fixed in regex 1.8.1. Thank you for the report!

crapStone added a commit to Calciumdibromid/CaBr2 that referenced this issue May 2, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [regex](https://github.com/rust-lang/regex) | dependencies | minor | `1.7.3` -> `1.8.1` |

---

### Release Notes

<details>
<summary>rust-lang/regex</summary>

### [`v1.8.1`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#&#8203;181-2023-04-21)

\==================
This is a patch release that fixes a bug where a regex match could be reported
where none was found. Specifically, the bug occurs when a pattern contains some
literal prefixes that could be extracted *and* an optional word boundary in the
prefix.

Bug fixes:

-   [BUG #&#8203;981](rust-lang/regex#981):
    Fix a bug where a word boundary could interact with prefix literal
    optimizations and lead to a false positive match.

### [`v1.8.0`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#&#8203;180-2023-04-20)

\==================
This is a sizeable release that will be soon followed by another sizeable
release. Both of them will combined close over 40 existing issues and PRs.

This first release, despite its size, essentially represents preparatory work
for the second release, which will be even bigger. Namely, this release:

-   Increases the MSRV to Rust 1.60.0, which was released about 1 year ago.
-   Upgrades its dependency on `aho-corasick` to the recently released 1.0
    version.
-   Upgrades its dependency on `regex-syntax` to the simultaneously released
    `0.7` version. The changes to `regex-syntax` principally revolve around a
    rewrite of its literal extraction code and a number of simplifications and
    optimizations to its high-level intermediate representation (HIR).

The second release, which will follow ~shortly after the release above, will
contain a soup-to-nuts rewrite of every regex engine. This will be done by
bringing [`regex-automata`](https://github.com/BurntSushi/regex-automata) into
this repository, and then changing the `regex` crate to be nothing but an API
shim layer on top of `regex-automata`'s API.

These tandem releases are the culmination of about 3
years of on-and-off work that [began in earnest in March
2020](rust-lang/regex#656).

Because of the scale of changes involved in these releases, I would love to
hear about your experience. Especially if you notice undocumented changes in
behavior or performance changes (positive *or* negative).

Most changes in the first release are listed below. For more details, please
see the commit log, which reflects a linear and decently documented history
of all changes.

New features:

-   [FEATURE #&#8203;501](rust-lang/regex#501):
    Permit many more characters to be escaped, even if they have no significance.
    More specifically, any ASCII character except for `[0-9A-Za-z<>]` can now be
    escaped. Also, a new routine, `is_escapeable_character`, has been added to
    `regex-syntax` to query whether a character is escapeable or not.
-   [FEATURE #&#8203;547](rust-lang/regex#547):
    Add `Regex::captures_at`. This filles a hole in the API, but doesn't otherwise
    introduce any new expressive power.
-   [FEATURE #&#8203;595](rust-lang/regex#595):
    Capture group names are now Unicode-aware. They can now begin with either a `_`
    or any "alphabetic" codepoint. After the first codepoint, subsequent codepoints
    can be any sequence of alpha-numeric codepoints, along with `_`, `.`, `[` and
    `]`. Note that replacement syntax has not changed.
-   [FEATURE #&#8203;810](rust-lang/regex#810):
    Add `Match::is_empty` and `Match::len` APIs.
-   [FEATURE #&#8203;905](rust-lang/regex#905):
    Add an `impl Default for RegexSet`, with the default being the empty set.
-   [FEATURE #&#8203;908](rust-lang/regex#908):
    A new method, `Regex::static_captures_len`, has been added which returns the
    number of capture groups in the pattern if and only if every possible match
    always contains the same number of matching groups.
-   [FEATURE #&#8203;955](rust-lang/regex#955):
    Named captures can now be written as `(?<name>re)` in addition to
    `(?P<name>re)`.
-   FEATURE: `regex-syntax` now supports empty character classes.
-   FEATURE: `regex-syntax` now has an optional `std` feature. (This will come
    to `regex` in the second release.)
-   FEATURE: The `Hir` type in `regex-syntax` has had a number of simplifications
    made to it.
-   FEATURE: `regex-syntax` has support for a new `R` flag for enabling CRLF
    mode. This will be supported in `regex` proper in the second release.
-   FEATURE: `regex-syntax` now has proper support for "regex that never
    matches" via `Hir::fail()`.
-   FEATURE: The `hir::literal` module of `regex-syntax` has been completely
    re-worked. It now has more documentation, examples and advice.
-   FEATURE: The `allow_invalid_utf8` option in `regex-syntax` has been renamed
    to `utf8`, and the meaning of the boolean has been flipped.

Performance improvements:

-   PERF: The upgrade to `aho-corasick 1.0` may improve performance in some
    cases. It's difficult to characterize exactly which patterns this might impact,
    but if there are a small number of longish (>= 4 bytes) prefix literals, then
    it might be faster than before.

Bug fixes:

-   [BUG #&#8203;514](rust-lang/regex#514):
    Improve `Debug` impl for `Match` so that it doesn't show the entire haystack.
-   BUGS [#&#8203;516](rust-lang/regex#516),
    [#&#8203;731](rust-lang/regex#731):
    Fix a number of issues with printing `Hir` values as regex patterns.
-   [BUG #&#8203;610](rust-lang/regex#610):
    Add explicit example of `foo|bar` in the regex syntax docs.
-   [BUG #&#8203;625](rust-lang/regex#625):
    Clarify that `SetMatches::len` does not (regretably) refer to the number of
    matches in the set.
-   [BUG #&#8203;660](rust-lang/regex#660):
    Clarify "verbose mode" in regex syntax documentation.
-   BUG [#&#8203;738](rust-lang/regex#738),
    [#&#8203;950](rust-lang/regex#950):
    Fix `CaptureLocations::get` so that it never panics.
-   [BUG #&#8203;747](rust-lang/regex#747):
    Clarify documentation for `Regex::shortest_match`.
-   [BUG #&#8203;835](rust-lang/regex#835):
    Fix `\p{Sc}` so that it is equivalent to `\p{Currency_Symbol}`.
-   [BUG #&#8203;846](rust-lang/regex#846):
    Add more clarifying documentation to the `CompiledTooBig` error variant.
-   [BUG #&#8203;854](rust-lang/regex#854):
    Clarify that `regex::Regex` searches as if the haystack is a sequence of
    Unicode scalar values.
-   [BUG #&#8203;884](rust-lang/regex#884):
    Replace `__Nonexhaustive` variants with `#[non_exhaustive]` attribute.
-   [BUG #&#8203;893](rust-lang/regex#893):
    Optimize case folding since it can get quite slow in some pathological cases.
-   [BUG #&#8203;895](rust-lang/regex#895):
    Reject `(?-u:\W)` in `regex::Regex` APIs.
-   [BUG #&#8203;942](rust-lang/regex#942):
    Add a missing `void` keyword to indicate "no parameters" in C API.
-   [BUG #&#8203;965](rust-lang/regex#965):
    Fix `\p{Lc}` so that it is equivalent to `\p{Cased_Letter}`.
-   [BUG #&#8203;975](rust-lang/regex#975):
    Clarify documentation for `\pX` syntax.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS42MS4wIiwidXBkYXRlZEluVmVyIjoiMzUuNjYuMyIsInRhcmdldEJyYW5jaCI6ImRldmVsb3AifQ==-->

Co-authored-by: cabr2-bot <[email protected]>
Co-authored-by: crapStone <[email protected]>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1874
Reviewed-by: crapStone <[email protected]>
Co-authored-by: Calciumdibromid Bot <[email protected]>
Co-committed-by: Calciumdibromid Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants