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

[Michael Niedermayer <michael@niedermayer.cc>]

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

On Fri, Jun 02, 2023 at 02:28:24AM +0200, Andreas Rheinhardt wrote:
> 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.)

posting a suboptimal fix, if one isnt sure what is the clean way to fix it
is a great way to have someone else make a better fix.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 

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