[FFmpeg-devel] [PATCH] avformat/mov: The iloc test is not redundant

[FFmpeg-devel] [PATCH] avformat/mov: The iloc test is not redundant

[Michael Niedermayer]

Fixes: Assertion failure
Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784

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

diff --git a/libavformat/mov.c b/libavformat/mov.c
index 2f29487beb..34691d0cda 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -7788,11 +7788,10 @@ static int mov_read_iloc(MOVContext *c, AVIOContext *pb, MOVAtom atom)
         return 0;
     }
 
-    if (c->avif_info) {
-        av_log(c->fc, AV_LOG_INFO, "Duplicate iloc box found\n");
+    if (c->avif_info || c->fc->nb_streams) {
+        av_log(c->fc, AV_LOG_INFO, "Duplicate or invalid iloc box found\n");
         return 0;
     }
-    av_assert0(!c->fc->nb_streams);
 
     version = avio_r8(pb);
     avio_rb24(pb);  // flags.
-- 
2.17.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] avformat/mov: The iloc test is not redundant

[Anton Khirnov]

Quoting Michael Niedermayer (2023-10-15 02:13:23)
> Fixes: Assertion failure
> Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---

The commit message is useless.

-- 
Anton Khirnov
_______________________________________________
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] avformat/mov: The iloc test is not redundant

[Michael Niedermayer]

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

On Thu, Oct 19, 2023 at 01:10:18PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-10-15 02:13:23)
> > Fixes: Assertion failure
> > Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> 
> The commit message is useless.

This comment is also not that usefull
What would you like to see in the commit message ?

The 2 checks are not redundant. Should the message detail how
the assertion failure occured ?

Would you prefer if the 2nd condition produces an error instead of return 0 ?

Is there something else ?

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The smallest minority on earth is the individual. Those who deny 
individual rights cannot claim to be defenders of minorities. - Ayn Rand

[-- 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] avformat/mov: The iloc test is not redundant

[Anton Khirnov]

Quoting Michael Niedermayer (2023-10-19 18:33:13)
> On Thu, Oct 19, 2023 at 01:10:18PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2023-10-15 02:13:23)
> > > Fixes: Assertion failure
> > > Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784
> > > 
> > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > 
> > The commit message is useless.
> 
> This comment is also not that usefull
> What would you like to see in the commit message ?
> 
> The 2 checks are not redundant. Should the message detail how
> the assertion failure occured ?

At least two people previously thought that the condition is redundant,
so it seems clear to me that an explanation is in order.

I actually find it quite baffling that this is not obvious to you. Do
you really think that "Fixes: Assertion failure" is sufficient
explanation for anyone reading this patch?

> Would you prefer if the 2nd condition produces an error instead of return 0 ?

Maybe. Depending on the conditions under which this happens.

-- 
Anton Khirnov
_______________________________________________
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] avformat/mov: The iloc test is not redundant

[Michael Niedermayer]

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

On Thu, Oct 19, 2023 at 07:42:30PM +0200, Anton Khirnov wrote:
> Quoting Michael Niedermayer (2023-10-19 18:33:13)
> > On Thu, Oct 19, 2023 at 01:10:18PM +0200, Anton Khirnov wrote:
> > > Quoting Michael Niedermayer (2023-10-15 02:13:23)
> > > > Fixes: Assertion failure
> > > > Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784
> > > > 
> > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > 
> > > The commit message is useless.
> > 
> > This comment is also not that usefull
> > What would you like to see in the commit message ?
> > 
> > The 2 checks are not redundant. Should the message detail how
> > the assertion failure occured ?
> 
> At least two people previously thought that the condition is redundant,
> so it seems clear to me that an explanation is in order.
> 
> I actually find it quite baffling that this is not obvious to you. Do
> you really think that "Fixes: Assertion failure" is sufficient
> explanation for anyone reading this patch?

let me ask this from the other direction (and i should probably have done
so sooner)

why would this be redundant ?

the failed check checks the number of streams, why should a random atom
not occur after x streams for thf irst time ?
what code was supposed to prevent this ?

thx

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.

[-- 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] avformat/mov: The iloc test is not redundant

[Anton Khirnov]

Quoting Michael Niedermayer (2023-10-19 20:53:05)
> On Thu, Oct 19, 2023 at 07:42:30PM +0200, Anton Khirnov wrote:
> > Quoting Michael Niedermayer (2023-10-19 18:33:13)
> > > On Thu, Oct 19, 2023 at 01:10:18PM +0200, Anton Khirnov wrote:
> > > > Quoting Michael Niedermayer (2023-10-15 02:13:23)
> > > > > Fixes: Assertion failure
> > > > > Fixes: 62866/clusterfuzz-testcase-minimized-ffmpeg_dem_MOV_fuzzer-5282997370486784
> > > > > 
> > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > > ---
> > > > 
> > > > The commit message is useless.
> > > 
> > > This comment is also not that usefull
> > > What would you like to see in the commit message ?
> > > 
> > > The 2 checks are not redundant. Should the message detail how
> > > the assertion failure occured ?
> > 
> > At least two people previously thought that the condition is redundant,
> > so it seems clear to me that an explanation is in order.
> > 
> > I actually find it quite baffling that this is not obvious to you. Do
> > you really think that "Fixes: Assertion failure" is sufficient
> > explanation for anyone reading this patch?
> 
> let me ask this from the other direction (and i should probably have done
> so sooner)
> 
> why would this be redundant ?
> 
> the failed check checks the number of streams, why should a random atom
> not occur after x streams for thf irst time ?
> what code was supposed to prevent this ?

The intent seems to be that for is_still_picture_avif=1 there should
only be one stream, created in avif_add_stream(), called after
mov_read_iloc(). Since avif_add_stream() will fail if any streams
already exist, or when mov_read_iloc() has not been called (since
avif_info_size will be 0), I'd say the correct thing to do is
fail at the very top of mov_read_trak() when is_still_picture_avif=1, so
no streams can be created for such files outside of avif_add_stream().
This should fix the assertion failure.

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