FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

automata: fix bug in reverse suffix/inner optimization by BurntSushi · Pull Request #1364 · rust-lang/regex · GitHub

automata: fix bug in reverse suffix/inner optimization - #1364

Merged
BurntSushi merged 12 commits into
masterfrom
ag/fix-reverse-optimizations
Jul 15, 2026
Merged

BurntSushi merged 12 commits into
masterfrom
ag/fix-reverse-optimizations

Conversation

BurntSushi commented Jun 7, 2026
edited
Loading

Copy link
Copy Markdown
Member

A minimal reproducer of this bug is on a haystack of zabb with the
regex .bb|b. The regex crate will report a match at 2..3, but the
correct match is 1..4.

While this seems like a simple regex, there are a pretty specific set
of circumstances required to trigger the bug:

  1. There are no prefix literals that activate a standard prefix
    literal scan.
  2. There needs to be an extractable suffix or inner literal.
  3. An actual match needs to be present in the haystack.
  4. The regex and haystack need to be such that the first candidate
    (from a literal scan) corresponds to a match whose start offset
    is greater than the start offset of a match confirmed by a
    subsequent candidate.

Crucially, note that because of (3), this bug will never lead to
Regex::is_match providing a false positive or a false negative.
This bug is strictly about leftmost-first match semantics being
incorrect in some cases and will report an incorrect match span.

(4) could do with a bit more explanation, since it's rather subtle.
Let's trace the minimal example through the regex crate's "reverse
suffix" optimization.

During compilation, there is no prefix literal that can be extracted.
The . defeats that class of optimization. Moreover, there is a suffix
literal in the regex. That is, all matches for .bb|b must end with
b. The regex crate sees this and will scan for matches of b. It will
then attempt to match the regex in reverse at each candidate match of
b. Let's see what happens:

  • Find first occurrence of b at offset 2 in zabb.
  • Start reverse confirmation step at offset 2.
  • The second alternation branch, b in .bb|b, matches at 2..3.
  • The second alternation branch is reported as the overall match.
    This happens because the first alternation branch, .bb, does not
    have a match ending at offset 3.

The fundamental problem here is that there is an overlap between the
reverse automaton for confirming the match and the literal scan. Small
changes, even to the haystack, can result in the bug disappearing.
For example, with a haystack of zbb, the correct match of 0..3 is
reported. This occurs because there is a quadratic "trip wire" that
triggers in this case that causes the search to bail out and fall back
to a DFA without using any literal optimizations.

This bug also applies to the "reverse inner" optimization. This
can happen when the literal is extracted from inside the regex
as opposed to it being a suffix literal. For example, the regex
(?:..acbb|b)a(?:c|d) on the haystack xzbacbbac reported a match at
2..5, but the correct match is 1..9.

Note that #1355 technically fixes this problem and is much simpler, but
in so doing, makes the reverse suffix and inner optimizations completely
ineffective. In other words, it nearly disables them entirely.

This PR keeps them enabled, but does some work to determine whether the
regex pattern combined with the literals exatracted could ever lead to
(4) being true.

This does unfortunately cause some regressions. For example,
(.*?,){13}z no longer gets the reverse suffix optimization because it
is difficult to prove that reverse scan of (.*?,){13} will never lead
to a later match than the true leftmost-first match after finding an
occurrence of z.

Fixes #1354, Closes #1355

BurntSushi commented Jun 7, 2026
edited
Loading

Copy link
Copy Markdown
Member Author

I wrote this PR with an LLM, so this still needs careful review. From a glance, the solution here seems very over complicated. But it does pass tests and rebar benchmarks.

BurntSushi force-pushed the ag/fix-reverse-optimizations branch from a9c5543 to fcc4980 Compare June 7, 2026 18:24
BurntSushi force-pushed the ag/fix-reverse-optimizations branch from fcc4980 to 829e11b Compare June 16, 2026 12:41
BurntSushi force-pushed the ag/fix-reverse-optimizations branch 4 times, most recently from c7db680 to 95b71de Compare July 1, 2026 13:02
... so that tools like ripgrep and fd will search it.
We're going to use it in more places and it was already getting
pretty repetitive.
BurntSushi force-pushed the ag/fix-reverse-optimizations branch from 95b71de to 285d721 Compare July 11, 2026 09:25
I'm still liking the infrastructure I set up, but the logic is
pretty complicated and about to get even more complicated. So it's
wise I think to finally have tests confirming a specific strategy
for a particular pattern.

I had been "testing" this previously through benchmarks. If a pattern
was slower than expected, then it probably got the wrong strategy.
However, this is annoying to do in practice and has much longer
iteration times. Plus, the results require interpretation, which isn't
terribly accessible to most folks.
And also in one example in `regex-syntax`.

I'm not sure how this happened.
BurntSushi force-pushed the ag/fix-reverse-optimizations branch 9 times, most recently from 99556fc to 42dbcc1 Compare July 15, 2026 11:40
This matches what we do for the reverse inner optimization. And
indeed, this is required for correctness given how the optimizations
currently work.
Previously, we just returned the `Hir` prefix (the thing that needs to
be compiled into a reverse automaton) and the actual prefilter itself.

