[FIX] WTV: resynchronise instead of trusting an implausible element length - #2323
Merged
Conversation
Collaborator
CCExtractor CI platform finished running the test files on windows. Below is a summary of the test results, when compared to test for commit c328108...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
…ngth get_data() reads a 32-byte element header and believes whatever 32-bit length it finds. On one recording the parser walks 182,902 elements correctly and then meets 8800 bytes it cannot interpret; the "length" read there is 0x8A9E4344. That value is passed to skip_sized_buffer(), which hands it to buffered_seek() as an int, turning a forward skip into a 1.8 GB backward seek and a segfault. Reject a length that cannot describe an element (below the 32-byte header, or implausibly large) and hunt forward for the next header instead. Most GUIDs that open a timeline element share the same 15 trailing bytes and differ only in the first, which makes them a dependable anchor; the scan keeps a rolling 32-byte window so the caller ends up positioned exactly as after a normal header read. The end-of-file marker is exempt from the length test. It legitimately carries a zero length and is handled further down, so treating it as garbage ended the parse early and changed the last cue of every WTV recording. On the affected file the parser resynchronises after 8800 bytes -- matching the gap measured independently between the last valid element and the next one -- and carries on to the end of the recording. Verified byte for byte over the 45 local WTV samples, master vs this branch: byte-identical output : 44 differing : 0 crash fixed : 1, cues 100 -> 151 Coverage on that recording goes from 228s to 356s of a 358s file, and the resync fires exactly once across the whole corpus.
Collaborator
CCExtractor CI platform finished running the test files on linux. Below is a summary of the test results, when compared to test for commit 6077cf5...:
Your PR breaks these cases:
NOTE: The following tests have been failing on the master branch as well as the PR:
It seems that not all tests were passed completely. This is an indication that the output of some files is not as expected (but might be according to you). Check the result page for more info. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A WTV recording crashes ccextractor, and the same defect silently costs a third of its captions.
What happens
get_data()reads a 32-byte element header and believes whatever 32-bit length it finds there. On5f6dfe831e35…wtvthe parser walks 182,902 elements correctly, then meets 8800 bytes it cannot interpret. The "length" it reads at that point is0x8A9E4344(2,325,627,716).That value flows to
skip_sized_buffer(uint32_t size), which passes it tobuffered_seek(ctx, int offset).2325627720 - 2^32 = -1969339576, so a forward skip becomes a 1.8 GB backward seek, and the read that follows walks off the buffer.Why the parser gets there
Independently of ccextractor, the file's own structure shows the gap. Walking the element chain from the last good header:
ffmpeg reads a packet at 236982768, inside that region, and decodes the whole file to completion, so the recording is sound — it is our element walk that cannot cross this.
Fix
Reject a length that cannot describe an element (smaller than the 32-byte header, or implausibly large) and hunt forward for the next header rather than trusting it.
Most GUIDs that open a timeline element share the same 15 trailing bytes and differ only in the first (data, sync, index, stream1, stream2…), which makes them a dependable anchor. The scan keeps a rolling 32-byte window, so when it re-anchors the caller is positioned exactly as it would be after a normal header read, and the existing code path continues unchanged.
Measured over all 45 local WTV samples
On the affected recording, coverage goes from 228 s to 356 s of a 358 s file. The resync fires after exactly 8800 bytes, matching the gap measured independently from the file structure — the two numbers were derived separately and agree.
The single resync across 45 files is the point worth noting: this is not a parser that now guesses its way through recordings, it is a guard that fires once, where the old code crashed.
Behaviour change worth flagging
Two samples (the same content, present twice locally) produced no captions before and no captions after, but now exit 10 ("no captions were found") instead of 0, because a failed resync ends the parse rather than running off the end. Exit 10 is the accurate code for a file with no captions, but it is a change, and one of those files is regression test rt7 — that test already fails today ("No output generated but there should be"), so it stays failing, just with a different reason.
Related
Found while investigating a segfault. #2322 hardens the buffer layer so a bogus size can no longer read out of bounds; this PR stops the bogus size being produced in the first place. They are independent and either is useful alone.