diff mbox

[FFmpeg-devel] avcodec/ass: Fix a memory leak defect.

Message ID CADpf0PRVnS=MKo425Y0C2S5QDwV9AX3SqMdfS+4gZVJkmHdhkQ@mail.gmail.com
State Superseded
Headers show

Commit Message

Gang Fan(范刚) Feb. 12, 2018, 10:55 a.m. UTC
There is a potential memory leak bug in file ass_split.c, here is the
description.

A piece of memory is allocated on line 283. When executing the loop twice
and if the av_realloc_array returns null the function returns without
freeing the memory pointed by order.

Suggested fix:
free(order) before return NULL; on line 284

Reference Ticket: https://trac.ffmpeg.org/ticket/7019#comment:1

Thanks
Gang
Sbrella


From 6850fc3a6562b4f5fb92e72eed125e057ad975ae Mon Sep 17 00:00:00 2001
From: Fan Gang <fangang@sbrella.com>
Date: Mon, 12 Feb 2018 18:46:20 +0800
Subject: [PATCH] avcodec/ass: Fix a memory leak defect when realloc fails.

---
 libavcodec/ass_split.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

                     order = tmp;
                     order[*number] = -1;
                     for (i=0; section->fields[i].name; i++)

Comments

Hendrik Leppkes Feb. 12, 2018, 11:49 a.m. UTC | #1
On Mon, Feb 12, 2018 at 11:55 AM, Gang Fan(范刚) <fan.gang.cn@gmail.com> wrote:
> There is a potential memory leak bug in file ass_split.c, here is the
> description.
>
> A piece of memory is allocated on line 283. When executing the loop twice
> and if the av_realloc_array returns null the function returns without
> freeing the memory pointed by order.
>
> Suggested fix:
> free(order) before return NULL; on line 284
>
> Reference Ticket: https://trac.ffmpeg.org/ticket/7019#comment:1
>
> Thanks
> Gang
> Sbrella
>
>
> From 6850fc3a6562b4f5fb92e72eed125e057ad975ae Mon Sep 17 00:00:00 2001
> From: Fan Gang <fangang@sbrella.com>
> Date: Mon, 12 Feb 2018 18:46:20 +0800
> Subject: [PATCH] avcodec/ass: Fix a memory leak defect when realloc fails.
>
> ---
>  libavcodec/ass_split.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/ass_split.c b/libavcodec/ass_split.c
> index 872528b..c7eb07d 100644
> --- a/libavcodec/ass_split.c
> +++ b/libavcodec/ass_split.c
> @@ -280,8 +280,10 @@ static const char *ass_split_section(ASSSplitContext
> *ctx, const char *buf)
>                  while (!is_eol(*buf)) {
>                      buf = skip_space(buf);
>                      len = strcspn(buf, ", \r\n");
> -                    if (!(tmp = av_realloc_array(order, (*number + 1),
> sizeof(*order))))
> +                    if (!(tmp = av_realloc_array(order, (*number + 1),
> sizeof(*order)))){
> +                        free(order);
>                          return NULL;
> +                    }
>                      order = tmp;
>                      order[*number] = -1;
>                      for (i=0; section->fields[i].name; i++)
> --

You would need to use av_free instead of free. However, a better
option would be just using av_reallocp_array, which automatically
frees the original pointer on failure.

- Hendrik
Gang Fan(范刚) Feb. 12, 2018, 12:32 p.m. UTC | #2
OK, should I email the new patch to the same thread or a new thread?

Thanks
Gang

On Mon, Feb 12, 2018 at 7:49 PM, Hendrik Leppkes <h.leppkes@gmail.com>
wrote:

> On Mon, Feb 12, 2018 at 11:55 AM, Gang Fan(范刚) <fan.gang.cn@gmail.com>
> wrote:
> > There is a potential memory leak bug in file ass_split.c, here is the
> > description.
> >
> > A piece of memory is allocated on line 283. When executing the loop twice
> > and if the av_realloc_array returns null the function returns without
> > freeing the memory pointed by order.
> >
> > Suggested fix:
> > free(order) before return NULL; on line 284
> >
> > Reference Ticket: https://trac.ffmpeg.org/ticket/7019#comment:1
> >
> > Thanks
> > Gang
> > Sbrella
> >
> >
> > From 6850fc3a6562b4f5fb92e72eed125e057ad975ae Mon Sep 17 00:00:00 2001
> > From: Fan Gang <fangang@sbrella.com>
> > Date: Mon, 12 Feb 2018 18:46:20 +0800
> > Subject: [PATCH] avcodec/ass: Fix a memory leak defect when realloc
> fails.
> >
> > ---
> >  libavcodec/ass_split.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/ass_split.c b/libavcodec/ass_split.c
> > index 872528b..c7eb07d 100644
> > --- a/libavcodec/ass_split.c
> > +++ b/libavcodec/ass_split.c
> > @@ -280,8 +280,10 @@ static const char *ass_split_section(
> ASSSplitContext
> > *ctx, const char *buf)
> >                  while (!is_eol(*buf)) {
> >                      buf = skip_space(buf);
> >                      len = strcspn(buf, ", \r\n");
> > -                    if (!(tmp = av_realloc_array(order, (*number + 1),
> > sizeof(*order))))
> > +                    if (!(tmp = av_realloc_array(order, (*number + 1),
> > sizeof(*order)))){
> > +                        free(order);
> >                          return NULL;
> > +                    }
> >                      order = tmp;
> >                      order[*number] = -1;
> >                      for (i=0; section->fields[i].name; i++)
> > --
>
> You would need to use av_free instead of free. However, a better
> option would be just using av_reallocp_array, which automatically
> frees the original pointer on failure.
>
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
diff mbox

Patch

diff --git a/libavcodec/ass_split.c b/libavcodec/ass_split.c
index 872528b..c7eb07d 100644
--- a/libavcodec/ass_split.c
+++ b/libavcodec/ass_split.c
@@ -280,8 +280,10 @@  static const char *ass_split_section(ASSSplitContext
*ctx, const char *buf)
                 while (!is_eol(*buf)) {
                     buf = skip_space(buf);
                     len = strcspn(buf, ", \r\n");
-                    if (!(tmp = av_realloc_array(order, (*number + 1),
sizeof(*order))))
+                    if (!(tmp = av_realloc_array(order, (*number + 1),
sizeof(*order)))){
+                        free(order);
                         return NULL;
+                    }