Add testing for malloc() failures #697
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8943,6 +8943,7 @@ while (1) | |
|
|
||
| has_vreverse = (*ccbegin == OP_VREVERSE); | ||
| if (*ccbegin == OP_REVERSE || has_vreverse) | ||
| // XXX what if error? | ||
| ccbegin = compile_reverse_matchingpath(common, ccbegin, &altbacktrack); | ||
|
|
||
| compile_matchingpath(common, ccbegin, cc, &altbacktrack); | ||
|
|
@@ -9714,6 +9715,7 @@ else if (opcode == OP_ASSERTBACK_NA && PRIVATE_DATA(ccbegin + 1)) | |
|
|
||
| has_vreverse = (*matchingpath == OP_VREVERSE); | ||
| if (*matchingpath == OP_REVERSE || has_vreverse) | ||
| // XXX what if error? | ||
| matchingpath = compile_reverse_matchingpath(common, matchingpath, backtrack); | ||
| } | ||
| else if (opcode == OP_ASSERT_NA || opcode == OP_ASSERTBACK_NA || opcode == OP_SCRIPT_RUN || opcode == OP_SBRA || opcode == OP_SCOND) | ||
|
|
@@ -9725,6 +9727,7 @@ else if (opcode == OP_ASSERT_NA || opcode == OP_ASSERTBACK_NA || opcode == OP_SC | |
| OP1(SLJIT_MOV, SLJIT_MEM1(STACK_TOP), STACK(0), TMP2, 0); | ||
|
|
||
| if (*matchingpath == OP_REVERSE) | ||
| // XXX what if error? | ||
| matchingpath = compile_reverse_matchingpath(common, matchingpath, backtrack); | ||
| } | ||
| else if (opcode == OP_ASSERT_SCS) | ||
|
|
@@ -12694,6 +12697,8 @@ if (current->cc[1] > OP_ASSERTBACK_NOT) | |
| { | ||
| /* Manual call of compile_bracket_matchingpath and compile_bracket_backtrackingpath. */ | ||
| compile_bracket_matchingpath(common, current->cc, current); | ||
| if (SLJIT_UNLIKELY(sljit_get_compiler_error(common->compiler))) | ||
| return; | ||
|
Comment on lines
+12699
to
+12700
Member
Author
There was a problem hiding this comment. This is a segfault fix! If malloc fails inside compile_bracket_matchingpath, then I have triaged this for security according to this logic: on modern platforms, it's very hard to get malloc to fail (more likely for the OS to allocate memory from swap if needed, and then terminate processes if that runs out). And, if the malloc call does fail, it sets a nice clean NULL which will give a deterministic segfault, rather than some bad access to the heap. This is therefore only a minor, hard-to-exploit, denial of service. |
||
| compile_bracket_backtrackingpath(common, current->top); | ||
| } | ||
| else | ||
|
|
@@ -12979,6 +12984,7 @@ while (current) | |
|
|
||
| case OP_BRAMINZERO: | ||
| compile_braminzero_backtrackingpath(common, current); | ||
| // XXX what if error? | ||
|
||
| break; | ||
|
|
||
| case OP_MARK: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice patch!
Did you checked this by returning a NULL?
The general concept is, that if an error occurs during JIT compilation, the error is set in the compiler, and all emit instruction commands are ignored. This way we don't need to check the returned value for each function. If a structure is allocated, and NULL is returned, we set memory error in the compiler and return without accessing the members of the structure. Most structures are only used once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, the tests should have exercised this path (since we fail each malloc systematically).
I wasn't going to examine the JIT code... but then I had to, because of the segfault I triggered.
I realised the general error-handling strategy, which seems sensible. I built a call graph of all the JIT functions. Then I tried to identify all the ones which could "fail" (hard to determine, since the "failing" functions can be void-returning). Then I worked up the call graph to see if I could prove that either: the caller of a "failing" function has an error-check; or alternatively that all the actions it takes are safe even if the previous call failed.
The "XXX" comments I left are for the cases I wasn't able to understand or verify.
For example, here, if
compile_reverse_matchingpathfails due to malloc, is it safe to callcompile_matchingpathbelow (with a NULLccbegin)? I don't understand the code well enough to be confident either way.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm,
ccbegincannot be NULL, since it points to something inside the (already alloc'ed) PCRE byte code. In theory we just generate code as usual, but nothing happens. This is very inefficient, but also a highly unlikely case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compile_reverse_matchingpathdoes return NULL when malloc fails. This would assign NULL toccbegin. Andcompile_matchingpathwill derefence that and crash.In my head, there is a potential problem here, but the tests didn't exercise it, so I don't know whether it's real.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might talk about something different.
https://github.com/PCRE2Project/pcre2/blob/master/src/pcre2_jit_compile.c#L8759
return cccannot returnNULL. I don't see any other return.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PUSH_BACKTRACK is a return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that is a misuse of the macro. It should return with
cc + 1 + 2 * IMM2_SIZEto simply skip the construct.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have pushed a speculative fix to change the return to
cc+1+2*IMM2_SIZE. I can exercise the problem by adding a "return NULL" underneath the problematic PUSH_BACKTRACK: if that specific thing fails then I get a segfault.I hope it's OK that none of the other uses of PUSH_BACKTRACK do the same (they all return NULL). None of the other return values appear to be used at their call sites though, so I think the others are OK.