Panic when parsing fuzzed file with malformed HTML in it #521
Comments
|
Dang, I thought we had caught all panics by now. It's great that you found this, because it almost certainly means there is a bug in the crate. I will try to reproduce this using your code snippet, and subsequently track down the offending code. Concerning the fuzzing: this is something that I've been thinking about for a while. I had actually considered to move the fuzzing code out of crate and make it a proper project of its own, because a good fuzzer could be useful for other markdown parsers as well. I haven't actually brought this up with any other contributors, so it's just to say that our direction with regards to fuzzing isn't quite clear yet. In any case, thanks for opening this issue. This will help make pulldown more reliable! |
|
If it helps, I can't seem to get it to panic on any input shorter than 8 bytes. And there seems to be multiple bugs, the input (in item_to_event). So my guess is that there's more than one bug, but it'll be difficult to find any more without fixing this main one. I can just re-run the fuzzer after checking out the fix and report any more bugs either here or in new issues, depending on whatever whoever fixes this wants me to do. |
|
FWIW there is another panic the fuzz target triggers almost as soon as it starts (with a dictionary and seed corpus borrowed from https://github.com/google/oss-fuzz/blob/master/projects/cmark/build.sh):
I think until it's decided another option would be to keep the "fuzz" directory in the OSS-Fuzz repository so that the project could be fuzzed continuously there. OSS-Fuzz automatically reports issues and closes them once they're fixed. I've opened google/oss-fuzz#5880. I'd appreciate it if you could let me know what you think. Thanks! |
|
Hi! I was testing pulldown-cmark to try my fuzzer, fuzzcheck, and also discovered three panics. I have added three minimal test cases below to reproduce them. The first two are already mentioned in this thread but the third one is new, I think. fn parse(input: &str) {
let parser = Parser::new(input);
for event in parser {
std::hint::black_box(event);
}
}
#[test]
fn reproduce_crash_1() {
let s = "><a\n";
parse(s);
// panicked at 'range start index 4 out of range for slice of length 3', src/scanners.rs:1061:45
}
#[test]
fn reproduce_crash2() {
let s = "><a a\n";
parse(s);
// panicked at 'range start index 6 out of range for slice of length 5', src/scanners.rs:871:17
}
#[test]
fn reproduce_crash3() {
let s = "><a a=\n毿>";
parse(s);
// panicked at 'invalid utf8: FromUtf8Error { bytes: [60, 97, 32, 97, 61, 10, 175, 191, 62], error: Utf8Error { valid_up_to: 6, error_len: Some(1) } }', src/parse.rs:252:61
} |
|
The two test cases you provided which indeed did crash pulldown in the past seem to no longer do that. I suspect it's due to the introduction of header attribute parsing done by @lo48576. I added two regression tests for this here, and reran your fuzzer for 10 minutes, but found nothing new. Thanks a bunch for bringing this issue to our attention, and for fixing a bunch of them! I'll close this for now as there are no more known panics. |
There's already the support for fuzzing here, but I found this issue through https://github.com/rust-fuzz/targets and wrote a simple
cargo-fuzzfuzzing target that only checks that parsing arbitrary HTML doesn't panic. (Might be worth having a cargo-fuzz target in addition to the existing one, simply because cargo fuzz is easier to get set up and seems to be able to run test cases faster? Though I know testing for panics isn't the main purpose of the existing fuzzer)Below's a standalone demo program. I tested that it crashes both with 0.8.0 from crates.io, as well as the latest version from git (d99667b, if just going to the directory in
~/.cargo/git/checkoutsis correct).Backtrace is below
The text was updated successfully, but these errors were encountered: