Repository navigation
[FIX] matroska: accumulate EBML element IDs in an unsigned int - #2407
Open
pranayr710 wants to merge 2 commits into
Open
pranayr710 wants to merge 2 commits into
pranayr710 wants to merge 2 commits into
Conversation
Every EBML parse loop in matroska.c builds the element ID one byte at a
time into a signed int:
int code = 0, code_len = 0;
while (pos + len > get_current_byte(file))
{
code <<= 8;
code += mkv_read_byte(file);
On a well-formed file the switch matches within four bytes and code is
reset, but nothing resets it when no case matches, so the shift walks
past INT_MAX and signed overflow is undefined behaviour.
UBSan on a fuzzed MKV corpus reports it at four of the eight identical
loops:
matroska.c:489:8: runtime error: left shift of 8521680 by 8 places
cannot be represented in type 'int'
matroska.c:1229, matroska.c:1275, matroska.c:1963: same
Make code unsigned in all eight. EBML IDs are at most four bytes, every
case label is a positive constant, and the existing diagnostics already
print it with "0x%x", which expects unsigned. Unsigned wraparound is
well defined, so the accumulator keeps the behaviour the format implies.
Verified against master built from the same source in the same image:
UBSan over a 25-file fuzzed MKV corpus: 3 sites -> 0
Well-formed MKV: byte-identical extracted track, stable over 5 runs
Compiler warnings: 15 before, 15 after (all pre-existing)
Three truncated inputs differ in stdout, but master already differs from
itself on those: its diagnostics print the accumulator, whose value was
undefined precisely because of this bug. Repeated runs of master alone
produce the same set of hashes flagged in the comparison.
cfsmp3
requested changes
Nov 10, 2026
cfsmp3
left a comment
cfsmp3
left a comment
Contributor
There was a problem hiding this comment.
This is Carlos' claude doing the first pass. Before Carlos takes a look, please address these issues.
The UB is real: UBSan on master reports matroska.c:620 and matroska.c:1969 (left shift of a 3-byte value by 8 when the first ID byte is >= 0x80). Your change removes it, and output is byte-identical on our MKV samples.
- The project uses fixed-width types. An EBML ID is exactly 32 bits, so please declare the accumulator as
uint32_t code = 0;in all eight loops instead ofunsigned int, and print it with"0x%" PRIx32in the "Unknown element" / "Deprecated element" diagnostics. - The description says nothing resets
code. That's wrong: each loop resets it after 4 bytes (code_len == MATROSKA_MAX_ID_LENGTH). The overflow happens on the 4th byte when the first byte has its top bit set. Please correct the description.
Review feedback: an EBML ID is exactly 32 bits, so say so with a fixed-width type rather than unsigned int, and print it with PRIx32 in the "Unknown element" and "Deprecated element" diagnostics.
Contributor
Author
|
Thanks — both points were right, and the second one was a real error in my description rather than a wording problem. 1. Fixed-width type. 2. The description was wrong and I have rewritten it. I wrote that nothing resets I also corrected the UBSan figures. The old description said four sites in prose and three in the table, which was careless; re-measured on this corpus it is what you reported. No functional change from the previous revision — type and format specifiers only. |
Collaborator
CCExtractor CI platform finished running the test files on linux. 171/238 tests matched the approved output:
67 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9878, commit 185fe3f:
Pass there, fail here:
Compared with the commit this branch was cut from — test 9750, commit d7eb132:
Fail on both but produce different output — behaviour moved even though the verdict did not:
Fail there, pass here:
This branch changes the behaviour of 1 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides. |
Collaborator
CCExtractor CI platform finished running the test files on windows. 171/238 tests matched the approved output:
67 tests do not match the approved output. That is the pass/fail verdict. Whether this branch caused it is a separate question, answered below.
Compared with the tip of master — test 9693, commit af4b28d:
Fail on both but produce different output — behaviour moved even though the verdict did not:
Fail there, pass here:
Compared with the commit this branch was cut from: the same run as the tip of master (test 9693), so the comparison above already covers it. This branch changes the behaviour of 42 test(s) relative to the tip of master. Those are the ones worth looking at; anything else in the list fails the same way on both sides. |
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
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.
Reason for this PR:
Sanity check:
What was wrong
Every EBML parse loop in
matroska.caccumulates the element ID a byte at a time into a signedint:An EBML ID is at most four bytes, and the loops do bound
codeto that: every matchingcaseresets it throughMATROSKA_SWITCH_BREAK, and thedefaultarm resets it oncecode_len == MATROSKA_MAX_ID_LENGTH. So the accumulator never grows past four bytes.The overflow is on that fourth byte. Going into the fourth iteration
codeholds three bytes;code <<= 8moves them into bits 8–31. If the first of those bytes is>= 0x80the result needs bit 31, whichintcannot represent — signed left-shift overflow, undefined behaviour under C11 6.5.7p4. It needs no malformed input beyond an unknown four-byte ID whose first byte has the top bit set.UBSan on master reports it at:
The change
codebecomesuint32_tin all eight loops — an EBML ID is exactly 32 bits, so the fixed-width type says what is meant and unsigned shift/wraparound is well defined. The"Unknown element"and"Deprecated element"diagnostics now print it with"0x%" PRIx32to match.code_lenis left asint; it is only a counter.Reproducing
One gotcha that cost me time: CCExtractor writes each MKV subtitle track to a file next to the input, so with a read-only input directory it stops at
Error: Cannot create output file for subtitle trackbefore reaching the parser loops. The input directory has to be writable to reach this code.Verification
Built master and this branch from identical source in the same image.
a58ba9a536a58ba9a536(stable over 5 runs each)One thing to be straight about: three truncated inputs show differing stdout between the two builds — but master already differs from itself on those. Its "Unknown element" diagnostic prints the accumulator, whose value was undefined precisely because of this bug. Running master alone five times produces the same set of hashes that the comparison flagged:
So that is pre-existing nondeterminism on malformed input, not a change in behaviour. Well-formed files are byte-identical.
This is UB a compiler is entitled to exploit rather than an observed misbehaviour — on x86/GCC today the shift does what you would naively expect. Same category as #2353.
(Prepared with AI assistance; the reproducers and the sanitizer runs are my own.)