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

[Reimar Döffinger]

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?

> > > +	    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, and in that case to free it without
setting it to NULL again would be wrong.
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...
Maybe I can find some time to suggest some code cleanups, like merging
the two almost identical vobsub_parse_ifo (with the second parameter
just ifo ? ifo : buf etc.

Greetings,
Reimar Döffinger