[FFmpeg-devel] [PATCH 1/5] avcodec/pngdec: Check last AVFrame before deref

[FFmpeg-devel] [PATCH 1/5] avcodec/pngdec: Check last AVFrame before deref

[Michael Niedermayer]

Fixes: NULL pointer dereference
Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016

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

diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
index f7751223b81..b24bfa248dc 100644
--- a/libavcodec/pngdec.c
+++ b/libavcodec/pngdec.c
@@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
         return AVERROR_INVALIDDATA;
     }
 
-    if ((sequence_number == 0 || !s->last_picture.f->data[0]) &&
+    if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) &&
         dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
         // No previous frame to revert to for the first frame
         // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND
-- 
2.43.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".

[FFmpeg-devel] [PATCH 2/5] avcodec/vp3: Call ff_progress_frame_unref() before ff_progress_frame_get_buffer()

[Michael Niedermayer]

Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688
Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904

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

diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
index 2a5f68dfa8e..09527607767 100644
--- a/libavcodec/vp3.c
+++ b/libavcodec/vp3.c
@@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame,
     if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe)
         return buf_size;
 
+    ff_progress_frame_unref(&s->current_frame);
     ret = ff_progress_frame_get_buffer(avctx, &s->current_frame,
                                        AV_GET_BUFFER_FLAG_REF);
     if (ret < 0) {
-- 
2.43.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 2/5] avcodec/vp3: Call ff_progress_frame_unref() before ff_progress_frame_get_buffer()

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688
> Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/vp3.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> index 2a5f68dfa8e..09527607767 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame,
>      if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe)
>          return buf_size;
>  
> +    ff_progress_frame_unref(&s->current_frame);
>      ret = ff_progress_frame_get_buffer(avctx, &s->current_frame,
>                                         AV_GET_BUFFER_FLAG_REF);
>      if (ret < 0) {

LGTM.
(If I am not mistaken, this would have triggered the av_log(avctx,
AV_LOG_ERROR, "pic->data[*]!=NULL in get_buffer_internal\n") codepath
before switching to ProgressFrames, i.e. using an assert instead of
erroring out uncovered a bug. Maybe we should use an assert in
ff_get_buffer(), too?)

- 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".

Re: [FFmpeg-devel] [PATCH 2/5] avcodec/vp3: Call ff_progress_frame_unref() before ff_progress_frame_get_buffer()

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]

Hi

On Sat, Apr 27, 2024 at 11:47:32AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: Assertion !f->f && !f->progress failed at libavcodec/decode.c:1688
> > Fixes: 68190/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_THEORA_fuzzer-5942090287611904
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/vp3.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> > index 2a5f68dfa8e..09527607767 100644
> > --- a/libavcodec/vp3.c
> > +++ b/libavcodec/vp3.c
> > @@ -2651,6 +2651,7 @@ static int vp3_decode_frame(AVCodecContext *avctx, AVFrame *frame,
> >      if (avctx->skip_frame >= AVDISCARD_NONKEY && !s->keyframe)
> >          return buf_size;
> >  
> > +    ff_progress_frame_unref(&s->current_frame);
> >      ret = ff_progress_frame_get_buffer(avctx, &s->current_frame,
> >                                         AV_GET_BUFFER_FLAG_REF);
> >      if (ret < 0) {
> 
> LGTM.

will apply

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [PATCH 3/5] avcodec/decode: Check progress before dereferencing

[Michael Niedermayer]

Fixes: NULL pointer dereference
Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904

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

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index d031b1ca176..a6131941f43 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
 
 void ff_progress_frame_await(const ProgressFrame *f, int n)
 {
+    if (!f->progress)
+        return;
     ff_thread_progress_await(&f->progress->progress, n);
 }
 
-- 
2.43.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 3/5] avcodec/decode: Check progress before dereferencing

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: NULL pointer dereference
> Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/decode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index d031b1ca176..a6131941f43 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
>  
>  void ff_progress_frame_await(const ProgressFrame *f, int n)
>  {
> +    if (!f->progress)
> +        return;
>      ff_thread_progress_await(&f->progress->progress, n);
>  }
>  

Can I get the sample? I see two places in VP8 where the VP8Frame
pointers are set before the actual frame inside it is properly allocated.

(Actually, it was intended for this API to not support waiting on
non-existent frames (i.e. let the caller check for this; in most
instances, it is already guaranteed that the frame one waits one exists,
so this is unnecessary for them).)

- 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".

Re: [FFmpeg-devel] [PATCH 3/5] avcodec/decode: Check progress before dereferencing

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2204 bytes --]

