| Message ID | 1554369931-21029-1-git-send-email-mypopydev@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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);