Mailing List Archive

Patch to avoid crash in 'ShortVirtualChannelTable' parsing
Hello,

I have been sitting on this patch for a little while -- forgot about it
completely to be honest.

In the last year, one of my local ATSC channels apparently changed
something in their systems. Previously, on their channel, they
broadcast two streams on separate subchannels -- one 1080i/30 the other
480i/30.

They made a change to remove the 480i subchannel. In addition, their HD
stream now changes frequently between 10801/30 and 1080p/24 (sometimes
every few seconds especially during commercials), probably due to some
detection in the source material. Broke some things in my transcode flow
but that's another subject ...

When they did this, it seems they kept sending the VCT for the other
subchannel, but it contained garbage data. This caused the parser to
overrun a buffer and crash. It was pretty systematic after tuning that
channel.

I've attached a patch I have been using for a while to correct this
problem, and haven't had issues since. I'm not sure if it's reproducable
with the current signal they are broadcasting, but I could try if a
backtrace is desired.

If I remember correctly -- the crash occurred inside the MPEGDescriptor
class because the 'end' pointer was not supplied. When this is the case,
no bounds checking is done. There are other uses of the MPEGDescriptor
class, and conceivably any of them could crash when there is bad data if
the end pointer is not given in the constructor.

There also seems to be a case where the same pointer is pushed into the
m_ptrs list twice (when 'descriptors_included' is false), but I do not
run into this case. I'm not sure if this is a problem, since I did not
look into how this list is used.

I attempted to open a ticket for this, but for some reason the Trac site
does not like something to do with my GitHub login (I get an error
complaining about an invalid username or password after submitting the
form that is asking for my name to associate with my GitHub account).

Thanks,

--
Douglas Paul
Re: Patch to avoid crash in 'ShortVirtualChannelTable' parsing [ In reply to ]
On Mon, 20 Jan 2020 at 19:24, Douglas Paul <doug@bogon.ca> wrote:

> Hello,
>
> I have been sitting on this patch for a little while -- forgot about it
> completely to be honest.
>
> In the last year, one of my local ATSC channels apparently changed
> something in their systems. Previously, on their channel, they
> broadcast two streams on separate subchannels -- one 1080i/30 the other
> 480i/30.
>
> They made a change to remove the 480i subchannel. In addition, their HD
> stream now changes frequently between 10801/30 and 1080p/24 (sometimes
> every few seconds especially during commercials), probably due to some
> detection in the source material. Broke some things in my transcode flow
> but that's another subject ...
>
> When they did this, it seems they kept sending the VCT for the other
> subchannel, but it contained garbage data. This caused the parser to
> overrun a buffer and crash. It was pretty systematic after tuning that
> channel.
>
> I've attached a patch I have been using for a while to correct this
> problem, and haven't had issues since. I'm not sure if it's reproducable
> with the current signal they are broadcasting, but I could try if a
> backtrace is desired.
>
> If I remember correctly -- the crash occurred inside the MPEGDescriptor
> class because the 'end' pointer was not supplied. When this is the case,
> no bounds checking is done. There are other uses of the MPEGDescriptor
> class, and conceivably any of them could crash when there is bad data if
> the end pointer is not given in the constructor.
>
> There also seems to be a case where the same pointer is pushed into the
> m_ptrs list twice (when 'descriptors_included' is false), but I do not
> run into this case. I'm not sure if this is a problem, since I did not
> look into how this list is used.
>
> I attempted to open a ticket for this, but for some reason the Trac site
> does not like something to do with my GitHub login (I get an error
> complaining about an invalid username or password after submitting the
> form that is asking for my name to associate with my GitHub account).
>
> Thanks for your patch. I appreciate that you create the fix and take the
trouble to send it. I will look at it.
However, it would be very convenient if I can reproduce the problem and
test possible fixes myself and I can do that if you would sent me a capture
of the complete transport stream that contains the problematic channel.
This can easily be made with the "MPTS channel" option that is now in
MythTV, or with any other appliance, e.g. dvbsnoop, that can be used to
make a recording of the full transport stream. Bigger is better, if
possible then e.g. 2GB (the maximum that can be sent for free with
WeTransfer) or e.g. 4GB (which can be sent as an email attachment to my
gmail address) would be perfect.

Thanks,
Klaas.
Re: Patch to avoid crash in 'ShortVirtualChannelTable' parsing [ In reply to ]
Hi Klaas,

