[FFmpeg-devel] Re: [PATCH] avutil/bprint: fix av_bprint_strftime with %p format string reporting truncated output (PR #20330)

[Marton Balint via ffmpeg-devel <ffmpeg-devel@ffmpeg.org>]

On Tue, 26 Aug 2025, Nicolas George via ffmpeg-devel wrote:

> Marton Balint via ffmpeg-devel (HE12025-08-24):
>>> From 4532caf820dcb2bc2cbac3d4e782ad27cf9fd76b Mon Sep 17 00:00:00 2001
>> From: Marton Balint <cus@passwd.hu>
>> Date: Sun, 24 Aug 2025 21:35:41 +0200
>> Subject: [PATCH 2/2] avutil/bprint: fix av_bprint_strftime with %p format
>>  string reporting truncated output
>>
>> strftime returns 0 in case of an empty output (e.g. %p format string with some
>> locales), there is no way to distinguish this from a buffer-too-small error
>> condition. So we must use some heuristics to handle this case, and not consume
>> INT_MAX RAM and falsely report a truncated output.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavutil/bprint.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavutil/bprint.c b/libavutil/bprint.c
>> index 932c03ce50..fa244b2e04 100644
>> --- a/libavutil/bprint.c
>> +++ b/libavutil/bprint.c
>> @@ -167,6 +167,7 @@ void av_bprint_strftime(AVBPrint *buf, const char *fmt, const struct tm *tm)
>>  {
>>      unsigned room;
>>      size_t l;
>
>> +    size_t fmt_len = 0;
>
> Why not set it here once and for all?

I just thought that for the typical case we can spare an strlen() call 
this way.

>
>>
>>      if (!*fmt)
>>          return;
>> @@ -174,9 +175,16 @@ void av_bprint_strftime(AVBPrint *buf, const char *fmt, const struct tm *tm)
>>          room = av_bprint_room(buf);
>>          if (room && (l = strftime(buf->str + buf->len, room, fmt, tm)))
>>              break;
>> +
>> +        if (!fmt_len)
>> +            fmt_len = strlen(fmt);
>> +        /* A 256x space requirement for output is unlikely, so it is likely empty */
>> +        if (room >> 8 > fmt_len)
>> +            break;
>> +
>
> IIUC, at this point l=0 means either the buffer is too small or the
> output is empty, and you distinguish between the cases by checking that
> there is plenty of room, 256 chars for any char in the buffer?

Yes.

>
> I think a lower margin should be ok, >>4 or >>5.

I'd rather keep it >>8, because strftime format strings can also 
have width specifiers, so something like "%80c" should work. Obviously 
this is always going to be a heuristics, but I wanted to be careful and 
reject only the very unlikely/insane format strings...

>
> OTOH, I think a comment would be a good idea.

There is one, but OK, I will make the existing comment a bit more 
verbose.

>
>>          /* strftime does not tell us how much room it would need: let us
>>             retry with twice as much until the buffer is large enough */
>> -        room = !room ? strlen(fmt) + 1 :
>> +        room = !room ? fmt_len + 1 :
>>                 room <= INT_MAX / 2 ? room * 2 : INT_MAX;
>>          if (av_bprint_alloc(buf, room)) {
>>              /* impossible to grow, try to manage something useful anyway */
>
> Or we could write our own strftime, not localized.

Some API users might depend on our strftime functions returning localized 
strings...

Regards,
Marton


>
> Regards,
>
> --
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
> To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org
>
_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org