[FFmpeg-devel] [PATCH] avcodec/hevcdec: fix out of bounds index -1 for inter prediction

[FFmpeg-devel] [PATCH] avcodec/hevcdec: fix out of bounds index -1 for inter prediction

[Nuo Mi]

It's a false positive. We use -1 to index an array, but it's not actually used.
This patch will make the "GCC UndefinedBehaviorSanitizer" happy.

Found by: checkasm-hevc_pel in https://fate.ffmpeg.org/report.cgi?time=20240211011905&slot=x86_64-archlinux-gcc-ubsan
Reported-by: James Almer <jamrial@gmail.com>
---
 libavcodec/hevcdsp_template.c | 6 +++---
 libavcodec/x86/hevcdsp_init.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/libavcodec/hevcdsp_template.c b/libavcodec/hevcdsp_template.c
index 9b48bdf08e..a4b8901e5d 100644
--- a/libavcodec/hevcdsp_template.c
+++ b/libavcodec/hevcdsp_template.c
@@ -301,9 +301,9 @@ IDCT_DC(32)
 //
 ////////////////////////////////////////////////////////////////////////////////
 #define ff_hevc_pel_filters ff_hevc_qpel_filters
-#define DECL_HV_FILTER(f)                                  \
-    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
-    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
+#define DECL_HV_FILTER(f)                                               \
+    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;  \
+    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;
 
 #define FW_PUT(p, f, t)                                                                                   \
 static void FUNC(put_hevc_## f)(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride, int height,        \
diff --git a/libavcodec/x86/hevcdsp_init.c b/libavcodec/x86/hevcdsp_init.c
index 31e81eb11f..e0f65177c4 100644
--- a/libavcodec/x86/hevcdsp_init.c
+++ b/libavcodec/x86/hevcdsp_init.c
@@ -87,9 +87,9 @@ IDCT_FUNCS(avx)
 
 
 #define ff_hevc_pel_filters ff_hevc_qpel_filters
-#define DECL_HV_FILTER(f)                                  \
-    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
-    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
+#define DECL_HV_FILTER(f)                                               \
+    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;  \
+    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;  \
 
 #define FW_PUT(p, a, b, depth, opt) \
 void ff_hevc_put_hevc_ ## a ## _ ## depth ## _##opt(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride,   \
-- 
2.25.1

_______________________________________________
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] avcodec/hevcdec: fix out of bounds index -1 for inter prediction

[Andreas Rheinhardt]

Nuo Mi:
> It's a false positive. We use -1 to index an array, but it's not actually used.
> This patch will make the "GCC UndefinedBehaviorSanitizer" happy.
> 
> Found by: checkasm-hevc_pel in https://fate.ffmpeg.org/report.cgi?time=20240211011905&slot=x86_64-archlinux-gcc-ubsan
> Reported-by: James Almer <jamrial@gmail.com>
> ---
>  libavcodec/hevcdsp_template.c | 6 +++---
>  libavcodec/x86/hevcdsp_init.c | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/hevcdsp_template.c b/libavcodec/hevcdsp_template.c
> index 9b48bdf08e..a4b8901e5d 100644
> --- a/libavcodec/hevcdsp_template.c
> +++ b/libavcodec/hevcdsp_template.c
> @@ -301,9 +301,9 @@ IDCT_DC(32)
>  //
>  ////////////////////////////////////////////////////////////////////////////////
>  #define ff_hevc_pel_filters ff_hevc_qpel_filters
> -#define DECL_HV_FILTER(f)                                  \
> -    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
> -    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
> +#define DECL_HV_FILTER(f)                                               \
> +    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;  \
> +    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;
>  
>  #define FW_PUT(p, f, t)                                                                                   \
>  static void FUNC(put_hevc_## f)(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride, int height,        \
> diff --git a/libavcodec/x86/hevcdsp_init.c b/libavcodec/x86/hevcdsp_init.c
> index 31e81eb11f..e0f65177c4 100644
> --- a/libavcodec/x86/hevcdsp_init.c
> +++ b/libavcodec/x86/hevcdsp_init.c
> @@ -87,9 +87,9 @@ IDCT_FUNCS(avx)
>  
>  
>  #define ff_hevc_pel_filters ff_hevc_qpel_filters
> -#define DECL_HV_FILTER(f)                                  \
> -    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
> -    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
> +#define DECL_HV_FILTER(f)                                               \
> +    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;  \
> +    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;  \

Spurious addition of trailing '\'.

>  
>  #define FW_PUT(p, a, b, depth, opt) \
>  void ff_hevc_put_hevc_ ## a ## _ ## depth ## _##opt(int16_t *dst, const uint8_t *src, ptrdiff_t srcstride,   \

I have just sent an alternative solution for this that avoids all these
branches (which the compiler will not be able to eliminate).
Unfortunately I can't test mips and loongarch myself; there is a
fate-runner for the latter, but nothing for the former. So hopefully
someone can test mips.

- Andreas

PS: The aarch64 code (hevcdsp_[eq]pel_neon.S) already offsets its
filters via dummy arrays. I wonder whether it could now reuse the
ordinary ones.

_______________________________________________
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] avcodec/hevcdec: fix out of bounds index -1 for inter prediction

