[FFmpeg-devel] [PATCH] Workaround for MPEG-TS crashes

[Baptiste Coudurier]

On 9/15/2009 1:29 AM, Michael Niedermayer wrote:
> On Mon, Sep 14, 2009 at 10:27:43PM -0700, Baptiste Coudurier wrote:
>> On 09/14/2009 07:43 PM, Michael Niedermayer wrote:
>>> On Tue, Sep 15, 2009 at 04:27:32AM +0200, Michael Niedermayer wrote:
>>>> On Mon, Sep 14, 2009 at 06:45:36PM -0700, Baptiste Coudurier wrote:
>>>>> Hi Michael,
>>>>>
>>>>> On 09/14/2009 06:14 PM, Michael Niedermayer wrote:
>>>>>> On Mon, Sep 14, 2009 at 11:52:39AM -0700, Baptiste Coudurier wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 09/14/2009 10:25 AM, Dan Dennedy wrote:
>>>>>>>> Hi
>>>>>>>>
>>>>>>>> On Sun, Sep 13, 2009 at 3:00 AM, Ivan Schreter<schreter at gmx.net>
>>>>>>>> wrote:
>>>>>>>>> Hi Baptiste,
>>>>>>>>>
>>>>>>>>> several people reported crashes when working with MPEG-TS files. I
>>>>>>>>> suppose,
>>>>>>>>> those files are either really or subtly broken and MPEG-TS format
>>>>>>>>> handler
>>>>>>>>> doesn't handle them gracefully.
>>>>>>>>>
>>>>>>>>> With this patch, the crashes in the samples I have seem to be gone:
>>>>>>>>>
>>>>>>>>
>>>>>>>> In addition, I ran into some small negative lengths passed to memcpy:
>>>>>>>>
>>>>>>>> Index: libavformat/mpegts.c
>>>>>>>> ===================================================================
>>>>>>>> --- libavformat/mpegts.c        (revision 19804)
>>>>>>>> +++ libavformat/mpegts.c        (working copy)
>>>>>>>> @@ -915,10 +915,12 @@
>>>>>>>>                  len = PES_START_SIZE - pes->data_index;
>>>>>>>>                  if (len>     buf_size)
>>>>>>>>                      len = buf_size;
>>>>>>>> -            memcpy(pes->header + pes->data_index, p, len);
>>>>>>>> -            pes->data_index += len;
>>>>>>>> -            p += len;
>>>>>>>> -            buf_size -= len;
>>>>>>>> +            if (len>     0) {
>>>>>>>> +                memcpy(pes->header + pes->data_index, p, len);
>>>>>>>> +                pes->data_index += len;
>>>>>>>> +                p += len;
>>>>>>>> +                buf_size -= len;
>>>>>>>> +            }
>>>>>>>
>>>>>>> Well, len<    0 is an error IMHO. I'd like to have a sample though,
>>>>>>> this should not happen.
>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>                  if (pes->data_index == PES_START_SIZE) {
>>>>>>>>                      /* we got all the PES or section header. We can
>>>>>>>> now
>>>>>>>>                         decide */
>>>>>>>>
>>>>>>>>> However, it's IMHO just a workaround and the real root cause of the
>>>>>>>>> problem
>>>>>>>>> should be found. At this time, pes->buffer is NULL, but data for PES
>>>>>>>>> packet
>>>>>>>>> are coming in =>     crash.
>>>>>>>>>
>>>>>>>>> Crashing sample is here:
>>>>>>>>> http://dennedy.org/~ddennedy/dvgrab-2009.03.28_19-07-22.m2t
>>>>>>>>> Playable sample is here:
>>>>>>>>> http://dennedy.org/~ddennedy/dvgrab-2009.03.28_19-06-41.m2t
>>>>>>>>>
>>>>>>>>> Both samples seem to be seriously broken at the beginning. They
>>>>>>>>> originated
>>>>>>>>> from a HDV-capable camcorder (MPEG-TS containing MPEG-2 video).
>>>>>>>>
>>>>>>>> Last night, after some serious testing and soul searching, I
>>>>>>>> discovered some file system corruption or bad file transfers when
>>>>>>>> archiving this material to external HDD. There were some HDV test
>>>>>>>> samples that were failing that I _know_ had worked well in the past.
>>>>>>>> I
>>>>>>>> had a backup of those on my network, and when I tested them over the
>>>>>>>> network, they worked fine! An e2fsck on that disk did indeed find
>>>>>>>> some
>>>>>>>> problems that the "automatic repair" option could not fix.
>>>>>>>>
>>>>>>>> So, the samples above are definitely not to be considered playable,
>>>>>>>> but I can confirm older versions of ffmpeg (0.5) at least did not
>>>>>>>> segfault on this corrupt input. With these two changes, it is much
>>>>>>>> more stable.
>>>>>>>>
>>>>>>>
>>>>>>> These 2 files can be played if MAX_RESYNC_SIZE is increased to 65536.
>>>>>>> Note that mplayer reads them.
>>>>>>>
>>>>>>> Is 65k reasonable ?
>>>>>>
>>>>>> i didnt follow this thread but if i understand the code correctly then
>>>>>> UINT64_MAX seems more reasonable to me ;)
>>>>>> whats the point in not continuing to search for a packet but failing
>>>>>> fatally, which i think is what the code would do?
>>>>>
>>>>> Avoiding reading 100MB on a slow link ?
>>>
>>> btw, forgot to ask
>>> mpeg-TS over a slow link?
>>>
>>> is mpeg-ts used on anything where 64k is not just a fraction of a second?
>>>
>>
>> Well, mpeg-ts is a pretty nice transport format IMHO, I can easily see it
>> used to transport live streams on the net :)
>
> it has quite a bit of overhead that seems to make it a bad choice to me
> if you have a slow link ...
>

It has some overhead, however it is pretty robust. All IPTV feeds are 
basically mpeg-ts over multicast udp. 65k is reasonable indeed and 
should not cause problems.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org