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

[Michael Niedermayer]

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?
a EAGAIN might make sense but a hard error IMHO not ...


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

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- 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/c537e679/attachment.pgp>