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

[Michael Niedermayer]

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?


> if you are ok with 65k, then I'll update it.

iam, but its your code anyway, its just in mpegts ...


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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090915/2f86a137/attachment.pgp>