Message ID | 20230301185008.2167529-1-jdorfman@google.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] libavcodec/h264dec: avoid arithmetic on null pointers | expand |
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 |
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]);
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]; }
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]);
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.
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.
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?
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 --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]; }