Closed addisoncrump closed 1 year ago
Because of your JIT comparisons, I don't think I can merge this just at the moment, because there have been some changes to the interpreter that have not yet been implemented in JIT. The expectation is that the updated handling of \w etc. will make it into JIT, but the new support for variable-length lookbehinds may lag for some time - this won't matter in a release because it involves an opcode that is not recognized by JIT, so JIT compilation will fail and matching should automatically fall back to the interpreter. (At the moment, the tests for the new behaviour of \w etc. have the "no_jit" option.)
How's this? I skip the comparison if the compilation failed, and I wrapped everything in #ifdef SUPPORT_JIT
. In cases where parity isn't present, this should just skip the comparison.
Yes, that's good for the new feature that JIT doesn't yet support. However, we still have to wait for JIT to catch up with the \w changes as otherwise the comparisons will fail. I am hoping these changes will happen fairly soon because we can't put out a new release until they are done.
Understood. I think I need to change CI + upstream OSS-Fuzz configuration (currently it doesn't --enable-jit
) before this lands anyways.
I think the jit simply falls back to interpreter for the new opcode. So it will be an interpreter-interpreter comparison.
@PhilipHazel Thanks for merging. Can you approve the related PR in OSS-Fuzz?
As the JIT is now updated, I have merged this patch. The compiler now complains about two unused variables: ovector_count and errorcode_jit. I will take a look and remove them if it's obvious they are redundant. Then wait to see what ClusterFuzz does.....
The compiler now complains about two unused variables: ovector_count and errorcode_jit.
Ah, these variables are declared even when --enable-jit
is not specified. That's a pretty quick fix (just wrapping them in #ifdef SUPPORT_JIT
).
Will do. I don't think I have any status to approve anything in OSS-Fuzz do I?
In the OSS-Fuzz issue, it lists you as one of the potential reviewers. Not sure if that's accurate though.
I didn't know that; I have not had many dealings with OSS-Fuzz; I just support pcre2_fuzzsupport. I will take a look.
I seem to have managed to approve.
Now that I look at the code in pcre2_fuzzsupport.c, I see a problem. It doesn't #include config.h and so SUPPORT_JIT will never be set! I don't suppose there's any reason why it shouldn't #include config.h, though. However, I have to finish for the day now.
:grimacing: Well, that's a dumb mistake on my part. I'll fix that and push changes, mark OSS-Fuzz PR as draft until then.
This PR makes two changes to the PCRE2 fuzzer. Namely, it uses part of the input to determine the selection of the options, allowing for a greater selection of options (historically, it was restricted to 256 combinations of options -- significantly lower than potential) and adds an extra step in which JIT matches are compared to non-JIT matches as a simple differential oracle allowing for the detection of JIT miscompilation or incorrectness in either implementation.
Caveats:
These strategies have been shown to be effective in rapidly finding issues in rust-lang/regex in the presence of various Regex optimisation strategies. If there are other scenarios in which we can compare the results of two different sets of options consistently (i.e. they should always be the same result, or one/both should error out and we skip the compare) then those should also be added.
I have run this fuzzer with the most up-to-date Clusterfuzz corpus and found no crashes at this time, but this may uncover future issues quite rapidly.