On Sat, Apr 27, 2024 at 01:13:54PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: NULL pointer dereference
> > Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/decode.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index d031b1ca176..a6131941f43 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
> >  
> >  void ff_progress_frame_await(const ProgressFrame *f, int n)
> >  {
> > +    if (!f->progress)
> > +        return;
> >      ff_thread_progress_await(&f->progress->progress, n);
> >  }
> >  
> 
> Can I get the sample? I see two places in VP8 where the VP8Frame
> pointers are set before the actual frame inside it is properly allocated.
> 
> (Actually, it was intended for this API to not support waiting on
> non-existent frames (i.e. let the caller check for this; in most
> instances, it is already guaranteed that the frame one waits one exists,
> so this is unnecessary for them).)

did you receive the sample i sent you in april ?

any update on this ?
its still crashing without this patch

Running: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
libavcodec/decode.c:1766:44: runtime error: member access within null pointer of type 'struct ProgressInternal'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/decode.c:1766:44 in
libavcodec/threadprogress.c:72:36: runtime error: member access within null pointer of type 'ThreadProgress' (aka 'struct ThreadProgress')
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/threadprogress.c:72:36 in

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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 3/5] avcodec/decode: Check progress before dereferencing

[Andreas Rheinhardt]

Michael Niedermayer:
> On Sat, Apr 27, 2024 at 01:13:54PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Fixes: NULL pointer dereference
>>> Fixes: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/decode.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>>> index d031b1ca176..a6131941f43 100644
>>> --- a/libavcodec/decode.c
>>> +++ b/libavcodec/decode.c
>>> @@ -1744,6 +1744,8 @@ void ff_progress_frame_report(ProgressFrame *f, int n)
>>>  
>>>  void ff_progress_frame_await(const ProgressFrame *f, int n)
>>>  {
>>> +    if (!f->progress)
>>> +        return;
>>>      ff_thread_progress_await(&f->progress->progress, n);
>>>  }
>>>  
>>
>> Can I get the sample? I see two places in VP8 where the VP8Frame
>> pointers are set before the actual frame inside it is properly allocated.
>>
>> (Actually, it was intended for this API to not support waiting on
>> non-existent frames (i.e. let the caller check for this; in most
>> instances, it is already guaranteed that the frame one waits one exists,
>> so this is unnecessary for them).)
> 
> did you receive the sample i sent you in april ?
> 
> any update on this ?
> its still crashing without this patch
> 
> Running: 68192/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_VP8_fuzzer-6180311026171904
> libavcodec/decode.c:1766:44: runtime error: member access within null pointer of type 'struct ProgressInternal'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/decode.c:1766:44 in
> libavcodec/threadprogress.c:72:36: runtime error: member access within null pointer of type 'ThreadProgress' (aka 'struct ThreadProgress')
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/threadprogress.c:72:36 in
> 

Totally forgot about this. Will look into it. Thanks for the reminder.

- 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".

[FFmpeg-devel] [PATCH 4/5] avcodec/hevcdec: Check ref frame

[Michael Niedermayer]

