[FFmpeg-devel] [PATCH] rmdec: make sure we actually have a buffer before writing into it

[Kostya]

On Sun, Sep 13, 2009 at 11:13:51AM +0200, Reimar D?ffinger wrote:
> On Sun, Sep 13, 2009 at 11:21:24AM +0300, Kostya wrote:
> > On Sun, Sep 13, 2009 at 10:02:06AM +0200, Reimar D?ffinger wrote:
> > > On Sun, Sep 13, 2009 at 10:33:34AM +0300, Kostya wrote:
> > > > On Sat, Sep 12, 2009 at 10:27:45PM +0200, Reimar D?ffinger wrote:
> > > > > rm_assemble_video_frame may write into vst->pkt.data even though that
> > > > > one is NULL because we just returned a packet and have not yet allocated
> > > > > a new one.
> > > > > There are loads of ways to fix that, and possibly even some better/more
> > > > > error resilient ones, but since I don't know the rm format that well I
> > > > > propose this rather simple one, which sets vst->slices to 0 in addition
> > > > > to vst->pkt.size/data and thus takes advantage of an existing check.
> > > > > Index: libavformat/rmdec.c
> > > > > ===================================================================
> > > > > --- libavformat/rmdec.c (revision 19824)
> > > > > +++ libavformat/rmdec.c (working copy)
> > > > > @@ -637,6 +637,7 @@
> > > > >          pkt->size = vst->videobufpos + 8*(vst->cur_slice - vst->slices);
> > > > >          pkt->pts = AV_NOPTS_VALUE;
> > > > >          pkt->pos = vst->pktpos;
> > > > > +        vst->slices = 0;
> > > > >          return 0;
> > > > >      }
> > > > 
> > > > Hmm, that does not seem correct since packet is allocated when first
> > > > part of it (i.e. slice number = 1) is seen. If we don't get it, packet
> > > > will be corrupted.
> > > 
> > > Of course it is corrupted, but that is no excuse for crashing!
> > 
> > What about just printing error message and not crashing? Otherwise it
> > looks like demuxer silently produces wrong packets.
> 
> Well, not to print a message is a decision already made. The code to
> handle this is already there already (e.g. when the first call to
> rm_assemble_video_frame does not allocate a frame it will not crash,
> the code protects against that - it is just the code I changed that
> seems to break assumptions by not resetting vst->slices when it resets
> vst->pkt).
> I don't know why the ++vst->cur_slice > vst->slices and vst->videobufpos
> + len > vst->videobufsize neither return an error nor print an error
> message, but if that is the wrong behaviour they still should be fixed
> independently and not as part of a crash fix.

I suspect that condition may be triggered but since it does not skip
packet body, it just resychronises in wrong point.
Feel free to apply your patch though.
 
> > > This is only supposed to fix the crash with corrupted/unsupported files,
> > > not make them work - that one is a different issue.
> > > Of course it might be possible to do both with one patch, but this seems
> > > safer to me.
> > 
> > I'd prefer supproting them instead :)
> 
> Of course but I consider that rather independent.

Who knows...