PSIP table parsing should be more robust

[linuxdude42]

Given the recent issue with noisy recordings (#596) and the ~40 Coverity errors related to blindly trusting data received from the network, I thought I'd take a look at trying to fix some of these errors.

As it stands today, the only validity checking that the PSIP table parsers perform is to validate the CRC (if present). Once processing of a table begins, there are no checks to ensure that the table is consistent or to ensure that processing doesn't read past the end of the received data. If any errors have crept into the tables before the application of the CRC (most tables require a CRC, but not all) then it is entirely possible for the parser to access memory past the end of the received data. This could cause unexpected behavior, anything from a glitch in AV processing to a crash of the system. MythTV should be able to recognize malformed tables, drop the portions that run out of bounds or drop them entirely, and then continue running.

I have a tree where I've enhanced the ATSC table parsers to perform bounds checking at each step, and to protect the system by dropping any field in a table that exceeds those bounds (and drop all subsequent fields). This should prevent MythTV from processing any data that could potentially crash the system. These parsers currently perform better than the existing parsers, in large part because they calculate the maximum table size up front and perform a single memory allocation, making those parts of the functions O(1) instead of the current O(log2 n). There's also no data copying each time the table is resized. The remainder of the functions are still O(n) processing the tables.

Example timing without optimization:

     Old: 0.0018 msecs per iteration (total: 61, iterations: 32768)
     New: 0.0011 msecs per iteration (total: 75, iterations: 65536)

     Old: 0.0048 msecs per iteration (total: 80, iterations: 16384)
     New: 0.0021 msecs per iteration (total: 72, iterations: 32768)

and with optimization

     Old: 0.00024 msecs per iteration (total: 65, iterations: 262144)
     New: 0.00017 msecs per iteration (total: 93, iterations: 524288)

     Old: 0.00047 msecs per iteration (total: 62, iterations: 131072)
     New: 0.00036 msecs per iteration (total: 95, iterations: 262144)

I don't believe that these changes go far enough. I think MythTV should completely drop any PSIP table when it detects that any of the fields exceeds the bounds of the received data. It should just throw an error from the constructor and let the caller handle it.

If there's agreement that this level of checking is worthwhile, then all the other *tables.cpp files should have similar changes.

[garybuhrmaster]

Better validation is always a good plan.

As I recall, at least some of the code may have originated from libdvbv5. Looking at that upstream repo I see a number of commits about improving robustness over the years.

Does it make any sense to either resync with, or perhaps just depend on, the upstream library (yes, some refactor may be needed)?

[kmdewaal]

By and large I do agree with doing more checks and especially I like optimizing performance by improving buffer allocations.
However, I have a few remarks on this.

1. Part of the motivation mentioned is issue mythfrontend crash when recording is very noisy #596 but this is a divide by zero in new code created by David himself. I think the real motivation is to have zero warning messages by any tool you can think of. That is not necessarily wrong but one should be clear about the reasoning.

2. I really want a separation between non-functional code improvements such as buffer allocations, index checking etc etc and what are functional changes. Functional changes should NOT be hidden in a sea of non-functional changes.
   Putting an incidental functional change in a sea of non-functional code improvement changes has happened before quite often and there is also an example of this in pull request More robust parsing for ATSC PSIP tables (proposal) #624.

Line 626 in mpegtables.cpp, commit fe6d17e, the additional check on DescriptorID, is a functional change. Is this really checked against the relevant standards? Is the IsVideo() check that preceeds it really not good enough to check that this is a video stream? If the IsVideo() not good enough is that not something that also should be fixed? Which stream created the problem for which this change is a fix? Really sure that this does not break anything?

For a functional change I would appreciate a separate commit, with a clear description of what problem is fixed by this commit, possible a stream that shows the problem so that it can be reproduced.

[linuxdude42]

Klaas,

Yes, my motivation is to eliminate bugs in MythTV to make it a better program. We run three well know static analysis tools (clang-tidy, Coverity, and cppcheck) and one lesser known static analysis tool (clazy) in order to point out problems in the code or point out places where the code could be improved. I have taken on the task of trying to fix these warnings (or at least analyze and notate why they shouldn't be fixed) because no-one else seems to have the time. As you pointed out I occasionally make mistakes, but my track record over the last five years is pretty damn good. When I started working on MythTV even a simple "clean" compilation threw several hundred warning messages. That was long before I started looking at the output from the static analysis tools.

My original comment should have referenced issue #589 (not 596), which was due to MPEGStreamData::ProcessTSPacket not properly handling the adaptation field. Yes, I know you blame my C to C++ array changes for causing that issue. They did not. The missing check for an adaptation field and the potential to read past the end of the TS packet appear to have always been present in that function. My change simply prevented reading past the end of the TS packet, and thereby shone a great big glaring spotlight on a preexisting undiscovered bug. I have mostly avoided the video processing code in the past, but that adaptation field problem and the number of Coverity bugs in the mpeg/mheg directories are why I decided to spend time looking at the mpeg and mheg code.

I had already created a separate commit for the functional change in ProgramMapTable::IsStillPicture. To address your questions about that change... I have spent quite a bit of time reading ISO/IEC 13818-1 and ATSC A/65; not quite as much reading SCTE 35 and the DVD Blue Book. (I also have copies of ATSC A/90, ETSI TR 101, and ETSI EN 301, which seem to be the references necessary for eventually investigating MHEG.) I discovered the problem addressed by fe6d17e while creating test cases using PMT packets captured from multiple services. I am using the TSDuck tools to parse these captured streams and produce the 'actual' values used in those test cases. The test case using a PMT packet transmitted by ESPN consistently failed, and I tracked the problem to ProgramMapTable::IsStillPicture. The IsVideo() check in that function is fine. The problem is that the next line assumes that a video stream entry only contains a single descriptor and that that descriptor is always of type video_stream_descriptor(0x02). Nowhere in ISO-IEC 13818-1 do I see that assertion stated. The ESPN PMT packet used to generate the test case has a video stream containing a single descriptor in the user private range (0x86), and I have a trace (but not a test case) from a New Zealand station showing a video stream that contains only a single maximum_bitrate_descriptor(0x14). Yes, I should have written a better message for that individual commit, but I am nowhere close to considering this set of changes ready to commit. I was interested in getting the concept of additional table validation in front of you and a wider audience. That's why the pull request was labeled as a "proposal".

I specifically wanted you to look at these proposed changes because you spend far more time in this part of the code than I do, and probably understand it better than I ever will. I see you're in basic agreement that checking is good, as is optimizing buffer allocations. What about the idea of discarding a malformed table vs processing only the parts of the table that fit within the packet? I think that if any part of a table is determined as corrupt, we shouldn't process any part of that table. I've come to believe that the constructor throwing an error is the correct response to an invalid packet, but there is not a lot of error processing in MythTV that is designed using throw/catch (although there is more than I expected to find when I checked). The other approach would be to set a flag that says this newly allocated data structure is corrupt, shouldn't be used, and should be immediately deleted. There's no memory management when throwing an error because the object is never created. I don't think adding a throw to the code causes any extra processing to the non-throwing case. If the throw is executed you're already into error handling code, and a few extra cycles to find the landing spot for the throw (which should always be the first landing spot) shouldn't cause too much slowdown. Its been a long time since I looked at compiler internals though. What about the idea of splitting out the ATSC/SCTE/DVB tests into their own separate test files, as there are going to be significantly more test cases than before? Or should they all remain in a single file? These are the sorts of questions I was hoping to address by filing this issue.

[linuxdude42]

Gary, thanks for the pointer to libdvbv5. I looked at it and and it doesn't seem to bear that much resemblance to our code.