[FFmpeg-devel] [PATCH] bprint: Remove custom vsnprintf

[Ronald S. Bultje]

Hi,

On Sun, Sep 9, 2012 at 12:22 PM, Nicolas George
<nicolas.george at normalesup.org> wrote:
> Le quartidi 24 fructidor, an CCXX, Ronald S. Bultje a écrit :
>> Speaking of - ffmpeg relies on the opposite behaviour in quite a few
>> cases. E.g. ffserver.c:
>>
>>     q += snprintf(q, q - (char *) c->buffer + c->buffer_size,
>> "HTTP/1.0 200 OK\r\n");
>>     q += snprintf(q, q - (char *) c->buffer + c->buffer_size, "Pragma:
>> no-cache\r\n");
>>         q += snprintf(q, q - (char *) c->buffer + c->buffer_size,
>> "Server: Cougar 4.1.0.3923\r\nCache-Control: no-cache\r\nPragma:
>> client-id=%d\r\nPragma: features=\"broadcast\"\r\n",
>> c->wmp_client_id);
>>     q += snprintf(q, q - (char *) c->buffer + c->buffer_size,
>> "Content-Type: %s\r\n", mime_type);
>>     q += snprintf(q, q - (char *) c->buffer + c->buffer_size, "\r\n");
>>     q += snprintf(q, c->buffer_size,
>
> I would not say that ffmpeg relies on the opposite behaviour: rather, it
> ignores the problem completely.
>
>> All of these are exploitable bugs in violation of the C spec.
>
> You are completely right on that. bprint was written specifically to replace
> this kind of invalid and exploitable uses of snprintf, with an easy and
> lightweight API (avio_open_dyn_buf is similar, but its API is IMHO much more
> cumbersome). Finding, eliminating and testing all of them takes time,
> unfortunately.

I doubt that introducing a hardcoded upper bound on the return value
of your vsnprintf replacement is a great replacement for this
(admittedly) buggy behaviour.

(Not that I have a better idea, I'm just saying that from a
design-point-of-view, swapping one bug for another when introducing a
completely new API isn't great.)

Ronald