MatroskaParser: fix hardcoded 32-track ceiling and its silent failure - #681
Open
MawCeron wants to merge 5 commits into
Open
MatroskaParser: fix hardcoded 32-track ceiling and its silent failure#681MawCeron wants to merge 5 commits into
MawCeron wants to merge 5 commits into
Conversation
m_kftime and m_seendf were fixed-size arrays sized by MAX_TRACKS regardless of how many tracks the file actually has. Sizing them to mf->nTracks via alloca (already used elsewhere in this file) is a behavior-preserving change on its own, and is a prerequisite for raising MAX_TRACKS without blowing up mkv_Seek's stack frame on every seek.
fillQueues() and mkv_Seek's internal completion tracking used an unsigned int bitmask (one bit per track) to record which tracks to ignore/skip. That silently breaks for nTracks > 32 (1<<n becomes UB). Replace it with a plain per-track byte array sized to mf->nTracks, so tracking is not tied to the machine word width. mkv_ReadFrame keeps its own unsigned int mask parameter for now (converted separately, next commit) but builds a local skip array from it internally, since it also does its own manual track-skip checks outside of fillQueues. No behavior change: MAX_TRACKS is still 32, so every array here is still implicitly bounded the same as before.
… index trackMask (and mkv_SetTrackMask/mkv_ReadFrame's mask parameters) is never used in this codebase to express an arbitrary multi-track bit combination - only "no filter" (mask == 0) or "keep exactly one track" (mask == ~(1<<i)). Representing that as a plain track index (selectedTrack, -1 meaning no filter) removes the bit-width ceiling entirely instead of just widening it, and is a smaller diff than a dynamically-sized bitset. This also happens to fix mkv_wrap.cpp's own latent UB: trackToRead is unsigned and `1 << trackToRead` was undefined behavior for trackToRead >= 32, independently of trackMask's own width. mkv_SetTrackMask/mkv_ReadFrame carry the `X` (export) macro in MatroskaParser.h, i.e. they're this vendored library's public API - changing their signature would be a breaking change for any other consumer. Confirmed mkv_wrap.cpp is the only consumer in this repo.
… ceiling MAX_TRACKS was never a structural requirement: mf->Tracks already grows dynamically (AGET/ArrayAlloc, the same pattern used for QBlocks/Cues/Attachments elsewhere in this file), and the previous commits removed the last two fixed-size arrays that depended on this constant (mkv_Seek's m_kftime/m_seendf) and the mask-width ceiling (trackMask). It was purely an early guard in parseTrackEntry. Picking a "realistic" number to bump it to (64, 128, ...) would just move the same arbitrary ceiling further out. Since nothing downstream depends on this constant for sizing anymore, raise it to a value whose only job is bounding memory a corrupt/malicious file could force us to commit to track parsing. Fixes TypesettingTools#532
The "Too many tracks." error already existed but was silently swallowed: when tracks are parsed inline during parseSegment's forward scan (the common case), parseSegment's own local setjmp catches the longjmp and just clears MPF_ERROR to tolerate it, so mkv_OpenEx never saw it and returned success with a truncated track list. Only files where Tracks is discovered lazily via parsePointers happened to propagate the error correctly. Add a sticky MPF_TRACKLIMIT flag that survives every recoverable-error local setjmp between parseTrackEntry and parseFile, and check it once at the top level after track parsing is fully resolved either way, so the user gets a real error message regardless of which path parsed the tracks.
Member
So does this mean that you did not test this PR in Aegisub itself? Please do at least this before sending untested pull requests for others to review. |
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.
Summary
Fixes #532. The 32-track limit turned out to be deeper than "widen the bitmask":
MAX_TRACKS(32) inMatroskaParser.cguards registration of tracks inparseTrackEntry— tracks past Adding lead-in/lead-out is broken on first use #32 are never read intomf->Tracksat all, so downstream code (including any bitmask) never even sees them.mf->Tracksalready grows dynamically (AGET/ArrayAlloc, same pattern asQBlocks/Cues/Attachments), soMAX_TRACKSwas never a structural requirement — it's now just a sanity ceiling (65536) against corrupt/malicious files, not a "hopefully big enough" guess.trackMask(and the two other bitmask-shaped internals it touches:fillQueues's mask param andmkv_Seek's own local per-track completion bitmask) is never used in this codebase to express an arbitrary multi-track combination — only "no filter" or "keep exactly one track". Replaced with a plain track index (selectedTrack, -1 = no filter), which removes the bit-width ceiling entirely instead of just moving it, and incidentally fixesmkv_wrap.cpp's own latent UB (1 << trackToReadfortrackToRead >= 32).setjmprecovery scope when tracks are parsed inline (the common path) —mkv_OpenExwould return success with a truncated track list and no indication anything was wrong. Added a sticky flag that survives that recovery so the error reaches the caller regardless of which internal path parsed the tracks.Split into 5 commits (see individual messages for the reasoning behind each): 3 behavior-preserving refactors that make everything downstream tolerate an arbitrary track count, then the actual ceiling raise, then the error-surfacing fix — in that order so every commit compiles and none of them regress behavior on its own.
Test plan
Built locally (meson/ninja) after every commit — each compiles clean. For the actual bug, wrote a small standalone harness that links
MatroskaParser.cdirectly (bypassing the GUI) and drives it exactly the waymkv_wrap.cppdoes, against a synthetic 40-subtitle-track.mkv(generated withffmpeg, mapping the same input stream 40 times):Before this fix (same harness built against
master):mkv_GetNumTracksreports 32 instead of 40 — tracks 32-39 are silently dropped,mkv_Openstill reports success, no error anywhere.After this fix:
mkv_GetNumTracksreports 40, the last subtitle track (index 39) is correctly selected viamkv_SetTrackMask, andmkv_ReadFramereturns exactly the one frame from track 39 with zero cross-track corruption.Regression check: same harness against a normal 2-track file still reads correctly.
Confirmed the pre-fix truncation (32 tracks silently registered instead of 40) against the same test file, via the same harness built against master.
Confirmed the post-fix file opens with all 40 tracks and reads the correct track's data.
Confirmed a normal small file (2 tracks) still works identically — no regression.
Manual GUI verification (open the synthetic file in Aegisub itself) — the harness above exercises the exact same
MatroskaParser/mkv_wrapfunctionsMatroskaWrapper::GetSubtitlescalls, with matching arguments.