[FFmpeg-devel] [PATCH] libavcodec/h264dec: avoid arithmetic on null pointers

[Jeremy Dorfman]

On Thu, Mar 2, 2023 at 6:37 AM James Almer <jamrial at gmail.com> wrote:
>
> On 3/2/2023 8:33 AM, James Almer wrote:
> > On 3/2/2023 6:05 AM, Anton Khirnov wrote:
> >> Quoting Jeremy Dorfman (2023-03-01 19:50:08)
> >>> null pointer arithmetic is undefined behavior in C.
> >>> ---
> >>>   libavcodec/h264dec.c | 4 ++--
> >>>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> >>> index 2d691731c5..ef698f2630 100644
> >>> --- a/libavcodec/h264dec.c
> >>> +++ b/libavcodec/h264dec.c
> >>> @@ -912,8 +912,8 @@ static int finalize_frame(H264Context *h, AVFrame
> >>> *dst, H264Picture *out, int *g
> >>>               av_log(h->avctx, AV_LOG_DEBUG, "Duplicating field %d to
> >>> fill missing\n", field);
> >>>               for (p = 0; p<4; p++) {
> >>> -                dst_data[p] = f->data[p] + (field^1)*f->linesize[p];
> >>> -                src_data[p] = f->data[p] +  field   *f->linesize[p];
> >>> +                dst_data[p] = f->data[p] ? f->data[p] +
> >>> (field^1)*f->linesize[p] : NULL;
> >>> +                src_data[p] = f->data[p] ? f->data[p] +  field
> >>> *f->linesize[p] : NULL;
> >>
> >> Why would that be NULL? Seems like something that should not happen.
> >
> > None of the supported pixel formats in this decoder use four planes, so
> > at least the last one will always be NULL. FF_PTR_ADD() is what we did
> > in similar situations, like in sws_receive_slice(), when we don't use
> > some helper to get the exact number of used planes from the pixfmt
> > descriptor.
>
> http://coverage.ffmpeg.org/index.h264dec.c.8820c603e94612cd02689417231bc605.html#l912
>
> The ubsan fate instance would have detected this long ago if we had a
> sample that covers this path.
> Do you happen to have one you can make public to be added to the FATE
> suite, Jeremy? Or was this problem found using some static analyzer?

This was found with a particular conformance stream and ffmpeg built
with -fsanitize=undefined. I'm afraid I can't share the conformance
stream. I've spent the last couple of hours trying to create a stream
that triggers the branch in finalize_frame and have not succeeded in
doing so. I suspect doing so may prove fragile as well; the
conformance stream decodes without issue with JM 19.0.