[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]

Reimar Döffinger wrote:

> On Mon, Oct 27, 2008 at 11:51:22PM +0100, aurel wrote:
> > Log:
> > Factorize vobsub idx/extradata handling.
> 
> I know, I forgot to comment further commenting on this but was it really
> impossible to do this in a few smaller steps?

I don't see obvious smaller step... Except maybe adding the new idx
parsing first and then removing the old one, but I don't think this
would be easier to review.

> > @@ -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).
Do you want me to add a check for realloc return value ?

> > @@ -1115,11 +994,10 @@ vobsub_open(const char *const name,const
> >  		    /* NOOP */ ;
> >  		rar_close(fd);
> >  	    }
> > -	    /* if no palette in .idx then use custom colors */
> > -	    if ((vob->custom == 0)&&(vob->have_palette!=1))
> > -		vob->custom = 1;
> > -	    if (spu && vob->orig_frame_width && vob->orig_frame_height)
> > -	      *spu = spudec_new_scaled_vobsub(vob->palette, vob->cuspal, vob->custom, vob->orig_frame_width, vob->orig_frame_height);
> > +	    if (spu)
> > +	      *spu = spudec_new_scaled(vob->palette, vob->orig_frame_width, vob->orig_frame_height, vob->extradata, vob->extradata_len);
> > +	    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.

Aurel