On Mon, Jan 20, 2020 at 08:50:02PM +0100, Klaas de Waal wrote:
> Thanks for your patch. I appreciate that you create the fix and take the
> trouble to send it. I will look at it.
> However, it would be very convenient if I can reproduce the problem and
> test possible fixes myself and I can do that if you would sent me a capture
> of the complete transport stream that contains the problematic channel.
> This can easily be made with the "MPTS channel" option that is now in
> MythTV, or with any other appliance, e.g. dvbsnoop, that can be used to
> make a recording of the full transport stream. Bigger is better, if
> possible then e.g. 2GB (the maximum that can be sent for free with
> WeTransfer) or e.g. 4GB (which can be sent as an email attachment to my
> gmail address) would be perfect.

I will look into this. Hopefully their configuration is still broken.

In the case they have fixed things, would it be of any use if I capture
a TS and modify it to exercise the failure I saw? Basically, the crash
occurred when the 'descriptors_included' bit was set, the
'number_of_vc_records' was large, and the length fields read from the
invalid MPEG descriptors was also very large.

Also, is there a simple way to feed Myth a TS that will be processed in
through the same flow as a recording so I could make sure that it
crashes?

Thanks,

--
Douglas Paul


_______________________________________________
mythtv-dev mailing list
mythtv-dev@mythtv.org
http://lists.mythtv.org/mailman/listinfo/mythtv-dev
http://wiki.mythtv.org/Mailing_List_etiquette
MythTV Forums: https://forum.mythtv.org
Re: Patch to avoid crash in 'ShortVirtualChannelTable' parsing [ In reply to ]
On Tue, 21 Jan 2020 at 14:19, Douglas Paul <doug@bogon.ca> wrote:

> Hi Klaas,
>
> On Mon, Jan 20, 2020 at 08:50:02PM +0100, Klaas de Waal wrote:
> > Thanks for your patch. I appreciate that you create the fix and take the
> > trouble to send it. I will look at it.
> > However, it would be very convenient if I can reproduce the problem and
> > test possible fixes myself and I can do that if you would sent me a
> capture
> > of the complete transport stream that contains the problematic channel.
> > This can easily be made with the "MPTS channel" option that is now in
> > MythTV, or with any other appliance, e.g. dvbsnoop, that can be used to
> > make a recording of the full transport stream. Bigger is better, if
> > possible then e.g. 2GB (the maximum that can be sent for free with
> > WeTransfer) or e.g. 4GB (which can be sent as an email attachment to my
> > gmail address) would be perfect.
>
> I will look into this. Hopefully their configuration is still broken.
>
> In the case they have fixed things, would it be of any use if I capture
> a TS and modify it to exercise the failure I saw? Basically, the crash
> occurred when the 'descriptors_included' bit was set, the
> 'number_of_vc_records' was large, and the length fields read from the
> invalid MPEG descriptors was also very large.
>
> Also, is there a simple way to feed Myth a TS that will be processed in
> through the same flow as a recording so I could make sure that it
> crashes?
>
> Also when the currently transmitted TS is fixed then a captured TS is
useful because it can then be used to verify that the patch, when
installed, does not break anything. It is not useful to modify the captured
TS manually to make things crash as this does not prove anything. I did
have a look at the code ad it is easy to see how it goes wrong if the
received data does not contain valid descriptors.

I do test a lot with the mythtv-setup channel scanner and as far as I know
it parses all descriptors in the stream, so that should also crash on your
broken stream.
In mythtv-setup I can do a channel scan from a recorded transport stream by
using a software-only capture card device "External (black box) recorder" ;
this device can be configured to use the bundled utility mythfilerecorder
(see https://www.mythtv.org/wiki/ExternalRecorder#mythfilerecorder) to play
an arbitrary transport stream.
According to https://www.mythtv.org/wiki/Import_recorder the software-only
capture card device "Import test recorder" can be used to make recordings
from a played-back transport stream but I have not yet tried this, mainly
because it is not yet possible to do channel scanning from the "Import test
recorder". But this might be a good occasion to try this.

So yes, your transport stream can possibly reproduce the problem and it can
be used to verify that the patch, when installed, does not cause a
regression. And it enables me to do regression tests with future fixes and
enhancements in MythTV.

Thanks,
Klaas.