[MPlayer-dev-eng] [PATCH] vf_delogo: variable rectangle

[Reimar Döffinger]

On Wed, Feb 09, 2011 at 07:55:03PM +0100, Nicolas George wrote:
> > Are you sure you want to allow it to go backwards?
> 
> Yes. The user may have just hit the left arrow to go back a few seconds.

Right, I forgot we have no way to just reset it or so in case of a seek.

> > > +            rect = realloc(rect, alloc_rect * sizeof(*rect));
> > Potential integer overflow.
> 
> Indeed. What is the solution in that case?
> 
> 	if (alloc_rect * sizeof(*rect) / sizeof(*rect) != alloc_rect)
> 	    abort();

if (alloc_rect > INT_MAX / sizeof(*rect))

Always keep the value you need to validate on its own, otherwise
it gets really difficult to make sure you didn't miss anything
(in this case it also has the very minor advantage of not requiring
a division at runtime, it can be calculated at compile-time).
Since you always multiply by two other checks (like checking that
previous_n * sizeof() < INT_MAX/2) are possible as well.

> There are a lot of similar problems all around the code.

I hope not, these are one of the more serious issues.

> I think what we need is something like mp_realloc_or_die that takes care
> both of improbable failure and integer overflow.

There is a struct_realloc somewhere that solves the integer overflow issue
I think.

> > I'd expect this whole part can be simplified by fscanf, in the worst case
> > by also using getc/ungetc
> 
> I do not see it getting simpler than that.

Probably not.

> +        p = sscanf(line, "%lf %d:%d:%d:%d:%d",
> +                   &ts, &nr->x, &nr->y, &nr->w, &nr->h, &nr->b);

I always forget what exactly sscanf supports, but adding a \n or
a space at the end should help catch some wrong formats e.g.
when it ends with "12something"