Fixes: NULL pointer dereferences
Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560

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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index fcfb275f63a..92b0e45eee0 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0,
 
     if (current_mv.pred_flag & PF_L0) {
         ref0 = refPicList[0].ref[current_mv.ref_idx[0]];
-        if (!ref0 || !ref0->frame->data[0])
+        if (!ref0 || !ref0->frame || !ref0->frame->data[0])
             return;
         hevc_await_progress(s, ref0, &current_mv.mv[0], y0, nPbH);
     }
     if (current_mv.pred_flag & PF_L1) {
         ref1 = refPicList[1].ref[current_mv.ref_idx[1]];
-        if (!ref1 || !ref1->frame->data[0])
+        if (!ref1 || !ref1->frame || !ref1->frame->data[0])
             return;
         hevc_await_progress(s, ref1, &current_mv.mv[1], y0, nPbH);
     }
-- 
2.43.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 4/5] avcodec/hevcdec: Check ref frame

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: NULL pointer dereferences
> Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560
> 
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> index fcfb275f63a..92b0e45eee0 100644
> --- a/libavcodec/hevcdec.c
> +++ b/libavcodec/hevcdec.c
> @@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0,
>  
>      if (current_mv.pred_flag & PF_L0) {
>          ref0 = refPicList[0].ref[current_mv.ref_idx[0]];
> -        if (!ref0 || !ref0->frame->data[0])
> +        if (!ref0 || !ref0->frame || !ref0->frame->data[0])
>              return;
>          hevc_await_progress(s, ref0, &current_mv.mv[0], y0, nPbH);
>      }
>      if (current_mv.pred_flag & PF_L1) {
>          ref1 = refPicList[1].ref[current_mv.ref_idx[1]];
> -        if (!ref1 || !ref1->frame->data[0])
> +        if (!ref1 || !ref1->frame || !ref1->frame->data[0])
>              return;
>          hevc_await_progress(s, ref1, &current_mv.mv[1], y0, nPbH);
>      }

Same as with 1/5: Checking for !ref0->frame is enough as HEVCFrame.f is
set if and only if the HEVCFrame.f->data[0] is set (with the possible
exception of hw-accelerated pixel formats that don't use AVFrame.data at
all (I don't know whether they exist); in any case, HEVCFrame.f is set
if and only if HEVCFrame.f->buf[0] is set).
Actually, I checked all the decoder that I ported to ProgressFrames for
this pattern, but apparently I overlooked way too much (maybe I only
checked for the ->buf[0] pattern?). Sorry for that.

- 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".

Re: [FFmpeg-devel] [PATCH 4/5] avcodec/hevcdec: Check ref frame

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1916 bytes --]

On Sat, Apr 27, 2024 at 12:14:18PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: NULL pointer dereferences
> > Fixes: 68197/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-6382538823106560
> > 
> > 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 | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
> > index fcfb275f63a..92b0e45eee0 100644
> > --- a/libavcodec/hevcdec.c
> > +++ b/libavcodec/hevcdec.c
> > @@ -1969,13 +1969,13 @@ static void hls_prediction_unit(HEVCLocalContext *lc, int x0, int y0,
> >  
> >      if (current_mv.pred_flag & PF_L0) {
> >          ref0 = refPicList[0].ref[current_mv.ref_idx[0]];
> > -        if (!ref0 || !ref0->frame->data[0])
> > +        if (!ref0 || !ref0->frame || !ref0->frame->data[0])
> >              return;
> >          hevc_await_progress(s, ref0, &current_mv.mv[0], y0, nPbH);
> >      }
> >      if (current_mv.pred_flag & PF_L1) {
> >          ref1 = refPicList[1].ref[current_mv.ref_idx[1]];
> > -        if (!ref1 || !ref1->frame->data[0])
> > +        if (!ref1 || !ref1->frame || !ref1->frame->data[0])
> >              return;
> >          hevc_await_progress(s, ref1, &current_mv.mv[1], y0, nPbH);
> >      }
> 
> Same as with 1/5: Checking for !ref0->frame is enough as HEVCFrame.f is
> set if and only if the HEVCFrame.f->data[0] is set (with the possible

will apply with this simplification

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [PATCH 5/5] avformat/mov: Check if heif item name is already allocated

[Michael Niedermayer]

Fixes: memleak
Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152

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

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 97a24e6737e..5b8278f736e 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx)
     avio_rb24(pb);  // flags.
     size -= 4;
 
+    if (c->heif_item[idx].name)
+        return AVERROR_INVALIDDATA;
+
     if (version < 2) {
         avpriv_report_missing_feature(c->fc, "infe version < 2");
         return 1;
-- 
2.43.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 5/5] avformat/mov: Check if heif item name is already allocated

[James Almer]

On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
> Fixes: memleak
> Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>   libavformat/mov.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 97a24e6737e..5b8278f736e 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx)
>       avio_rb24(pb);  // flags.
>       size -= 4;
>   
> +    if (c->heif_item[idx].name)
> +        return AVERROR_INVALIDDATA;

