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

[Michael Niedermayer]

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?

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

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- 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/b62859cf/attachment.pgp>