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

[Baptiste Coudurier]

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, if you are ok with 65k, then I'll update it.

> 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...

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