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

setting heap_limit could be truncated since PCRE2 10.41 #183

Closed
carenas opened this issue Dec 29, 2022 · 0 comments
Closed

setting heap_limit could be truncated since PCRE2 10.41 #183

carenas opened this issue Dec 29, 2022 · 0 comments

Comments

@carenas
Copy link
Contributor

carenas commented Dec 29, 2022

After d90fb23 (Refactor match_data() to always use the heap instead of having an initial frames vector on the stack..., 2022-07-27) the value for mb->heap_limit (using a PCRE2_SIZE) is set to the number of bytes, instead of KB (as used everywhere else).

The problem with doing that is that the variable that holds this in the match/expression context is only 32bit wide and therefore setting the value itself requires multiplying that value by 1024 and that can overflow.

Changing the code to widen the multiplication itself helps when PCRE2_SIZE is 64bit but it won't in 32bit systems where PCRE2_SIZE isn't just wide enough:

diff --git a/src/pcre2_match.c b/src/pcre2_match.c
index 168b9fa..7e65985 100644
--- a/src/pcre2_match.c
+++ b/src/pcre2_match.c
@@ -6816,7 +6816,7 @@ frame_size = (offsetof(heapframe, ovector) +
 smaller. */
 
 mb->heap_limit = ((mcontext->heap_limit < re->limit_heap)?
-  mcontext->heap_limit : re->limit_heap) * 1024;
+  mcontext->heap_limit : re->limit_heap) * 1024ul;
 
 mb->match_limit = (mcontext->match_limit < re->limit_match)?
   mcontext->match_limit : re->limit_match;

Additionally, since the default for heap_limit is set to 20000000KB, it is already larger (when converted into bytes) than what a 32bit PCRE2_SIZE can hold, and while that specific issue was already present in previous code, meaning that the limit was practically unreachable, it was at least representable and useful for evaluation.

@carenas carenas changed the title setting heap_limit could overflow since PCRE2 10.41 setting heap_limit could be truncated since PCRE2 10.41 Dec 30, 2022
carenas added a commit to carenas/pcre2 that referenced this issue Jan 1, 2023
If a LIMIT_HEAP value once converted to bytes is larger than UINT_MAX
would result in a bogus setting that could trigger a matching failure
as shown by the following:

  PCRE2 version 10.42 2022-12-11
    re> /(*LIMIT_HEAP=4194304)a/
  data> a
  Failed: error -63: heap limit exceeded

Remove the multiplication and instead keep track of the maximum heap
allowed in KB as was done originally.

Aditionally, add a check to avoid overflowing a PCRE2_SIZE while
doubling the heap used and that could result in a crash (mostly on
systems with a 32-bit PCRE2_SIZE with non standard settings).

Fixes: d90fb23 (Refactor match_data() to always use the heap instead
       of having an initial frames vector on the stack..., 2022-07-27)
Closes: PCRE2Project#183
carenas added a commit to carenas/pcre2 that referenced this issue Jan 10, 2023
If a LIMIT_HEAP value once converted to bytes is larger than UINT_MAX
would result in a bogus valueg that could trigger a matching failure
as shown by the following:

  PCRE2 version 10.42 2022-12-11
    re> /(*LIMIT_HEAP=4194304)a/
  data> a
  Failed: error -63: heap limit exceeded

Remove the multiplication and instead keep track of the maximum heap
allowed in KB as was done originally.

Aditionally, add a check to avoid overflowing a PCRE2_SIZE while
doubling the heap used and that could result in a crash (only on
systems with a 32-bit PCRE2_SIZE and using non standard settings).

Unlike the original, this code avoids rounding the heapframes_size
to the frame_size at the allocation time, which simplifies the logic
and wasn't really needed.

Fixes: d90fb23 (Refactor match_data() to always use the heap instead
       of having an initial frames vector on the stack..., 2022-07-27)
Closes: PCRE2Project#183
carenas added a commit to carenas/pcre2 that referenced this issue Jan 14, 2023
If a LIMIT_HEAP value once converted to bytes is larger than UINT_MAX
would result in a bogus setting that could trigger a matching failure
as shown by the following:

  PCRE2 version 10.42 2022-12-11
    re> /(*LIMIT_HEAP=4194304)a/
  data> a
  Failed: error -63: heap limit exceeded

Remove the multiplication and instead keep track of the maximum heap
allowed in KB as was done originally.

Aditionally, add a check to avoid overflowing a PCRE2_SIZE while
doubling the heap used and that could result in a crash (only on
systems with a 32-bit PCRE2_SIZE and using non standard settings).

Unlike the original, this code avoids rounding the heapframes_size
to the frame_size at the allocation time, which simplifies the logic
and wasn't really needed.

Fixes: d90fb23 (Refactor match_data() to always use the heap instead
       of having an initial frames vector on the stack..., 2022-07-27)
Closes: PCRE2Project#183
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

No branches or pull requests

1 participant