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

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

[Nicolas George via ffmpeg-devel]

Marton Balint via ffmpeg-devel (HE12025-08-24):
> >From f8e83bce6269c95fbad90f34434ceb641bf753d5 Mon Sep 17 00:00:00 2001
> From: Marton Balint <cus@passwd.hu>
> Date: Sun, 24 Aug 2025 21:42:54 +0200
> Subject: [PATCH 1/2] avutil/bprint: make av_bprintf use av_vbprintf
> 
> No reason to duplicate the code.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>

No objection.

> ---
>  libavutil/bprint.c | 33 +++++++++------------------------
>  1 file changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/libavutil/bprint.c b/libavutil/bprint.c
> index 4e9571715c..932c03ce50 100644
> --- a/libavutil/bprint.c
> +++ b/libavutil/bprint.c
> @@ -96,35 +96,12 @@ void av_bprint_init_for_buffer(AVBPrint *buf, char *buffer, unsigned size)
>      *buf->str = 0;
>  }
>  
> -void av_bprintf(AVBPrint *buf, const char *fmt, ...)
> -{
> -    unsigned room;
> -    char *dst;
> -    va_list vl;
> -    int extra_len;
> -
> -    while (1) {
> -        room = av_bprint_room(buf);
> -        dst = room ? buf->str + buf->len : NULL;
> -        va_start(vl, fmt);
> -        extra_len = vsnprintf(dst, room, fmt, vl);
> -        va_end(vl);
> -        if (extra_len <= 0)
> -            return;
> -        if (extra_len < room)
> -            break;
> -        if (av_bprint_alloc(buf, extra_len))
> -            break;
> -    }
> -    av_bprint_grow(buf, extra_len);
> -}
> -
>  void av_vbprintf(AVBPrint *buf, const char *fmt, va_list vl_arg)
>  {
>      unsigned room;
>      char *dst;

> -    int extra_len;
>      va_list vl;
> +    int extra_len;

Uh?

>  
>      while (1) {
>          room = av_bprint_room(buf);
> @@ -142,6 +119,14 @@ void av_vbprintf(AVBPrint *buf, const char *fmt, va_list vl_arg)
>      av_bprint_grow(buf, extra_len);
>  }
>  
> +void av_bprintf(AVBPrint *buf, const char *fmt, ...)
> +{
> +    va_list vl;
> +    va_start(vl, fmt);
> +    av_vbprintf(buf, fmt, vl);
> +    va_end(vl);
> +}
> +
>  void av_bprint_chars(AVBPrint *buf, char c, unsigned n)
>  {
>      unsigned room, real_n;

Regards,

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

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

[Nicolas George via ffmpeg-devel]

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?

>  
>      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?

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

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

>          /* 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.

Regards,

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

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

[Marton Balint via ffmpeg-devel]

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

> Marton Balint via ffmpeg-devel (HE12025-08-24):
>>> From f8e83bce6269c95fbad90f34434ceb641bf753d5 Mon Sep 17 00:00:00 2001
>> From: Marton Balint <cus@passwd.hu>
>> Date: Sun, 24 Aug 2025 21:42:54 +0200
>> Subject: [PATCH 1/2] avutil/bprint: make av_bprintf use av_vbprintf
>>
>> No reason to duplicate the code.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>
> No objection.
>
>> ---
>>  libavutil/bprint.c | 33 +++++++++------------------------
>>  1 file changed, 9 insertions(+), 24 deletions(-)
>>
>> diff --git a/libavutil/bprint.c b/libavutil/bprint.c
>> index 4e9571715c..932c03ce50 100644
>> --- a/libavutil/bprint.c
>> +++ b/libavutil/bprint.c
>> @@ -96,35 +96,12 @@ void av_bprint_init_for_buffer(AVBPrint *buf, char *buffer, unsigned size)
>>      *buf->str = 0;
>>  }
>>
>> -void av_bprintf(AVBPrint *buf, const char *fmt, ...)
>> -{
>> -    unsigned room;
>> -    char *dst;
>> -    va_list vl;
>> -    int extra_len;
>> -
>> -    while (1) {
>> -        room = av_bprint_room(buf);
>> -        dst = room ? buf->str + buf->len : NULL;
>> -        va_start(vl, fmt);
>> -        extra_len = vsnprintf(dst, room, fmt, vl);
>> -        va_end(vl);
>> -        if (extra_len <= 0)
>> -            return;
>> -        if (extra_len < room)
>> -            break;
>> -        if (av_bprint_alloc(buf, extra_len))
>> -            break;
>> -    }
>> -    av_bprint_grow(buf, extra_len);
>> -}
>> -
>>  void av_vbprintf(AVBPrint *buf, const char *fmt, va_list vl_arg)
>>  {
>>      unsigned room;
>>      char *dst;
>
>> -    int extra_len;
>>      va_list vl;
>> +    int extra_len;
>
> Uh?

This change is diff-algorithm dependant, but I have already force pushed a 
new version which gets rid of this.

>
>>
>>      while (1) {
>>          room = av_bprint_room(buf);
>> @@ -142,6 +119,14 @@ void av_vbprintf(AVBPrint *buf, const char *fmt, va_list vl_arg)
>>      av_bprint_grow(buf, extra_len);
>>  }
>>
>> +void av_bprintf(AVBPrint *buf, const char *fmt, ...)
>> +{
>> +    va_list vl;
>> +    va_start(vl, fmt);
>> +    av_vbprintf(buf, fmt, vl);
>> +    va_end(vl);
>> +}
>> +
>>  void av_bprint_chars(AVBPrint *buf, char c, unsigned n)
>>  {
>>      unsigned room, real_n;
>
> Regards,

Thanks,
Marton
_______________________________________________
ffmpeg-devel mailing list -- ffmpeg-devel@ffmpeg.org
To unsubscribe send an email to ffmpeg-devel-leave@ffmpeg.org

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

[Marton Balint via ffmpeg-devel]

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