| Message ID | 20231015001323.1073-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/mov: The iloc test is not redundant | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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.
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 [...]
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.
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 [...]
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.
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.
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(-)