diff mbox

[FFmpeg-devel,1/4] avcodec/hevcdec: Fix memleak of a53_caption

Message ID 20190630221651.12795-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 30, 2019, 10:16 p.m. UTC
Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hevcdec.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

James Almer July 1, 2019, 1:43 a.m. UTC | #1
On 6/30/2019 7:16 PM, Michael Niedermayer wrote:
> Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevcdec.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index 515b346535..b5d918d07d 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -3331,6 +3331,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
>  
>      ff_h2645_packet_uninit(&s->pkt);
>  
> +    ff_hevc_reset_sei(&s->sei);
> +
>      return 0;
>  }

LGTM.
James Almer July 1, 2019, 2:18 a.m. UTC | #2
On 6/30/2019 10:43 PM, James Almer wrote:
> On 6/30/2019 7:16 PM, Michael Niedermayer wrote:
>> Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944
>>
>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>> ---
>>  libavcodec/hevcdec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
>> index 515b346535..b5d918d07d 100644
>> --- a/libavcodec/hevcdec.c
>> +++ b/libavcodec/hevcdec.c
>> @@ -3331,6 +3331,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
>>  
>>      ff_h2645_packet_uninit(&s->pkt);
>>  
>> +    ff_hevc_reset_sei(&s->sei);
>> +
>>      return 0;
>>  }
> 
> LGTM.

You could also add it to hevc_decode_flush() while at it.
Michael Niedermayer July 1, 2019, 2:24 p.m. UTC | #3
On Sun, Jun 30, 2019 at 10:43:33PM -0300, James Almer wrote:
> On 6/30/2019 7:16 PM, Michael Niedermayer wrote:
> > Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevcdec.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index 515b346535..b5d918d07d 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -3331,6 +3331,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
> >  
> >      ff_h2645_packet_uninit(&s->pkt);
> >  
> > +    ff_hevc_reset_sei(&s->sei);
> > +
> >      return 0;
> >  }
> 
> LGTM.

will apply

thx
[...]
Michael Niedermayer July 1, 2019, 2:24 p.m. UTC | #4
On Sun, Jun 30, 2019 at 11:18:55PM -0300, James Almer wrote:
> On 6/30/2019 10:43 PM, James Almer wrote:
> > On 6/30/2019 7:16 PM, Michael Niedermayer wrote:
> >> Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944
> >>
> >> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >> ---
> >>  libavcodec/hevcdec.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> >> index 515b346535..b5d918d07d 100644
> >> --- a/libavcodec/hevcdec.c
> >> +++ b/libavcodec/hevcdec.c
> >> @@ -3331,6 +3331,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
> >>  
> >>      ff_h2645_packet_uninit(&s->pkt);
> >>  
> >> +    ff_hevc_reset_sei(&s->sei);
> >> +
> >>      return 0;
> >>  }
> > 
> > LGTM.
> 
> You could also add it to hevc_decode_flush() while at it.

will post a patch once i (lightly) tested it

thx

[...]
Michael Niedermayer Sept. 9, 2019, 8:59 p.m. UTC | #5
On Mon, Jul 01, 2019 at 07:11:50PM -0300, James Almer wrote:
> On 7/1/2019 11:24 AM, Michael Niedermayer wrote:
> > On Sun, Jun 30, 2019 at 11:18:55PM -0300, James Almer wrote:
> >> On 6/30/2019 10:43 PM, James Almer wrote:
> >>> On 6/30/2019 7:16 PM, Michael Niedermayer wrote:
> >>>> Fixes: 15295/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5675655187922944
> >>>>
> >>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> >>>> ---
> >>>>  libavcodec/hevcdec.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> >>>> index 515b346535..b5d918d07d 100644
> >>>> --- a/libavcodec/hevcdec.c
> >>>> +++ b/libavcodec/hevcdec.c
> >>>> @@ -3331,6 +3331,8 @@ static av_cold int hevc_decode_free(AVCodecContext *avctx)
> >>>>  
> >>>>      ff_h2645_packet_uninit(&s->pkt);
> >>>>  
> >>>> +    ff_hevc_reset_sei(&s->sei);
> >>>> +
> >>>>      return 0;
> >>>>  }
> >>>
> >>> LGTM.
> >>
> >> You could also add it to hevc_decode_flush() while at it.
> > 
> > will post a patch once i (lightly) tested it
> 
> Do you have hevc samples with closed captions? I'm not sure if there's a

no, i did not find one on my disk


> case where one such caption would not be attached to the frame (And thus
> removed from the decoder context) before a call to
> avcodec_flush_buffers() takes place. Maybe some sample where slices are
> in different packets.
> 

> For that matter, h264 seems to handle closed captions a lot better than
> hevc. It uses AVBufferRefs to keep them in sync between frame thread
> contexts, and it also makes sure to reset frame-specific SEI state on
> all frames and on flushing, all things hevc currently doesn't.
> Implementing the former should be trivial, but the latter isn't as slice
> handling is different.

for a bugfix that we should backport as it may have some leak->OOM potential
redesigning the handling is problematic. As backporting that would likely
not happen ...

So i suggest to go with a simpler solution like the original patch or
one with the call also in hevc_decode_flush()

Are you ok with this ?
if so do you prefer the call also be done in flush ?
also this issue is approaching the deadline ...

Thanks

[...]
diff mbox

Patch

diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 515b346535..b5d918d07d 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -3331,6 +3331,8 @@  static av_cold int hevc_decode_free(AVCodecContext *avctx)
 
     ff_h2645_packet_uninit(&s->pkt);
 
+    ff_hevc_reset_sei(&s->sei);
+
     return 0;
 }