But in order to deal with #1354, we're going to want to do some analysis
on the actual literals returned. So we expose that information.
This doesn't have any behavior changes. It just reads a bit
more nicely.
Specifically, this bug surfaced because we are returning the `Core`
engine in more cases. Basically, we were being a bit too eager to run a
prefix literal scan when we already have a match in hand. When we do, we
should not re-run the prefix scan or else we might miss the match we
already have.
BurntSushi force-pushed the ag/fix-reverse-optimizations branch from 42dbcc1 to 4db9875 Compare July 15, 2026 13:03

Copy link
Copy Markdown
Member Author

I think this is probably good to go. I need to do another round of review, but I significantly simplified the original LLM fix for this. There will likely unfortunately be some performance regressions here, but I tried to claw back what I could.

BurntSushi marked this pull request as ready for review July 15, 2026 14:26
A minimal reproducer of this bug is on a haystack of `zabb` with the
regex `.bb|b`. The `regex` crate will report a match at `2..3`, but the
correct match is `1..4`.

While this seems like a simple regex, there are a pretty specific set
of circumstances required to trigger the bug:

1. There are no prefix literals that activate a standard prefix
   literal scan.
2. There needs to be an extractable *suffix* or *inner* literal.
3. An actual match needs to be present in the haystack.
4. The regex and haystack need to be such that the first candidate
   (from a literal scan) corresponds to a match whose start offset
   is greater than the start offset of a match confirmed by a
   subsequent candidate.

Crucially, note that because of (3), this bug will never lead to
`Regex::is_match` providing a false positive *or* a false negative.
This bug is strictly about leftmost-first match semantics being
incorrect in some cases and will report an incorrect match span.

(4) could do with a bit more explanation, since it's rather subtle.
Let's trace the minimal example through the regex crate's "reverse
suffix" optimization.

During compilation, there is no prefix literal that can be extracted.
The `.` defeats that class of optimization. Moreover, there is a suffix
literal in the regex. That is, all matches for `.bb|b` must end with
`b`. The regex crate sees this and will scan for matches of `b`. It will
then attempt to match the regex in reverse at each candidate match of
`b`. Let's see what happens:

* Find first occurrence of `b` at offset `2` in `zabb`.
* Start reverse confirmation step at offset `2`.
* The second alternation branch, `b` in `.bb|b`, matches at `2..3`.
* The second alternation branch is reported as the overall match.
  This happens because the first alternation branch, `.bb`, does _not_
  have a match ending at offset `3`.

The fundamental problem here is that there is an overlap between the
reverse automaton for confirming the match and the literal scan. Small
changes, even to the haystack, can result in the bug disappearing.
For example, with a haystack of `zbb`, the correct match of `0..3` is
reported. This occurs because there is a quadratic "trip wire" that
triggers in this case that causes the search to bail out and fall back
to a DFA without using any literal optimizations.

This bug also applies to the "reverse inner" optimization. This
can happen when the literal is extracted from inside the regex
as opposed to it being a suffix literal. For example, the regex
`(?:..acbb|b)a(?:c|d)` on the haystack `xzbacbbac` reported a match at
`2..5`, but the correct match is `1..9`.

Note that #1355 technically fixes this problem and is much simpler, but
in so doing, makes the reverse suffix and inner optimizations completely
ineffective. In other words, it nearly disables them entirely.

This PR keeps them enabled, but does some work to determine whether the
regex pattern combined with the literals exatracted could ever lead to
(4) being true.

This does unfortunately cause some regressions. For example,
`(.*?,){13}z` no longer gets the reverse suffix optimization because it
is difficult to prove that reverse scan of `(.*?,){13}` will never lead
to a later match than the true leftmost-first match after finding an
occurrence of `z`.

Fixes #1354, Closes #1355
BurntSushi force-pushed the ag/fix-reverse-optimizations branch from 4db9875 to e8a82e4 Compare July 15, 2026 18:34

Copy link
Copy Markdown
Member Author

I'm doing one last run of the rebar benchmark suite first.

BurntSushi merged commit 75fcb96 into master Jul 15, 2026
19 checks passed

Copy link
Copy Markdown
Member Author

rebar benchmarks diffed with current master:

$ rebar diff x/master.csv x/change07.csv -t 1.2
benchmark                                            engine      x/master.csv        x/change07.csv
---------                                            ------      ------------        --------------
curated/06-cloud-flare-redos/original                rust/regex  566.9 MB/s (1.00x)  314.9 MB/s (1.80x)
curated/06-cloud-flare-redos/simplified-long         rust/regex  84.7 GB/s (1.00x)   42.9 GB/s (1.97x)
imported/leipzig/word-ending-nn                      rust/regex  29.2 GB/s (1.00x)   825.5 MB/s (36.19x)
imported/leipzig/tom-sawyer-huckle-fin-prefix-short  rust/regex  19.2 GB/s (1.00x)   823.7 MB/s (23.88x)
imported/leipzig/tom-sawyer-huckle-fin-prefix-long   rust/regex  18.4 GB/s (1.00x)   823.7 MB/s (22.90x)
imported/leipzig/ing                                 rust/regex  3.1 GB/s (1.00x)    765.5 MB/s (4.10x)
imported/leipzig/bounded-strings-ending-z            rust/regex  49.0 GB/s (1.00x)   824.2 MB/s (60.94x)
imported/rsc/reallyhard0-1k                          rust/regex  10.9 GB/s (1.00x)   794.7 MB/s (14.00x)
imported/rsc/reallyhard0-32k                         rust/regex  45.4 GB/s (1.00x)   825.6 MB/s (56.37x)
imported/rsc/reallyhard0-1mb                         rust/regex  44.0 GB/s (1.00x)   826.5 MB/s (54.46x)
imported/sherlock/word-ending-n                      rust/regex  835.8 MB/s (1.00x)  545.6 MB/s (1.53x)
imported/sherlock/ing-suffix                         rust/regex  3.5 GB/s (1.00x)    694.6 MB/s (5.19x)
opt/reverse-inner/factored-prefix                    rust/regex  9.1 GB/s (1.00x)    803.5 MB/s (11.64x)

There ight be a way to claw some of these back, but it's hard and I've already been working on this bug fix for over a month now (off-and-on). So I wanted to ship a correctness fix before doing more work here.

BurntSushi deleted the ag/fix-reverse-optimizations branch July 15, 2026 20:15
UneBaguette pushed a commit to vexahub/voprf-vx that referenced this pull request Aug 5, 2026
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [displaydoc](https://github.com/yaahc/displaydoc) | dependencies | patch | `0.2.6` → `0.2.7` |
| [hybrid-array](https://github.com/RustCrypto/hybrid-array) | dependencies | patch | `0.4.13` → `0.4.14` |
| [regex](https://github.com/rust-lang/regex) | dev-dependencies | patch | `1.13.0` → `1.13.1` |
| [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | dependencies | patch | `1.0.228` → `1.0.229` |
| [serde_json](https://github.com/serde-rs/json) | dev-dependencies | patch | `1.0.150` → `1.0.151` |

---

### Release Notes

<details>
<summary>yaahc/displaydoc (displaydoc)</summary>

### [`v0.2.7`](https://github.com/yaahc/displaydoc/blob/HEAD/CHANGELOG.md#027---2026-07-28)

[Compare Source](yaahc/displaydoc@v0.2.6...v0.2.7)

#### Added

- Updated `syn` dependency to 3.0

#### Changed

- Bumped MSRV to 1.71

</details>

<details>
<summary>RustCrypto/hybrid-array (hybrid-array)</summary>

### [`v0.4.14`](https://github.com/RustCrypto/hybrid-array/blob/HEAD/CHANGELOG.md#0414-2026-07-30)

[Compare Source](RustCrypto/hybrid-array@v0.4.13...v0.4.14)

##### Added

- Sizes for `sntrup` ([#&#8203;231])

[#&#8203;231]: RustCrypto/hybrid-array#231

</details>

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

### [`v1.13.1`](https://github.com/rust-lang/regex/blob/HEAD/CHANGELOG.md#1131-2026-07-15)

[Compare Source](rust-lang/regex@1.13.0...1.13.1)

\===================
This is a release that fixes a bug where incorrect regex match offsets could be
reported. Note that this doesn't impact whether a match occurs or not, just
where it occurs. The match offsets are still valid for slicing, they just may
not refer to the correct leftmost-first match. See
[#&#8203;1364](rust-lang/regex#1364) for (many) more details.

Bug fixes:

- [#&#8203;1354](rust-lang/regex#1354):
  Fixes previously unsound reverse suffix and inner optimizations.

</details>

<details>
<summary>serde-rs/serde (serde)</summary>

### [`v1.0.229`](https://github.com/serde-rs/serde/releases/tag/v1.0.229)

[Compare Source](serde-rs/serde@v1.0.228...v1.0.229)

- Update to syn 3

</details>

<details>
<summary>serde-rs/json (serde_json)</summary>

### [`v1.0.151`](https://github.com/serde-rs/json/releases/tag/v1.0.151)

[Compare Source](serde-rs/json@v1.0.150...v1.0.151)

- Add RawValue::from\_string\_unchecked ([#&#8203;1331](serde-rs/json#1331), thanks [@&#8203;WonderLawrence](https://github.com/WonderLawrence))

</details>

---

### Configuration

📅 **Schedule**: (UTC)

- 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.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

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

---

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

Reviewed-on: https://dev.unebaguette.fr/vexahub/voprf-vx/pulls/16
Co-authored-by: Renovate Bot <renovatebot@lunomail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reverse suffix doesn't return the leftmost match when alternates overlap

1 participant


Back | FazBrowse Home | New Git URL