I prefer to free the old one before the av_bprint_finalize() call 
instead of aborting.

> +
>       if (version < 2) {
>           avpriv_report_missing_feature(c->fc, "infe version < 2");
>           return 1;
_______________________________________________
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 5/5] avformat/mov: Check if heif item name is already allocated

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1521 bytes --]

On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
> On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
> > Fixes: memleak
> > Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >   libavformat/mov.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/libavformat/mov.c b/libavformat/mov.c
> > index 97a24e6737e..5b8278f736e 100644
> > --- a/libavformat/mov.c
> > +++ b/libavformat/mov.c
> > @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx)
> >       avio_rb24(pb);  // flags.
> >       size -= 4;
> > +    if (c->heif_item[idx].name)
> > +        return AVERROR_INVALIDDATA;
> 
> I prefer to free the old one before the av_bprint_finalize() call instead of
> aborting.

does the format allow this to be changing ?

because security wise, allowing an attacker to change things is worse
than allowing her to just set it once.

and heif seems to have a rather high density of issues already, judging
from how many of the recent mov issues where in heif code

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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 5/5] avformat/mov: Check if heif item name is already allocated

[James Almer]

On 4/26/2024 9:03 PM, Michael Niedermayer wrote:
> On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
>> On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
>>> Fixes: memleak
>>> Fixes: 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
>>>
>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>    libavformat/mov.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 97a24e6737e..5b8278f736e 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, AVIOContext *pb, MOVAtom atom, int idx)
>>>        avio_rb24(pb);  // flags.
>>>        size -= 4;
>>> +    if (c->heif_item[idx].name)
>>> +        return AVERROR_INVALIDDATA;
>>
>> I prefer to free the old one before the av_bprint_finalize() call instead of
>> aborting.
> 
> does the format allow this to be changing ?

This code is not supposed to be invoked twice if an iinf box is 
correctly parsed. It can only happen if a second iinf box shows up after 
the first and demuxing wasn't aborted, either because the error was 
ignored, or an infe box within was version < 2.

> 
> because security wise, allowing an attacker to change things is worse
> than allowing her to just set it once.

A second iinf box could show up after the first was not finished 
parsing, and as long as it has no names for the items, it would still 
finish parsing even after this patch. So better just clean old values 
and keep parsing. Further sanity checks will happen when all the items 
are turned into something meant to be exported.

> 
> and heif seems to have a rather high density of issues already, judging
> from how many of the recent mov issues where in heif code
> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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 5/5] avformat/mov: Check if heif item name is already allocated

[James Almer]

