x DTS: fix infinite loop in Extensions() when handler overflows asset boundary - #2631
x DTS: fix infinite loop in Extensions() when handler overflows asset boundary#2631mjuhasz wants to merge 2 commits into
Conversation
… boundary XLL() temporarily expands Element_Size to cover the full XLL frame. If parsing fails inside XLL(), Trusted_IsNot() fires and sets Element_Offset=Element_Size (the expanded value). XLL() then restores Element_Size=Element_Size_Save (the smaller outer asset boundary), leaving Element_Offset>Element_Size. The unsigned subtraction in the while-loop condition wraps around, making the loop run forever. Reproducible: MKV file with DTS-HD MA audio and any attached file (e.g. SRT). The hang was introduced when commit 892a9a6 restored the Skip_XX integrity check, enabling the Trusted_IsNot() code path for the first time.
|
Need to be careful when reassigning Element_Size. Maybe (I did not read DTS specs or see what is actually going on with that sample file) check should be done before reassigning Element_Size instead. Usually Element_Size is assigned smaller then restored and not expanded. If Element_Size is expanded beyond the end of buffer, integrity checks will fail and there may be buffer overread. |
|
I think something else is wrong and this PR just covers up the symptoms. AssetFsize: 3428 bytes and the first XLL is 3304 bytes. The 2nd XLL thus only has 124 bytes but the Segments in it are 3167 bytes. |
…e exceeds asset boundary When the declared XLL logical frame size (LLFrameSize) exceeds the remaining bytes in the current ExSS asset, XLL() was unconditionally expanding Element_Size beyond the actual buffer boundary. Subsequent Skip_XX calls then triggered the INTEGRITY_SIZE_ATLEAST integrity check (restored by 892a9a6), which called Trusted_IsNot() and set Element_Offset = Element_Size (the oversized value). When XLL() restored Element_Size to the outer asset boundary, Element_Offset was left far past it, causing the unsigned underflow and infinite loop in Extensions(). Add a bounds check before expanding Element_Size: if the computed XLL frame end exceeds the current asset boundary, unwind the parser state (BS_End + Element_End0) and skip to the end of the asset, mirroring the existing LLFrameSize < 6 guard pattern directly above.
|
@cjee21 you are right. The original fix only addresses the symptom. Here is an attempt to find the root cause and a proper fix. Root cause
The formula is correct for valid data. The sync word (4 bytes) is consumed by The problem is that there is no check that this computed end stays within the outer asset boundary stored in The file in questionThe 3428-byte asset contains two XLL frames. The 1st (3304 bytes) parses cleanly. The 2nd starts at asset offset 3304, leaving only 124 bytes available. Its header declares Concrete numbers from the sample with the asset starting at a relative origin:
Exact failure chain (with commit 892a9a6)
My original PR's clamp in The fixCheck before expanding This mirrors the existing The Why early return rather than partial parsingThe second XLL's Attempting to parse the header and channel set of the partial frame anyway would require adding additional overflow guards deeper in the function; the simple early return is cleaner and costs nothing in the normal (valid-data) path. Regarding the original Extensions() clampThe |
| return; | ||
| } | ||
| auto Element_Size_Save=Element_Size; | ||
| if (Element_Offset_Start-3+LLFrameSize>Element_Size_Save) |
There was a problem hiding this comment.
Ensure that Element_Offset_Start - 3+ LLFrameSize can not overflow. Maybe LLFrameSize>Element_Size - Element_Offset_Start - 3 is better?
| continue; | ||
| } | ||
| auto Element_Size_Save=Element_Size; | ||
| Element_Size=Element_Offset+Size; |
There was a problem hiding this comment.
We should do similar checks here to prevent malformed file from expanding Element_Size and causing over-reads.
|
Looks like a corrupt stream, all header CRCs are correct but the LLFrameSize and SegmentSize of second XLL element exceeds the space available before the next frame. Anyway, MediaInfo needs to check boundaries before re-assigning Element_Size based on data from stream that may be malformed. Also should validate sizes that are read from stream before using them in Get_BS that only handles max 32-bits. |
2 participants
Problem
MediaInfo 26.05 hangs indefinitely when parsing the attached MKV file that contains both DTS-HD MA audio and an attachment (an SRT subtitle file). Version 26.01 handled this file without issue. Remuxing using mkvmerge does not help.
Root Cause
Extensions()iterates DTS extension assets with an unsigned while-loop condition:XLL() temporarily expands Element_Size to cover the full XLL frame. If parsing fails inside XLL() (e.g. a segment-size mismatch causes Trusted_IsNot() to fire), Element_Offset is set to the expanded Element_Size. XLL() then restores Element_Size = Element_Size_Save (the smaller outer asset boundary), leaving Element_Offset > Element_Size. The unsigned subtraction wraps to ~2^64, and the loop runs forever.
This regression was introduced by commit 892a9a6, which restored the Skip_XX integrity check that calls Trusted_IsNot().
Fix
Clamp Element_Offset to Element_Size after each extension handler call, before Element_End0(). One line added.
Reproduction
I have an MKV with a DTS-HD MA audio track and an attached file (srt) that will reproduce the hang with mediainfo 26.05.
Test.mkv.zip