pcre2_jit_compile: avoid potential wraparound if framesize == 0

[carenas]

Additional code would be generated when framesize == 0 that would be dangerous so exempt it and add an assert to catch any situations that could trigger it in the future.

[carenas]

well, that was embarrasing, and indeed it is being triggered in the newly added assert:

frame #5: 0x00000001001916dc pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf50, current=0x00006210026d8c18) at pcre2_jit_compile.c:13893:5
   13890        {
   13891        SLJIT_ASSERT(opcode == OP_COND || opcode == OP_SCOND);
   13892        assert = CURRENT_AS(bracket_backtrack)->u.assert;
-> 13893        SLJIT_ASSERT(assert->framesize != 0);
   13894        if ((ccbegin[1 + LINK_SIZE] == OP_ASSERT_NOT || ccbegin[1 + LINK_SIZE] == OP_ASSERTBACK_NOT) && assert->framesize >= 0)
   13895          {
   13896          OP1(SLJIT_MOV, STACK_TOP, 0, SLJIT_MEM1(SLJIT_SP), assert->private_data_ptr);
p *assert
(assert_backtrack) {
  common = {
    prev = 0x00006210026d8cc0
    simple_backtracks = NULL
    top = 0xbebebebebebebebe
    own_backtracks = 0xbebebebebebebebe
    cc = 0x0000000000000000
  }
  condfailed = NULL
  framesize = 0
  private_data_ptr = 0
  matchingpath = NULL
}

[zherczeg]

I don't remember but the code has special meaning for framesize < 0. What is the pattern?

[zherczeg]

Framesizes usually come form here: https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L2047

The end is also interesting: https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L2231 Looks like it does not really return with 0.

[carenas]

What is the pattern?

/^(?: 
(?: A| (1? (?=(?<cond>2)?) (1) 2 (?('cond')|3)) )
(Z)
)+$/x

but it can be replicated without PCRE2_EXTENDED as well, it seems the first time we hit this code we probably have a bogus current:

* frame #0: 0x0000000100191873 pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf90, current=0x000062100002a418) at pcre2_jit_compile.c:13893:10
    frame #1: 0x000000010010c2b0 pcre2test`compile_backtrackingpath(common=0x00007ff7bfefcf90, current=0x000062100002a418) at pcre2_jit_compile.c:14347:5
    frame #2: 0x000000010018fce1 pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029d50) at pcre2_jit_compile.c:13697:1
    frame #3: 0x000000010010c2b0 pcre2test`compile_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029d50) at pcre2_jit_compile.c:14347:5
    frame #4: 0x00000001001915ef pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029850) at pcre2_jit_compile.c:13882:5
    frame #5: 0x000000010010c2b0 pcre2test`compile_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029850) at pcre2_jit_compile.c:14347:5
    frame #6: 0x000000010018fce1 pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029790) at pcre2_jit_compile.c:13697:1
    frame #7: 0x000000010010c2b0 pcre2test`compile_backtrackingpath(common=0x00007ff7bfefcf90, current=0x0000621000029790) at pcre2_jit_compile.c:14347:5
    frame #8: 0x000000010018fce1 pcre2test`compile_bracket_backtrackingpath(common=0x00007ff7bfefcf90, current=0x00006210000296d8) at pcre2_jit_compile.c:13697:1
    frame #9: 0x000000010010c2b0 pcre2test`compile_backtrackingpath(common=0x00007ff7bfefcf90, current=0x00006210000296d8) at pcre2_jit_compile.c:14347:5
    frame #10: 0x00000001000ee027 pcre2test`jit_compile(code=0x0000611000000680, mode=1) at pcre2_jit_compile.c:15019:1

[zherczeg]

Btw, 13893 SLJIT_ASSERT(assert->framesize != 0); this is not in the patch.

The code is not wrong, but not nice. The assert contains a random value, if the condition is not an assert, but its never resolved if the left side of the logical and fails (short circuit evaluation). The if could be split into two ifs, and read the assert before the second.

[carenas]

I don't think I am fully understanding your comment, but if you were suggesting the original assert that was removed should had been doing instead:

SLJIT_ASSERT((ccbegin[1 + LINK_SIZE] == OP_ASSERT_NOT || ccbegin[1 + LINK_SIZE] == OP_ASSERTBACK_NOT) && assert->framesize != 0);

I agree with you, but sadly, that will also crash for the same reason; not sure why the normal code does not, neither how we get some random values in the assert variable that happen to have a value of 0 for its framesize.

[zherczeg]

if (ccbegin[1 + LINK_SIZE] == OP_ASSERT_NOT || ccbegin[1 + LINK_SIZE] == OP_ASSERTBACK_NOT)
  {
  assert = ;
  ASSERT(...);
  if (...) {}
  }

The assert is loaded from a union, and may contain a completely invalid pointer. Reading it can be dangerous.

[carenas]

Updated patch, including only a few of the asserts since they are mostly no longer needed.

[zherczeg]

So <= 1 is invalid?

[carenas]

Not sure I understand the context for your question, but no, there are at least two negative values of framesize that are valid, eventhough they were AFAIK never supported by the code I am changing.

[zherczeg]

The code looks ok