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

[Baptiste Coudurier]

On 09/14/2009 07:27 PM, 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 ?
>> IMHO we need a limit,
>
> why cant the user abort it when he considers it not worth waiting any
> longer?

Yes ideally he could, in current situation he cannot unfortunately.
Also user might not be present to stop it, like in ffserver.

>> if you are ok with 65k, then I'll update it.
>
> iam, but its your code anyway, its just in mpegts ...

Yes, I like having other's opinion as well :)

>>
>>> a EAGAIN might make sense but a hard error IMHO not ...
>>
>> Well EAGAIN seems interesting, it would be good to indicate there was an
>> error somewhere though...
>
> yes ...

Let's think about a mechanism then, I remember Reimar suggested it in 
the mov demuxer as well.

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