[mpegts] Validate PSI section CRC32 before parsing PAT/PMT#2084
[mpegts] Validate PSI section CRC32 before parsing PAT/PMT#2084wmdebrito wants to merge 1 commit into
Conversation
The demuxer skipped the CRC32 of every PSI section (it only did `end_psi -= 4` to drop it) and never verified it. Combined with accepting PAT/PMT on any PID (59fd1b3), this means random TS payload whose bytes happen to start with a PSI-like table_id (0x00/0x02) is misparsed as a PAT/PMT: it spuriously (de)registers streams, forces a program change, corrupts the demux state and can crash on the next TSResync() (SIGSEGV in AP4_ByteStream::Read on the async TS reader thread, at HLS segment boundaries on discontinuous live streams). Validate the section CRC32 once the section is fully assembled, before acting on it. The 3-byte section header (table_id + syntax/length word) is reconstructed, since only the section body is kept in the table buffer. This keeps the 59fd1b3 behaviour intact: a real PAT/PMT carried on a non-standard PID has a valid CRC and is still accepted (issue xbmc#2010, whose sample has no PAT and only a PMT on PID 0x1001). Only sections that fail the CRC -- i.e. payload that was never a PSI section -- are dropped. Verified with a standalone harness driving the unmodified demuxer over the issue xbmc#2010 sample and synthetic streams (AddressSanitizer/UBSan clean): - xbmc#2010 (no PAT, PMT on 0x1001, valid CRC): h264+aac discovered, before & after. - valid PMT on an arbitrary PID: still discovered. - corrupt PMT (wrong CRC): accepted by baseline (spurious program change + bogus stream), correctly ignored after the fix. Signed-off-by: Wagner Brito <wagnerbrito@gmail.com>
|
This was tested manually for both scenarios that were failing. Part of the code was written by AI. |
|
Decision: APPROVE Issues: none Evidence:
Review Details
|
|
@kodiai please review PR |
|
Decision: APPROVE Issues: none Evidence:
|
CastagnaIT
left a comment
CastagnaIT
left a comment
There was a problem hiding this comment.
LGTM thank you!
please do backport
2 participants
Description
The demuxer skipped the CRC32 of every PSI section (it only did
end_psi -= 4to drop it) and never verified it. Combined with accepting PAT/PMT on any PID (59fd1b3), this means random TS payload whose bytes happen to start with a PSI-like table_id (0x00/0x02) is misparsed as a PAT/PMT: it spuriously (de)registers streams, forces a program change, corrupts the demux state and can crash on the next TSResync() (SIGSEGV in AP4_ByteStream::Read on the async TS reader thread, at HLS segment boundaries on discontinuous live streams).Validate the section CRC32 once the section is fully assembled, before acting on it. The 3-byte section header (table_id + syntax/length word) is reconstructed, since only the section body is kept in the table buffer.
This keeps the 59fd1b3 behaviour intact: a real PAT/PMT carried on a non-standard PID has a valid CRC and is still accepted (issue #2010, whose sample has no PAT and only a PMT on PID 0x1001). Only sections that fail the CRC -- i.e. payload that was never a PSI section -- are dropped.
Motivation and context
Playback PlutoTv and other malformed streams on IA 21.5.21
How has this been tested?
A custom version with those changes was built and installed on Kodi.
The same stream that was failing after a few minutes, every time, now run for more than 24h without crashing. Same thing was happening with PlutoTv when ads were playing, it crashed every time, but now works fine.
Verified with a standalone harness driving the unmodified demuxer over the issue #2010 sample and synthetic streams (AddressSanitizer/UBSan clean):
Screenshots (if appropriate):
Types of change
Checklist: