[MPlayer-cvslog] r27841 - in trunk: libmpdemux/demux_mkv.c libmpdemux/stheader.h mencoder.c mplayer.c spudec.c spudec.h vobsub.c vobsub.h

[Aurelien Jacobs]

On Fri, 31 Oct 2008 12:06:06 +0100
Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:

> On Thu, Oct 30, 2008 at 10:59:34PM +0100, Aurelien Jacobs wrote:
> > > > @@ -973,6 +855,12 @@ vobsub_parse_one_line(vobsub_t *vob, rar
> > > >  	if (line_size < 0) {
> > > >  	    break;
> > > >  	}
> > > > +
> > > > +	vob->extradata = realloc(vob->extradata, vob->extradata_len+line_size+1);
> > > > +	memcpy(vob->extradata+vob->extradata_len, line, line_size);
> > > 
> > > This seems very suspicious, are you really sure this can not go wrong?
> > 
> > I can't imagine how this could go wrong.
> > Well, except maybe in case of out-of-memory failure (but that certainly
> > won't be the first point of failure in this situation).
> 
> vob->extradata_len+line_size+1 can never overflow?

Ah. I see what you mean.
Well, idx files are pretty small, so with real world files, there is
no way it can overflow.
But it would indeed be possible to craft a hugh multi-gigabytes idx
file to generate an overflow.
Attached patch adds an extra sanity check.

> > > > +	    if (vob->extradata)
> > > > +	      free(vob->extradata);
> > > 
> > > Same about this, can there really never be a double free?
> > 
> > I don't think so.
> > Here is a quick summary of what this function does regarding extradata:
> > 
> > vobsub_open()
> > {
> >   vobsub_t *vob = calloc(1, sizeof(vobsub_t));
> >   while (vobsub_parse_one_line(vob, ...))  //ie. realloc(vob->extradata)
> >   {  }
> >   spudec_new_scaled(..., vob->extradata, vob->extradata_len);
> >   if (vob->extradata)
> >     free(vob->extradata);
> > }
> > 
> > And extradata is never touched from any other place. So this
> > looks pretty safe.
> 
> Well, I usually use an if() around free to indicate an optional field
> that may contain data or be NULL,

That's exactly the case here. extradata may still be NULL here, if
no line were read.

> and in that case to free it without setting it to NULL again would
> be wrong.

Except if this field won't ever be used again.

> I am wondering a bit if it is such a good idea to have the extradata
> inside the vob, since it is handled different from the other fields,
> e.g. it is only used internally in vobsub_open, it is not freed by
> vobsub_close...

Second attached patch moves extradata out of the vob, but it is
ugly IMHO.

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vobsub_check.diff
Type: text/x-diff
Size: 386 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/attachments/20081101/9f3e25e8/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vobsub_extradata.diff
Type: text/x-diff
Size: 2353 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/attachments/20081101/9f3e25e8/attachment-0001.diff>