[FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[Kacper Michajłow]

av_get_token() allocates an output buffer with the same size as the
input. Generally, this is harmless, but when the input string is large
and consists of many small tokens, calling av_get_token() repeatedly to
extract all tokens will significantly amplify memory allocations.

To fix this, after obtaining the return value, simply realloc the buffer
to the actual size needed for output string.

Fixes OOM when parsing filter graph string.
Fixes OSS-Fuzz: 394983446

Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
---
 libavutil/avstring.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavutil/avstring.c b/libavutil/avstring.c
index 875eb691db..b4266aefe5 100644
--- a/libavutil/avstring.c
+++ b/libavutil/avstring.c
@@ -142,7 +142,7 @@ end:
 
 char *av_get_token(const char **buf, const char *term)
 {
-    char *out     = av_malloc(strlen(*buf) + 1);
+    char *out     = av_realloc(NULL, strlen(*buf) + 1);
     char *ret     = out, *end = out;
     const char *p = *buf;
     if (!out)
@@ -172,7 +172,7 @@ char *av_get_token(const char **buf, const char *term)
 
     *buf = p;
 
-    return ret;
+    return av_realloc(ret, out - ret + 2);
 }
 
 char *av_strtok(char *s, const char *delim, char **saveptr)
-- 
2.47.2

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[Andreas Rheinhardt]

Kacper Michajłow:
> av_get_token() allocates an output buffer with the same size as the
> input. Generally, this is harmless, but when the input string is large
> and consists of many small tokens, calling av_get_token() repeatedly to
> extract all tokens will significantly amplify memory allocations.
> 
> To fix this, after obtaining the return value, simply realloc the buffer
> to the actual size needed for output string.
> 
> Fixes OOM when parsing filter graph string.
> Fixes OSS-Fuzz: 394983446
> 
> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> ---
>  libavutil/avstring.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> index 875eb691db..b4266aefe5 100644
> --- a/libavutil/avstring.c
> +++ b/libavutil/avstring.c
> @@ -142,7 +142,7 @@ end:
>  
>  char *av_get_token(const char **buf, const char *term)
>  {
> -    char *out     = av_malloc(strlen(*buf) + 1);
> +    char *out     = av_realloc(NULL, strlen(*buf) + 1);

I thought pointers provided by av_malloc could be used with
av_realloc()? Or is it to avoid the unnecessary alignment provided by
av_malloc()?

>      char *ret     = out, *end = out;
>      const char *p = *buf;
>      if (!out)
> @@ -172,7 +172,7 @@ char *av_get_token(const char **buf, const char *term)
>  
>      *buf = p;
>  
> -    return ret;
> +    return av_realloc(ret, out - ret + 2);

You seem to presume that av_realloc() can't fail when used for
shrinking. But this is not documented.

>  }
>  
>  char *av_strtok(char *s, const char *delim, char **saveptr)

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[Kacper Michajlow]

On Fri, 4 Jul 2025 at 20:22, Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> Kacper Michajłow:
> > av_get_token() allocates an output buffer with the same size as the
> > input. Generally, this is harmless, but when the input string is large
> > and consists of many small tokens, calling av_get_token() repeatedly to
> > extract all tokens will significantly amplify memory allocations.
> >
> > To fix this, after obtaining the return value, simply realloc the buffer
> > to the actual size needed for output string.
> >
> > Fixes OOM when parsing filter graph string.
> > Fixes OSS-Fuzz: 394983446
> >
> > Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> > ---
> >  libavutil/avstring.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> > index 875eb691db..b4266aefe5 100644
> > --- a/libavutil/avstring.c
> > +++ b/libavutil/avstring.c
> > @@ -142,7 +142,7 @@ end:
> >
> >  char *av_get_token(const char **buf, const char *term)
> >  {
> > -    char *out     = av_malloc(strlen(*buf) + 1);
> > +    char *out     = av_realloc(NULL, strlen(*buf) + 1);
>
> I thought pointers provided by av_malloc could be used with
> av_realloc()? Or is it to avoid the unnecessary alignment provided by
> av_malloc()?

I was thinking about this. But the docs, of av_realloc() say

 * @param ptr  Pointer to a memory block already allocated with
 *             av_realloc() or `NULL`

so to honor this, I changed to av_realloc(NULL). I've looked at other
uses and it seems to be a pattern that is used for cases like this
one.

> >      char *ret     = out, *end = out;
> >      const char *p = *buf;
> >      if (!out)
> > @@ -172,7 +172,7 @@ char *av_get_token(const char **buf, const char *term)
> >
> >      *buf = p;
> >
> > -    return ret;
> > +    return av_realloc(ret, out - ret + 2);
>
> You seem to presume that av_realloc() can't fail when used for
> shrinking. But this is not documented.

This function returns a pointer. So if it fails, returning NULL is
valid. Unless you mean that we add another possible point of failure?
I don't think it matters, if we are already close to OOM state, this
small alloc wouldn't be a difference maker.

- Kacper

> >  }
> >
> >  char *av_strtok(char *s, const char *delim, char **saveptr)
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[Kacper Michajlow]

On Fri, 4 Jul 2025 at 20:37, Kacper Michajlow <kasper93@gmail.com> wrote:
>
> On Fri, 4 Jul 2025 at 20:22, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
> >
> > Kacper Michajłow:
> > > av_get_token() allocates an output buffer with the same size as the
> > > input. Generally, this is harmless, but when the input string is large
> > > and consists of many small tokens, calling av_get_token() repeatedly to
> > > extract all tokens will significantly amplify memory allocations.
> > >
> > > To fix this, after obtaining the return value, simply realloc the buffer
> > > to the actual size needed for output string.
> > >
> > > Fixes OOM when parsing filter graph string.
> > > Fixes OSS-Fuzz: 394983446
> > >
> > > Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
> > > ---
> > >  libavutil/avstring.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libavutil/avstring.c b/libavutil/avstring.c
> > > index 875eb691db..b4266aefe5 100644
> > > --- a/libavutil/avstring.c
> > > +++ b/libavutil/avstring.c
> > > @@ -142,7 +142,7 @@ end:
> > >
> > >  char *av_get_token(const char **buf, const char *term)
> > >  {
> > > -    char *out     = av_malloc(strlen(*buf) + 1);
> > > +    char *out     = av_realloc(NULL, strlen(*buf) + 1);
> >
> > I thought pointers provided by av_malloc could be used with
> > av_realloc()? Or is it to avoid the unnecessary alignment provided by
> > av_malloc()?
>
> I was thinking about this. But the docs, of av_realloc() say
>
>  * @param ptr  Pointer to a memory block already allocated with
>  *             av_realloc() or `NULL`
>
> so to honor this, I changed to av_realloc(NULL). I've looked at other
> uses and it seems to be a pattern that is used for cases like this
> one.
>
> > >      char *ret     = out, *end = out;
> > >      const char *p = *buf;
> > >      if (!out)
> > > @@ -172,7 +172,7 @@ char *av_get_token(const char **buf, const char *term)
> > >
> > >      *buf = p;
> > >
> > > -    return ret;
> > > +    return av_realloc(ret, out - ret + 2);
> >
> > You seem to presume that av_realloc() can't fail when used for
> > shrinking. But this is not documented.
>
> This function returns a pointer. So if it fails, returning NULL is
> valid. Unless you mean that we add another possible point of failure?
> I don't think it matters, if we are already close to OOM state, this
> small alloc wouldn't be a difference maker.

Well, actually if we want to handle OOM here gracefully, I will update
to return the original pointer, instead of leaking it.

- Kacper
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH] avutil/avstring: shrink allocation from av_get_token to fit token

[Andreas Rheinhardt]

Kacper Michajlow:
> On Fri, 4 Jul 2025 at 20:22, Andreas Rheinhardt
> <andreas.rheinhardt@outlook.com> wrote:
>>
>> Kacper Michajłow:
>>> av_get_token() allocates an output buffer with the same size as the
>>> input. Generally, this is harmless, but when the input string is large
>>> and consists of many small tokens, calling av_get_token() repeatedly to
>>> extract all tokens will significantly amplify memory allocations.
>>>
>>> To fix this, after obtaining the return value, simply realloc the buffer
>>> to the actual size needed for output string.
>>>
>>> Fixes OOM when parsing filter graph string.
>>> Fixes OSS-Fuzz: 394983446
>>>
>>> Signed-off-by: Kacper Michajłow <kasper93@gmail.com>
>>> ---
>>>  libavutil/avstring.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavutil/avstring.c b/libavutil/avstring.c
>>> index 875eb691db..b4266aefe5 100644
>>> --- a/libavutil/avstring.c
>>> +++ b/libavutil/avstring.c
>>> @@ -142,7 +142,7 @@ end:
>>>
>>>  char *av_get_token(const char **buf, const char *term)
>>>  {
>>> -    char *out     = av_malloc(strlen(*buf) + 1);
>>> +    char *out     = av_realloc(NULL, strlen(*buf) + 1);
>>
>> I thought pointers provided by av_malloc could be used with
>> av_realloc()? Or is it to avoid the unnecessary alignment provided by
>> av_malloc()?
> 
> I was thinking about this. But the docs, of av_realloc() say
> 
>  * @param ptr  Pointer to a memory block already allocated with
>  *             av_realloc() or `NULL`
> 
> so to honor this, I changed to av_realloc(NULL). I've looked at other
> uses and it seems to be a pattern that is used for cases like this
> one.
> 
>>>      char *ret     = out, *end = out;
>>>      const char *p = *buf;
>>>      if (!out)
>>> @@ -172,7 +172,7 @@ char *av_get_token(const char **buf, const char *term)
>>>
>>>      *buf = p;
>>>
>>> -    return ret;
>>> +    return av_realloc(ret, out - ret + 2);
>>
>> You seem to presume that av_realloc() can't fail when used for
>> shrinking. But this is not documented.
> 
> This function returns a pointer. So if it fails, returning NULL is
> valid. Unless you mean that we add another possible point of failure?
> I don't think it matters, if we are already close to OOM state, this
> small alloc wouldn't be a difference maker.
> 

There is a leak in case of reallocation failure. Which is unacceptable.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".