On 4/26/2024 9:23 PM, James Almer wrote:
> On 4/26/2024 9:03 PM, Michael Niedermayer wrote:
>> On Fri, Apr 26, 2024 at 08:57:02PM -0300, James Almer wrote:
>>> On 4/26/2024 8:52 PM, Michael Niedermayer wrote:
>>>> Fixes: memleak
>>>> Fixes: 
>>>> 68212/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-4963488540721152
>>>>
>>>> Found-by: continuous fuzzing process 
>>>> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>> ---
>>>>    libavformat/mov.c | 3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>> index 97a24e6737e..5b8278f736e 100644
>>>> --- a/libavformat/mov.c
>>>> +++ b/libavformat/mov.c
>>>> @@ -8136,6 +8136,9 @@ static int mov_read_infe(MOVContext *c, 
>>>> AVIOContext *pb, MOVAtom atom, int idx)
>>>>        avio_rb24(pb);  // flags.
>>>>        size -= 4;
>>>> +    if (c->heif_item[idx].name)
>>>> +        return AVERROR_INVALIDDATA;
>>>
>>> I prefer to free the old one before the av_bprint_finalize() call 
>>> instead of
>>> aborting.
>>
>> does the format allow this to be changing ?
> 
> This code is not supposed to be invoked twice if an iinf box is 
> correctly parsed. It can only happen if a second iinf box shows up after 
> the first and demuxing wasn't aborted, either because the error was 
> ignored, or an infe box within was version < 2.
> 
>>
>> because security wise, allowing an attacker to change things is worse
>> than allowing her to just set it once.
> 
> A second iinf box could show up after the first was not finished 
> parsing, and as long as it has no names for the items, it would still 
> finish parsing even after this patch. So better just clean old values 
> and keep parsing. Further sanity checks will happen when all the items 
> are turned into something meant to be exported.
> 
>>
>> and heif seems to have a rather high density of issues already, judging
>> from how many of the recent mov issues where in heif code
>>
>> thx

Can you test 
https://ffmpeg.org//pipermail/ffmpeg-devel/2024-April/326317.html ?
_______________________________________________
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 1/5] avcodec/pngdec: Check last AVFrame before deref

[Andreas Rheinhardt]

Michael Niedermayer:
> Fixes: NULL pointer dereference
> Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/pngdec.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> index f7751223b81..b24bfa248dc 100644
> --- a/libavcodec/pngdec.c
> +++ b/libavcodec/pngdec.c
> @@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
>          return AVERROR_INVALIDDATA;
>      }
>  
> -    if ((sequence_number == 0 || !s->last_picture.f->data[0]) &&
> +    if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) &&
>          dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
>          // No previous frame to revert to for the first frame
>          // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND

Just checking for !s->last_picture.f is enough -- s->last_picture.f is
set if and only if s->last_picture.f->data[0] is set.

- 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".

Re: [FFmpeg-devel] [PATCH 1/5] avcodec/pngdec: Check last AVFrame before deref

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1563 bytes --]

On Sat, Apr 27, 2024 at 11:36:30AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Fixes: NULL pointer dereference
> > Fixes: 68184/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APNG_fuzzer-4926478069334016
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/pngdec.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c
> > index f7751223b81..b24bfa248dc 100644
> > --- a/libavcodec/pngdec.c
> > +++ b/libavcodec/pngdec.c
> > @@ -1218,7 +1218,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s,
> >          return AVERROR_INVALIDDATA;
> >      }
> >  
> > -    if ((sequence_number == 0 || !s->last_picture.f->data[0]) &&
> > +    if ((sequence_number == 0 || !s->last_picture.f || !s->last_picture.f->data[0]) &&
> >          dispose_op == APNG_DISPOSE_OP_PREVIOUS) {
> >          // No previous frame to revert to for the first frame
> >          // Spec says to just treat it as a APNG_DISPOSE_OP_BACKGROUND
> 
> Just checking for !s->last_picture.f is enough -- s->last_picture.f is
> set if and only if s->last_picture.f->data[0] is set.

ok will apply with that simplified

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is a danger to trust the dream we wish for rather than
the science we have, -- Dr. Kenneth Brown

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".