[FFmpeg-devel,V1] lavf/matroskaenc: Fix memory leak after write trailer

Submitted by Jun Zhao on April 4, 2019, 9:25 a.m.

Details

Message ID 1554369931-21029-1-git-send-email-mypopydev@gmail.com
State New
Headers show

Commit Message

Jun Zhao April 4, 2019, 9:25 a.m.
From: Jun Zhao <barryjzhao@tencent.com>

Fix memory leak after write trailer for #7827

Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavformat/matroskaenc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Hendrik Leppkes April 4, 2019, 9:42 a.m.
On Thu, Apr 4, 2019 at 11:25 AM Jun Zhao <mypopydev@gmail.com> wrote:
>
> From: Jun Zhao <barryjzhao@tencent.com>
>
> Fix memory leak after write trailer for #7827
>
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  libavformat/matroskaenc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index b9f99c4..22ba93a 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -2571,13 +2571,13 @@ static int mkv_write_trailer(AVFormatContext *s)
>      // check if we have an audio packet cached
>      if (mkv->cur_audio_pkt.size > 0) {
>          ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0);
> -        av_packet_unref(&mkv->cur_audio_pkt);
>          if (ret < 0) {
>              av_log(s, AV_LOG_ERROR,
>                     "Could not write cached audio packet ret:%d\n", ret);
>              return ret;
>          }
>      }
> +    av_packet_unref(&mkv->cur_audio_pkt);
>

Won't this leak instead when the error path above is triggered?
Also, whats in the packet if it has a size of 0?

- Hendrik
mypopy@gmail.com April 4, 2019, 9:52 a.m.
On Thu, Apr 4, 2019 at 5:43 PM Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>
> On Thu, Apr 4, 2019 at 11:25 AM Jun Zhao <mypopydev@gmail.com> wrote:
> >
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > Fix memory leak after write trailer for #7827
> >
> > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > ---
> >  libavformat/matroskaenc.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index b9f99c4..22ba93a 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -2571,13 +2571,13 @@ static int mkv_write_trailer(AVFormatContext *s)
> >      // check if we have an audio packet cached
> >      if (mkv->cur_audio_pkt.size > 0) {
> >          ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0);
> > -        av_packet_unref(&mkv->cur_audio_pkt);
> >          if (ret < 0) {
> >              av_log(s, AV_LOG_ERROR,
> >                     "Could not write cached audio packet ret:%d\n", ret);
> >              return ret;
> >          }
> >      }
> > +    av_packet_unref(&mkv->cur_audio_pkt);
> >
>
> Won't this leak instead when the error path above is triggered?
> Also, whats in the packet if it has a size of 0?
>
I think is mkv_write_packet() with the codec_type ==
AVMEDIA_TYPE_AUDIO in
https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/matroskaenc.c#L2537
Oliver Collyer via ffmpeg-devel April 4, 2019, 11:28 a.m.
Hendrik Leppkes:
> On Thu, Apr 4, 2019 at 11:25 AM Jun Zhao <mypopydev@gmail.com> wrote:
>>
>> From: Jun Zhao <barryjzhao@tencent.com>
>>
>> Fix memory leak after write trailer for #7827
>>
>> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
>> ---
>>  libavformat/matroskaenc.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index b9f99c4..22ba93a 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -2571,13 +2571,13 @@ static int mkv_write_trailer(AVFormatContext *s)
>>      // check if we have an audio packet cached
>>      if (mkv->cur_audio_pkt.size > 0) {
>>          ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0);
>> -        av_packet_unref(&mkv->cur_audio_pkt);
>>          if (ret < 0) {
>>              av_log(s, AV_LOG_ERROR,
>>                     "Could not write cached audio packet ret:%d\n", ret);
>>              return ret;
>>          }
>>      }
>> +    av_packet_unref(&mkv->cur_audio_pkt);
>>
> 
> Won't this leak instead when the error path above is triggered?

Yes, it will.

> Also, whats in the packet if it has a size of 0?
> 
It contains side-data which is used to update the extradata (the md5
and number of samples etc. that is part of the flac STREAMINFO header).

- Andreas
Oliver Collyer via ffmpeg-devel April 4, 2019, 1:04 p.m.
And I think that this memleak in mkv_write_trailer() has a twin in
mkv_write_packet(): Audio frames are not written directly, but rather
put into storage in cur_audio_pkt via av_packet_ref(). But if the
preceding audio packet was a zero size packet (but possibly with
side-data), then the preceding packet's side data will leak at this
point. A better solution seems to be never to store a packet whose
buffer has size zero in cur_audio_pkt. The only use we have for such
packets is to use them for mkv_check_new_extra_data() and that has
already been done at this point.

- Andreas
Hendrik Leppkes April 4, 2019, 1:13 p.m.
On Thu, Apr 4, 2019 at 3:05 PM Andreas Rheinhardt via ffmpeg-devel
<ffmpeg-devel@ffmpeg.org> wrote:
>
> And I think that this memleak in mkv_write_trailer() has a twin in
> mkv_write_packet(): Audio frames are not written directly, but rather
> put into storage in cur_audio_pkt via av_packet_ref(). But if the
> preceding audio packet was a zero size packet (but possibly with
> side-data), then the preceding packet's side data will leak at this
> point. A better solution seems to be never to store a packet whose
> buffer has size zero in cur_audio_pkt. The only use we have for such
> packets is to use them for mkv_check_new_extra_data() and that has
> already been done at this point.
>

That seems like a good idea.

- Hendrik

Patch hide | download patch | download mbox

diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index b9f99c4..22ba93a 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -2571,13 +2571,13 @@  static int mkv_write_trailer(AVFormatContext *s)
     // check if we have an audio packet cached
     if (mkv->cur_audio_pkt.size > 0) {
         ret = mkv_write_packet_internal(s, &mkv->cur_audio_pkt, 0);
-        av_packet_unref(&mkv->cur_audio_pkt);
         if (ret < 0) {
             av_log(s, AV_LOG_ERROR,
                    "Could not write cached audio packet ret:%d\n", ret);
             return ret;
         }
     }
+    av_packet_unref(&mkv->cur_audio_pkt);
 
     if (mkv->dyn_bc) {
         end_ebml_master_crc32(pb, &mkv->dyn_bc, mkv, mkv->cluster);