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

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

[Jeremy Dorfman]

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];
             }
 
-- 
2.40.0.rc0.216.gc4246ad0f0-goog

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

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

[James Almer]

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]);
_______________________________________________
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".

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

[Jeremy Dorfman]

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];
             }
_______________________________________________
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".

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

[Jeremy Dorfman]

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]);
_______________________________________________
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".

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

[Anton Khirnov]

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.

-- 
Anton Khirnov
_______________________________________________
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".

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

[James Almer]

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

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

[James Almer]

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?
_______________________________________________
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".

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

[Jeremy Dorfman]

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