[mpegts] Validate PSI section CRC32 before parsing PAT/PMT - Backport#2085
Conversation
… from Piers 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>
|
Decision: APPROVE Issues: none Evidence:
Review Details
|
|
@CastagnaIT it seems to be failing on a lib download. Not related to the code: CUSTOMBUILD : error : downloading 'https://github.com/google/googletest/archive/refs/tags/v1.14.0.tar.gz' failed [D:\a\1\s\build\googletest.vcxproj] |
Backport from: #2084
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.
Verified with a standalone harness driving the unmodified demuxer over the issue #2010 sample and synthetic streams (AddressSanitizer/UBSan clean):
Description
Motivation and context
How has this been tested?
Screenshots (if appropriate):
Types of change
Checklist: