Repository navigation
strings/replace: +fix Symbol-outside-alphabet dereference and end_marker_dfa/marker_nft mutate-while-iterating - #878
Merged
Conversation
Adda0
marked this pull request as ready for review
October 16, 2026 05:54
 [Medium impact] The PR appears safe to merge; no actionable issues were found.
|
…ker_dfa/marker_nft mutate-while-iterating create_identity_with_single_symbol_replace() dereferences end() when the symbol to replace is not in the alphabet, causing a SIGSEGV. The function checked only for empty() alphabet but not for the absence of the symbol itself. ReluctantReplace::end_marker_dfa() inserts into and erases from regex.final while iterating over it, causing the SparseSet's dense buffer to reallocate mid-iteration. With 2-3 final states this silently corrupts the result, and with 4+ states it aborts with std::length_error: vector::_M_default_append. ReluctantReplace::marker_nft() erases and inserts targets in a SymbolPost while iterating via moves_epsilons(), also moving dangling pointers. All three were caught in #764 test cases: - symbol 'z' outside alphabet {'a', 'b'}: std::bad_alloc before fix - chain DFA 0 -a-> 1 ... -a-> n with all states final, n >= 3: vector::_M_default_append exception before fix - 0 -ε-> {1, 2} with marker_nft: marker_state < 3 and empty post before fix BEFORE (Release build): - symbol outside alphabet: std::bad_alloc - end_marker_dfa n=3: terminate with vector::_M_default_append - marker_nft two targets: marker_state=2 (expected >=3), empty post AFTER: - symbol outside alphabet: throws std::runtime_error("Alphabet does not contain...") - end_marker_dfa n=1..40: final count preserved, language equivalent - marker_nft: both targets get exactly one marker hop Fixes #764.
Adda0
force-pushed
the
issue-764-replace-fixes-pr
branch
from
October 16, 2026 06:09
40c93af to
4bf2e2a
Compare
Collaborator
Author
|
Rebased onto devel and reformatted before merge: resolved the conflict in |
Adda0
added a commit
that referenced
this pull request
Oct 16, 2026
…luctant replace without dead states (#943) Segmentation kept every ε-transition twice: in an unordered_map of vectors keyed by depth and in a nested unordered_map keyed by depth and source. Each state is expanded once and all its ε-moves are handled in a row, so its ε-transitions are contiguous at a single depth. They now live once in a vector indexed by depth, with a per-state span into it. get_epsilon_transitions_by_depth() and get_epsilon_transitions(state) expose them without copies; split_aut_into_segments(), noodlify() and noodlify_mult_eps() use them. get_epsilon_depths() keeps its type for the Python binding and is built on its first call, safely for concurrent const calls; get_epsilon_depth_trans_map() is built the same way and deprecated. Segmentation stays copy- and move-constructible: a copy carries the ε-transition index and builds its own views on demand. noodlify_mult_eps() probed segments_one_initial_final with find({target, final}) for every final state of the next segment after every ε-transition. The ordered map keeps the pairs of one initial state contiguous, so one lower_bound scan finds exactly the connected ones. The hits are visited in the order of the segment's final states, so the noodle order is unchanged. This also answers the "should we use unordered_map?" TODO: an unordered map would lose that contiguity. reluctant_nfa_with_marker() added a marker self-loop to every state, intersected the result with a two-state guard NFA and reduced it. The copy of a final state behind a marker was a dead lasso that ended up in the NFT. In the reduced product, the after-marker copy of a non-final state merges with its letter copy and the one behind a final state is dead. The function now builds this result directly: the part of the NFA reachable over alphabet letters, with final states without successors and a marker self-loop on every non-final state. Symbols outside the alphabet are still dropped. reluctant_leftmost_nft() ended the replacement with three or four ε-transitions: insert_word_by_levels() created a fresh level-1 target, then an ε to a level-0 state that was inserted into and cleared from the final states, and for ReplaceMode::All two more ε-steps back to the initial state. The last replacement letter (or the single ε for an empty replacement) now goes straight to the initial state, or to the final state for ReplaceMode::Single. For the example from #568 (replace 'a' by 'f' over {a, f}), reluctant_leftmost_nft() drops from 15 to 9 states and has no isolated states, and replace_reluctant_regex() drops from 20 to 12 states. begin_marker_nfa() reverted the marker DFA and then swapped its initial and final states back, with a TODO asking for a flag on revert(). An internal revert_delta() helper now names that step; the public revert() is unchanged, as no other caller needs the flag. Segmentation::get_segments() trims each raw segment with nfa::trim() instead of trimming a copy in place (#745, item 1). The tests of reluctant_leftmost_nft() compare against hard-coded NFTs with nft::are_equivalent(), which reads ε as a letter. Their expected NFTs are regenerated. Fixes #795. Fixes #568. Part of #745. ## Measurements Throwaway harness linked against `build/release/g++/src/libmata.a` of this commit and of `devel` (b6f7fd1), g++ `-O3 -DNDEBUG`. The machine was loaded (load average ~23 on 16 cores), so user-space instruction counts (`perf stat -e instructions:u`, deterministic) are the primary figure; wall clock is the median of 9 alternating base/new pairs on one pinned core. Workloads: - `replace`: `replace_reluctant_regex()` for 11 regexes (`a`, `ab`, `a+b`, `(a|b)c*`, `a*`, `(ab|ba)+c?`, `a(b|c)*a`, `(a|b)*c(a|b)`, `a+b+c`, `cb+a+`, `(a|b|c)*ab`) × replacements `''`, `d`, `ddd` × both modes. - `seg`: `Segmentation` + `get_segments()`. - `noodle`: segment automaton with 4 segments of 40 states, 12 ε-sources per depth (~23k noodles). - `wide`: 3 segments of 800 states, 400 ε-sources per depth, letters only inside blocks of 4 states, so most pair probes miss. | workload | devel instr. | this PR instr. | change | |---|---:|---:|---:| | replace ×20 | 554,189,026 | 435,911,013 | −21.3 % | | noodle seg ×200 | 458,281,680 | 332,013,904 | −27.6 % | | noodle `noodlify_mult_eps` ×4 | 508,649,431 | 509,477,078 | +0.2 % | | noodle `noodlify` ×4 | 331,395,567 | 325,482,021 | −1.8 % | | wide seg ×20 | 415,027,966 | 298,211,326 | −28.1 % | | wide `noodlify_mult_eps` ×1 | 336,822,446 | 311,681,463 | −7.5 % | | wide `noodlify` ×1 | 585,575,482 | 535,628,445 | −8.5 % | | workload (wall clock) | devel median | this PR median | median ratio | |---|---:|---:|---:| | replace ×40 | 144.1 ms | 112.2 ms | 0.80 | | noodle seg ×400 | 107.1 ms | 71.2 ms | 0.67 | | wide `noodlify_mult_eps` ×2 | 82.1 ms | 70.0 ms | 0.89 | The range scan in `noodlify_mult_eps()` gains only when the next segment has many final states that a given ε-target does not reach (`wide`); with few final states (`noodle`) it is flat. Most of the time in noodlification remains in `segs_one_initial_final()`, which this PR does not touch. Equivalence checks with the same harness, base vs this PR: - noodles of `noodlify_mult_eps()` and `noodlify()` and the segments of `get_segments()` on both generated automata: byte-identical order-sensitive digests (segment state/transition counts and ε counters per noodle, in output order; segments printed in full). - `reluctant_leftmost_nft()` for all regex × replacement × mode combinations: identical images (ε removed) of every input word up to length 5 over `{a, b, c, BEGIN_MARKER}`; `replace_reluctant_regex()`: identical images of every word up to length 6 over `{a, b, c}`; `reluctant_nfa_with_marker()`: identical word sets up to length 6. 66 NFTs, 6,762 distinct output words. State counts of `replace_reluctant_regex()` drop on every case, e.g. `a` → `d` (Single) 24 → 20, (All) 21 → 13; `ab` → `ddd` (All) 43 → 31. ## Tests - `reluctant_nfa_with_marker()`: no dead marker state (state counts, trim-stable, deterministic, marker accepted inside but not after a match); symbols outside the alphabet are still dropped. Fails on devel (`3 == 2`, `4 == 3`, `5 == 4`; trim shrinks the result). - `reluctant_leftmost_nft()`: the #568 example has 9 states and is trim-stable (devel: 15, trim 13). Expected NFTs of the three existing sections regenerated (`nft::are_equivalent()` reads ε as a letter); semantic equality with the old NFTs is checked above. - `Segmentation::get_epsilon_transitions()`: per-depth vector, per-state views over several ε symbols, states without ε-transitions and out of range. Fails when the view is truncated. - `Segmentation` copies and moves: copy- and move-constructible (static_assert), and a copy or move made before or after the lazy getters ran, queried after the source is destroyed, returns the same `get_epsilon_depths()`, `get_epsilon_transitions()` and segments. Fails to compile when the lazy views are not copyable. - The existing index-compared noodle tests catch a changed noodle order (checked by reversing the sort). `just tests-cpp::check` on devel ba4a338 (debug and release): `test cases: 330 | 324 passed | 2 skipped | 4 failed as expected` both times. `just bindings::python::check`: `107 passed, 3 skipped, 2 xfailed`. (Measurements above were taken on devel b6f7fd1; the commits since then do not touch the strings code.) ## Overlap with open PRs - #878 (draft) edits `end_marker_dfa()` / `marker_nft()` in `replace.cc` and appends tests at the end of `tests/applications/strings/replace.cc`. This PR does not touch those functions and adds its tests next to the `reluctant_*` test cases, so no textual conflict is expected. - #939 changes the `unused_state` line in `noodlify()`/`noodlify_mult_eps()` and the `curr_state` line in `reluctant_leftmost_nft()`; both are adjacent to lines changed here, so whichever lands second needs a trivial rebase. - #910 changes `determinize()` calls in `replace.cc` elsewhere; #872 changes `noodlify_for_equation()`; no overlap. - #880 (draft) touches the Python `strings.pyx`; `get_epsilon_depths()` keeps its type here, so the binding is unaffected.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
create_identity_with_single_symbol_replace() dereferences end() when the symbol
to replace is not in the alphabet, causing a SIGSEGV. The function checked only
for empty() alphabet but not for the absence of the symbol itself.
ReluctantReplace::end_marker_dfa() inserts into and erases from regex.final
while iterating over it, causing the SparseSet's dense buffer to reallocate
mid-iteration. With 2-3 final states this silently corrupts the result, and with
4+ states it aborts with std::length_error: vector::_M_default_append.
ReluctantReplace::marker_nft() erases and inserts targets in a SymbolPost
while iterating via moves_epsilons(), also moving dangling pointers.
All three were caught in #764 test cases:
vector::_M_default_append exception before fix
BEFORE (Release build):
AFTER: