guard end-before-start match length in pcre2grep -o and --output - #954
Conversation
NWilson
left a comment
NWilson
left a comment
There was a problem hiding this comment.
This is great, thank you!
However... we do have a convention already in a couple of other places in pcre2grep, where these pathological "reversed matches" are just printed as [min(start,end) ... max(start,end)].
Would it be OK to swap these two you found to behave the same, just for consistency?
I also asked GPT to check for others, and it found some. It should be quick & easy (hopefully!) to make the same change in each place.
There was a problem hiding this comment.
Here's one needing the same treatment.
Create local variables, swap if not ordered.
There was a problem hiding this comment.
Done, start/end locals with a swap, shared by both offset options.
There was a problem hiding this comment.
Covered by the same block as --line-offsets now.
There was a problem hiding this comment.
This should be testing which is larger, and doing the reversed subtraction if they are not ordered.
There was a problem hiding this comment.
Done, reversed subtraction when end < start so it agrees with the memcpy pass below.
There was a problem hiding this comment.
Swapped here too.
…oing Follow the existing convention used by the colour and multiline code: when \K leaves the end offset before the start, print the range [min, max] rather than treating it as empty. Apply the same swap to --line-offsets, --file-offsets, and the callout argument length and copy, which had the same unsigned subtraction.
|
Makes sense, switched both to the swap convention and did the same in the four spots you marked (--line-offsets, --file-offsets, and the callout argslen/memcpy pair). For the argslen one I kept it as a conditional subtraction since it's only a length, the rest use start/end locals with the same swap block as the colour path. Test 161 now expects "foo" and covers --line-offsets / --file-offsets too. RunGrepTest is green with and without ASAN. |
There was a problem hiding this comment.
Pull request overview
Prevents unsigned length underflow when \K produces reversed match offsets in pcre2grep.
Changes:
- Normalizes reversed capture and match offsets before output.
- Covers
-o,--output, callouts, and offset-reporting modes. - Adds regression test 161 and expected output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/pcre2grep.c |
Safely handles end-before-start offsets. |
RunGrepTest |
Adds regression commands for affected modes. |
testdata/grepoutput |
Records expected regression output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
NWilson
left a comment
NWilson
left a comment
There was a problem hiding this comment.
Amazing, thank you very much!
|
I pushed a small commit adding the matching update |
Length underflow on an end-before-start match in pcre2grep
With
--allow-lookaround-bsk, a \K inside a lookaround can leave a match whose end offset is before its start ((?=foo\K)onXfooYgives ovector 4..1). The -o and --output paths take the printed length as an unsignedend - start, which wraps near SIZE_MAX and runs print_match/fwrite off the end of the line buffer. The colour and multiline paths already swap for this exact case; these two did not. Guarded both so a reversed match counts as empty, which is what the existingplen > 0/capturesize > 0tests were already after.Repro under ASAN (small --buffer-size to land on the redzone):
Both fault in the length-based write (pcre2grep.c:2889 for -o, :2216 for --output) before the patch and run clean after. Added test 161 to RunGrepTest.