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

[Jeremy Dorfman]

On Wed, Mar 1, 2023 at 3:22 PM Jeremy Dorfman <jdorfman at google.com> wrote:
>
> On Wed, Mar 1, 2023 at 2:07 PM James Almer <jamrial at gmail.com> wrote:
> >
> > On 3/1/2023 3:50 PM, Jeremy Dorfman wrote:
> > > 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;
> > >                   linesizes[p] = 2*f->linesize[p];
> > >               }
> >
> > Probably cleaner and clearer to do it like this:
> >
> > dst_data[p] = FF_PTR_ADD(f->data[p], (field^1)*f->linesize[p]);
> > src_data[p] = FF_PTR_ADD(f->data[p],  field   *f->linesize[p]);
>
> Thank you for the feedback. That seems reasonable to me; I wasn't aware of FF_PTR_ADD.
>
> ---
>  libavcodec/h264dec.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 2d691731c5..0ac04baa4d 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -31,6 +31,7 @@
>
>  #include "libavutil/avassert.h"
>  #include "libavutil/imgutils.h"
> +#include "libavutil/internal.h"
>  #include "libavutil/opt.h"
>  #include "libavutil/thread.h"
>  #include "libavutil/video_enc_params.h"
> @@ -912,8 +913,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] = FF_PTR_ADD(f->data[p], (field^1)*f->linesize[p]);
> +                src_data[p] = FF_PTR_ADD(f->data[p],  field   *f->linesize[p]);
>                  linesizes[p] = 2*f->linesize[p];
>              }
>

I apologize for the mangled patch and spam. Hopefully this comes
through as text/plain without the corrupted patch:

---
 libavcodec/h264dec.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
index 2d691731c5..0ac04baa4d 100644
--- a/libavcodec/h264dec.c
+++ b/libavcodec/h264dec.c
@@ -31,6 +31,7 @@

 #include "libavutil/avassert.h"
 #include "libavutil/imgutils.h"
+#include "libavutil/internal.h"
 #include "libavutil/opt.h"
 #include "libavutil/thread.h"
 #include "libavutil/video_enc_params.h"
@@ -912,8 +913,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] = FF_PTR_ADD(f->data[p], (field^1)*f->linesize[p]);
+                src_data[p] = FF_PTR_ADD(f->data[p],  field   *f->linesize[p]);