diff mbox series

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

Message ID 20230301185008.2167529-1-jdorfman@google.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/h264dec: avoid arithmetic on null pointers | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jeremy Dorfman March 1, 2023, 6:50 p.m. UTC 
null pointer arithmetic is undefined behavior in C.
---
 libavcodec/h264dec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

James Almer March 1, 2023, 7:07 p.m. UTC | #1 
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]);
Jeremy Dorfman March 1, 2023, 8:22 p.m. UTC | #2 
On Wed, Mar 1, 2023 at 2:07 PM James Almer <jamrial@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];
             }
Jeremy Dorfman March 1, 2023, 8:31 p.m. UTC | #3 
On Wed, Mar 1, 2023 at 3:22 PM Jeremy Dorfman <jdorfman@google.com> wrote:
>
> On Wed, Mar 1, 2023 at 2:07 PM James Almer <jamrial@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]);
Anton Khirnov March 2, 2023, 9:05 a.m. UTC | #4 
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.
James Almer March 2, 2023, 11:33 a.m. UTC | #5 
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.
James Almer March 2, 2023, 11:37 a.m. UTC | #6 
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?
Jeremy Dorfman March 2, 2023, 4:09 p.m. UTC | #7 
On Thu, Mar 2, 2023 at 6:37 AM James Almer <jamrial@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.
diff mbox series

Patch

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];
             }