Skip to content

fix(parser): stop panicking on malformed (and on some valid) lcov input - #1515

Open
Eljees wants to merge 1 commit into
mozilla:masterfrom
Eljees:fix/1511-lcov-records-without-source
Open

fix(parser): stop panicking on malformed (and on some valid) lcov input#1515
Eljees wants to merge 1 commit into
mozilla:masterfrom
Eljees:fix/1511-lcov-records-without-source

Conversation

@Eljees

@Eljees Eljees commented Aug 6, 2026

Copy link
Copy Markdown

Fixes #1511. Also fixes #1501.

parse_lcov has a dedicated error path — the manage_parsing_error! macro and ParserError::InvalidRecord — but two unwrap() calls sitting next to it panic instead of using it. With lenient parsing (ignore_parsing_error = true) the panic happens even though the caller explicitly asked the parser not to fail.

1. Any line starting with e was treated as end_of_record.
The outer dispatch (src/parser.rs:186) matches on the first byte of the line, so excluded_by_a_tool — or any other record grcov does not know — took the end_of_record branch and hit cur_file.unwrap() on line 194. This is reachable from valid lcov files.

The same byte-level dispatch is why DA:<line>,<count>,<checksum> (the optional third field of a DA record, standard lcov) crashes: the DA branch stops after <count>, the checksum e4f2 is then read as the start of a new line, and e → panic. That is issue #1501.

2. end_of_record before any SF: panicked instead of reporting InvalidRecord.

3. An unknown key longer than four uppercase letters panicked in lenient mode.
key is already computed with checked_mul/checked_add and manage_parsing_error! is already called when it overflows — but in lenient mode the macro only logs, and the following match key.unwrap() (line 231) panics anyway.

The patch routes all three through the existing error path and makes the end_of_record branch check the whole token instead of one byte. No new error semantics are introduced: unknown records are skipped exactly like the ones already handled by the catch-all arm.

Six unit tests are added, in the style of the existing inline-buffer tests (test_lcov_parser_empty_DA_record). Five of them panic on master.

before / after
$ cargo test --lib                # master
test result: FAILED. 133 passed; 4 failed        # 4 = llvm_tools::*, need llvm-profdata

$ cargo test --lib lcov_parser    # master + new tests only
test result: FAILED. 8 passed; 5 failed
  ... panicked at src/parser.rs:194:30: called `Option::unwrap()` on a `None` value
  ... panicked at src/parser.rs:231:27: called `Option::unwrap()` on a `None` value

$ cargo test --lib                # master + new tests + fix
test result: FAILED. 139 passed; 4 failed        # same 4 llvm_tools failures as on master

Not in this PR, tracked for a follow-up. #1511 also mentions the hand-rolled decimal folds. They are genuinely reachable, but only from input no producer emits, and fixing them touches seven call sites, so I kept the diff focused. Reproducers, all panicking on master today:

DA:7,18446744073709551615   ->  src/parser.rs:281  attempt to add with overflow
DA:99999999999,1            ->  src/parser.rs:259  attempt to multiply with overflow
DA:5,!                      ->  src/parser.rs:274  attempt to subtract with overflow

Happy to send that as a second PR (or fold it in here) if you prefer.

AI-assisted: I used Claude to help map the panic sites and to draft the patch and the tests. I reviewed every line, ran each new test against unpatched master first to confirm it panics without the fix, and ran cargo fmt --check and cargo clippy -- -D warnings per the repo's pre-commit config; the analysis and the runs are mine.

parse_lcov has a dedicated error path - the manage_parsing_error! macro and
ParserError::InvalidRecord - but two unwrap() calls next to it panic instead
of using it, even when the caller asked for lenient parsing.

- The outer dispatch matches on the first byte of the line, so any record
  starting with 'e' (excluded_by_a_tool, or the checksum field of a
  DA:<line>,<count>,<checksum> record) took the end_of_record branch and hit
  cur_file.unwrap(). This is reachable from valid lcov files.
- end_of_record before any SF: panicked instead of reporting InvalidRecord.
- An unknown key longer than four uppercase letters panicked in lenient mode,
  because match key.unwrap() runs after manage_parsing_error! has only logged.

Route all three through the existing error path and match the whole token in
the end_of_record branch instead of one byte. Unknown records are skipped
exactly like the ones already handled by the catch-all arm.

Fixes mozilla#1511
Fixes mozilla#1501
@Eljees

Eljees commented Aug 9, 2026

Copy link
Copy Markdown
Author

A note on the red Lint job, since it is not caused by this PR.

All five clippy errors are outside the diff:

src/producer.rs:112:21        this `if` can be collapsed into the outer `match`
src/producer.rs:148:26        can be more succinctly written as a byte str
src/producer.rs:148:57        can be more succinctly written as a byte str
src/path_rewriting.rs:426:21  this `if` can be collapsed into the outer `match`
src/cobertura.rs:67:45        iterating on a map's values

This branch only touches src/parser.rs. I also ran the pre-commit clippy hook in a container against both trees — pristine master and master plus this patch — and on the toolchain I had available (1.88) each exits 0, which suggests the lints above were added to clippy some time after the last commit on master rather than by anything here. All ten test jobs on this PR are green.

If it would help, I am happy to send those five as a separate cleanup PR to get Lint back to green; I kept them out of this one so the diff stays on the parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant