Michael Niedermayer: > On error pointers can be left NULL while code later assumes these not to be > NULL > > Fixes: NULL pointer dereference > Fixes: > 59359/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_AV1_fuzzer-6726080594313216 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <mich...@niedermayer.cc> > --- > libavcodec/cbs_av1.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c > index 8788fee099..7f3f4da2f5 100644 > --- a/libavcodec/cbs_av1.c > +++ b/libavcodec/cbs_av1.c > @@ -1013,8 +1013,10 @@ static int cbs_av1_read_unit(CodedBitstreamContext > *ctx, > case AV1_OBU_METADATA: > { > err = cbs_av1_read_metadata_obu(ctx, &gbc, &obu->obu.metadata); > - if (err < 0) > + if (err < 0) { > + memset(&obu->obu.metadata, 0, sizeof(obu->obu.metadata)); > return err; > + } > } > break; > case AV1_OBU_PADDING:
1. Before 97f4263, the current_obu was reset (and the packet effectively discarded) upon errors from ff_cbs_read_packet(); yet this is no longer true and it seems that the contents of current_obu will be processed in the next call to av1_receive_frame(). This change seems to have been unintentional. 2. The commit message is weird: You claim that it is bad that a pointer is NULL; and then you go on and zero it again. It would be better to claim that the metadata is in an inconsistent state. 3. There is a possibility for inconsistency in cbs_av1_syntax_template, namely if the allocation fails, metadata OBU nevertheless claims to have a payload_size > 0; payload_size should only be set after the allocation succeeded. But this does not seem to be the issue in this testcase. Presumably there are not enough bits left for the itu_t_t35_country_code or its extension? In this case, cbs_av1 did not even make an error. 4. Your fix is dangerous: In case the code were changed so that an error can happen after a successful allocation, your memset would lead to leaks. (The most likely possibility is the addition of a new type of metadata; another way would be for the code to be changed to avoid reading the metadata twice by reallocating (and overallocating) the buffer as needed.) - 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-requ...@ffmpeg.org with subject "unsubscribe".