[Nuo Mi]

On Sun, Feb 11, 2024 at 5:00 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Nuo Mi:
> > It's a false positive. We use -1 to index an array, but it's not
> actually used.
> > This patch will make the "GCC UndefinedBehaviorSanitizer" happy.
> >
> > Found by: checkasm-hevc_pel in
> https://fate.ffmpeg.org/report.cgi?time=20240211011905&slot=x86_64-archlinux-gcc-ubsan
> > Reported-by: James Almer <jamrial@gmail.com>
> > ---
> >  libavcodec/hevcdsp_template.c | 6 +++---
> >  libavcodec/x86/hevcdsp_init.c | 6 +++---
> >  2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/hevcdsp_template.c
> b/libavcodec/hevcdsp_template.c
> > index 9b48bdf08e..a4b8901e5d 100644
> > --- a/libavcodec/hevcdsp_template.c
> > +++ b/libavcodec/hevcdsp_template.c
> > @@ -301,9 +301,9 @@ IDCT_DC(32)
> >  //
> >
> ////////////////////////////////////////////////////////////////////////////////
> >  #define ff_hevc_pel_filters ff_hevc_qpel_filters
> > -#define DECL_HV_FILTER(f)                                  \
> > -    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
> > -    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
> > +#define DECL_HV_FILTER(f)
>  \
> > +    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;
> \
> > +    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;
> >
> >  #define FW_PUT(p, f, t)
>                                    \
> >  static void FUNC(put_hevc_## f)(int16_t *dst, const uint8_t *src,
> ptrdiff_t srcstride, int height,        \
> > diff --git a/libavcodec/x86/hevcdsp_init.c
> b/libavcodec/x86/hevcdsp_init.c
> > index 31e81eb11f..e0f65177c4 100644
> > --- a/libavcodec/x86/hevcdsp_init.c
> > +++ b/libavcodec/x86/hevcdsp_init.c
> > @@ -87,9 +87,9 @@ IDCT_FUNCS(avx)
> >
> >
> >  #define ff_hevc_pel_filters ff_hevc_qpel_filters
> > -#define DECL_HV_FILTER(f)                                  \
> > -    const uint8_t *hf = ff_hevc_ ## f ## _filters[mx - 1]; \
> > -    const uint8_t *vf = ff_hevc_ ## f ## _filters[my - 1];
> > +#define DECL_HV_FILTER(f)
>  \
> > +    const uint8_t *hf = mx ? ff_hevc_ ## f ## _filters[mx - 1] : NULL;
> \
> > +    const uint8_t *vf = my ? ff_hevc_ ## f ## _filters[my - 1] : NULL;
> \
>
> Spurious addition of trailing '\'.

oh, you are so careful

>


> >
> >  #define FW_PUT(p, a, b, depth, opt) \
> >  void ff_hevc_put_hevc_ ## a ## _ ## depth ## _##opt(int16_t *dst, const
> uint8_t *src, ptrdiff_t srcstride,   \
>
> I have just sent an alternative solution for this that avoids all these
> branches (which the compiler will not be able to eliminate).
>
Think about it for about 3 seconds, then give up :)
👍

> Unfortunately I can't test mips and loongarch myself; there is a
> fate-runner for the latter, but nothing for the former. So hopefully
> someone can test mips.
>

> - Andreas
>
> PS: The aarch64 code (hevcdsp_[eq]pel_neon.S) already offsets its
> filters via dummy arrays. I wonder whether it could now reuse the
> ordinary ones.
>
> _______________________________________________
> 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".
>
_______________________________________________
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".