Re: [FFmpeg-devel] [PATCH 2/3] avcodec/cbs_av1: Clear obu.metadata on error

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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 <michael@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-request@ffmpeg.org with subject "unsubscribe".