[FFmpeg-devel] [PATCH] lavu: add av_bprintf and related.

[Stefano Sabatini]

On date Sunday 2012-03-11 23:28:25 +0100, Nicolas George encoded:
> Le duodi 22 ventôse, an CCXX, Stefano Sabatini a écrit :
> > nit+++: maybe len_add to stress the fact that it represents a length
> 
> I'll try not to forget to change it.
> 
> > > +        room = av_bprint_room(buf);
> > > +        dst = room ? buf->str + buf->len : NULL;
> > > +        va_start(vl, fmt);
> > > +        add = vsnprintf(dst, room, fmt, vl);
> > 
> > > +    /* arbitrary margin to avoid small overflows */
> > > +    add = FFMIN(add, UINT_MAX - 5 - buf->len);
> > 
> > I'm not sure I understand the need for this check.
> > 
> > room provides the available size left in the buffer, since the max
> > size is UINT_MAX then we're sure that:
> > len+room <= UINT_MAX
> > 
> > and so since add+1 <= room then:
> > len+add+1 <= UINT_MAX.
> 
> I think you are missing the fact that the return value of snprintf is the
> length of the string that would have been written if the buffer had been
> large enough.
> 
> In other words, snprintf(buf, 5, "Hello World!") returns 12, not 4 or 5.
> 
> Therefore, since add can be greater than room, len+add can overflow.
> 

So why not:
add = FFMIN(add, room-1);
?

> Furthermore, I want the user to be able to be able to write "len+3", for
> small values of 3, without bothering to check for overflow: 5 is a nice
> margin: a 4-bytes length field plus the string plus a 0-terminator.

What I'm concerned is that you can have len expressing a value
different from strlen(buf.str).

> Of course, all this is purely theoretical: in practice, I do not expect any
> strings with this API to reach even UINT_MAX/10.

Of course but these are the kind of limit situations which are looked
after for exploitations.
-- 
FFmpeg = Faithless & Faithful Maxi Pitiful Exciting Gigant