Repository navigation
Conversation
decode_number() accumulated digits in an unsigned long without checking for overflow, and the callers then narrowed the result to int, uint32_t or size_t. Large values wrapped silently: - -o and --only-matching store the group number as an int, so 3000000000 became negative. The check against capture_max and the later check against the match result both pass for a negative number, and offsets[2 * n] then reads outside the ovector. This crashed with an access violation on Windows. - --buffer-size is multiplied by 3 for the main buffer. A value above PCRE2_SIZE_MAX / 3 made that size wrap, leaving a tiny buffer while the code still assumed bufthird bytes per third. - --om-capture above INT_MAX made every -o request fail, because the limit was compared as an int. UINT32_MAX also wrapped offset_size to 0. Give each option a limit based on the variable it is stored in, and fail with a new "Out-of-range number" message, like the existing "Malformed number" one. decode_number() now uses uint64_t, so the result is the same on LP64 and LLP64 platforms. Test 162 was added to RunGrepTest and RunGrepTest.py.
| break; | ||
|
|
||
| case OP_SIZE: | ||
| limit = PCRE2_SIZE_MAX / 3; |
There was a problem hiding this comment.
Hardcoding the /3 here is fragile.
Please use the full/correct upper bound here; but we need an overflow check later on, when we actually work with the input value and scale it up.
| if (op->type == OP_OP_NUMBERS) | ||
| { | ||
| unsigned long int n = decode_number(option_data, op, longop); | ||
| uint64_t n = decode_number(option_data, op, longop); |
There was a problem hiding this comment.
These casts could be neater perhaps as int n = (int)decode_number(...). If we know that the validated size is an int, let's express that early.
| /* Set up default compile and match contexts and match data blocks. */ | ||
|
|
||
| offset_size = capture_max + 1; | ||
| offset_size = (capture_max == UINT32_MAX) ? capture_max : capture_max + 1; |
There was a problem hiding this comment.
Why is this correct? Clearly the overflow is a problem. But do we need to reject the value, rather than skip the +1?
What do you think?
Let --buffer-size and --max-buffer-size use the full range of PCRE2_SIZE in decode_number(), and check 3 * bufthird before allocating the main buffer. max_bufthird is clamped, since it is only a limit for growing the buffer. Cast the result of decode_number() where it is read.
|
Thanks! Pushed the changes:
On
|
decode_number()accumulated digits without an overflow check and callers narrowed the result, so-o 3000000000became a negative group number that read outside the ovector (access violation on Windows), and a large--buffer-sizewrapped3 * bufthird. Each option now has a limit based on its variable, with a new "Out-of-range number" error. Test 162 is added to both harnesses.Verified:
pcre2_grep_teston Windows (clang, CMake),RunGrepTestandRunTeston Linux with a zig cc UBSan build fromconfig.h.generic. Test 162 fails before the fix and passes after it.Not available: the devcontainer presets (debug, asan-ubsan, coverage, gcc), Valgrind, ManyConfigTests, MSVC, 32-bit, EBCDIC, autotools. On Windows, RunTest test 8 fails in my debug build, also on the clean base; it passes on Linux. The shell
RunGrepTestneeds Linux; under Git Bash it differs by CRLF.