diff mbox series

[FFmpeg-devel] avcodec/vvcdec: check pred flag to fix undefined beavhiours

Message ID TYSPR06MB6433EB106999113B6C788B8FAA5C2@TYSPR06MB6433.apcprd06.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel] avcodec/vvcdec: check pred flag to fix undefined beavhiours | 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

Nuo Mi March 3, 2024, 3:31 a.m. UTC 
libavcodec/vvc/vvc_inter.c:823:18: runtime error: signed integer overflow: 1426128896 + 1426128896 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/vvc/vvc_inter.c:823:18
---
 libavcodec/vvc/vvc_inter.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

Comments

Andreas Rheinhardt March 3, 2024, 6:45 a.m. UTC | #1 
Nuo Mi:
> libavcodec/vvc/vvc_inter.c:823:18: runtime error: signed integer overflow: 1426128896 + 1426128896 cannot be represented in type 'int'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/vvc/vvc_inter.c:823:18
> ---
>  libavcodec/vvc/vvc_inter.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/vvc/vvc_inter.c b/libavcodec/vvc/vvc_inter.c
> index d5be32aa14..48c566b097 100644
> --- a/libavcodec/vvc/vvc_inter.c
> +++ b/libavcodec/vvc/vvc_inter.c
> @@ -816,13 +816,16 @@ static void derive_affine_mvc(MvField *mvc, const VVCFrameContext *fc, const MvF
>      const int hs = fc->ps.sps->hshift[1];
>      const int vs = fc->ps.sps->vshift[1];
>      const MvField* mv2 = ff_vvc_get_mvf(fc, x0 + hs * sbw, y0 + vs * sbh);
> +
>      *mvc = *mv;
> -    mvc->mv[0].x += mv2->mv[0].x;
> -    mvc->mv[0].y += mv2->mv[0].y;
> -    mvc->mv[1].x += mv2->mv[1].x;
> -    mvc->mv[1].y += mv2->mv[1].y;
> -    ff_vvc_round_mv(mvc->mv + 0, 0, 1);
> -    ff_vvc_round_mv(mvc->mv + 1, 0, 1);
> +    for (int i = L0; i <= L1; i++) {
> +        PredFlag mask = 1 << i;
> +        if (mv2->pred_flag & mask) {
> +            mvc->mv[i].x += mv2->mv[i].x;
> +            mvc->mv[i].y += mv2->mv[i].y;
> +            ff_vvc_round_mv(mvc->mv + i, 0, 1);
> +        }
> +    }
>  }
>  
>  static void pred_affine_blk(VVCLocalContext *lc)

This adds branches; the UB alone could also be fixed/suppressed with
casting to unsigned. Would the latter cause any problems or are the
values set here unused lateron?

- Andreas
Frank Plowman March 3, 2024, 11:01 a.m. UTC | #2 
On 03/03/2024 03:31, Nuo Mi wrote:
> libavcodec/vvc/vvc_inter.c:823:18: runtime error: signed integer overflow: 1426128896 + 1426128896 cannot be represented in type 'int'
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior libavcodec/vvc/vvc_inter.c:823:18
> ---
>  libavcodec/vvc/vvc_inter.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/vvc/vvc_inter.c b/libavcodec/vvc/vvc_inter.c
> index d5be32aa14..48c566b097 100644
> --- a/libavcodec/vvc/vvc_inter.c
> +++ b/libavcodec/vvc/vvc_inter.c
> @@ -816,13 +816,16 @@ static void derive_affine_mvc(MvField *mvc, const VVCFrameContext *fc, const MvF
>      const int hs = fc->ps.sps->hshift[1];
>      const int vs = fc->ps.sps->vshift[1];
>      const MvField* mv2 = ff_vvc_get_mvf(fc, x0 + hs * sbw, y0 + vs * sbh);
> +
>      *mvc = *mv;
> -    mvc->mv[0].x += mv2->mv[0].x;
> -    mvc->mv[0].y += mv2->mv[0].y;
> -    mvc->mv[1].x += mv2->mv[1].x;
> -    mvc->mv[1].y += mv2->mv[1].y;
> -    ff_vvc_round_mv(mvc->mv + 0, 0, 1);
> -    ff_vvc_round_mv(mvc->mv + 1, 0, 1);
> +    for (int i = L0; i <= L1; i++) {
> +        PredFlag mask = 1 << i;
> +        if (mv2->pred_flag & mask) {
> +            mvc->mv[i].x += mv2->mv[i].x;
> +            mvc->mv[i].y += mv2->mv[i].y;
> +            ff_vvc_round_mv(mvc->mv + i, 0, 1);
> +        }
> +    }
>  }
>  
>  static void pred_affine_blk(VVCLocalContext *lc)

LGTM.  I happened to have a minimal case which reproduces this handy,
and the patch removes the error.
Nuo Mi March 3, 2024, 2:06 p.m. UTC | #3 
On Sun, Mar 3, 2024 at 2:43 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> Nuo Mi:
> > libavcodec/vvc/vvc_inter.c:823:18: runtime error: signed integer
> overflow: 1426128896 + 1426128896 cannot be represented in type 'int'
> > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> libavcodec/vvc/vvc_inter.c:823:18
> > ---
> >  libavcodec/vvc/vvc_inter.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavcodec/vvc/vvc_inter.c b/libavcodec/vvc/vvc_inter.c
> > index d5be32aa14..48c566b097 100644
> > --- a/libavcodec/vvc/vvc_inter.c
> > +++ b/libavcodec/vvc/vvc_inter.c
> > @@ -816,13 +816,16 @@ static void derive_affine_mvc(MvField *mvc, const
> VVCFrameContext *fc, const MvF
> >      const int hs = fc->ps.sps->hshift[1];
> >      const int vs = fc->ps.sps->vshift[1];
> >      const MvField* mv2 = ff_vvc_get_mvf(fc, x0 + hs * sbw, y0 + vs *
> sbh);
> > +
> >      *mvc = *mv;
> > -    mvc->mv[0].x += mv2->mv[0].x;
> > -    mvc->mv[0].y += mv2->mv[0].y;
> > -    mvc->mv[1].x += mv2->mv[1].x;
> > -    mvc->mv[1].y += mv2->mv[1].y;
> > -    ff_vvc_round_mv(mvc->mv + 0, 0, 1);
> > -    ff_vvc_round_mv(mvc->mv + 1, 0, 1);
> > +    for (int i = L0; i <= L1; i++) {
> > +        PredFlag mask = 1 << i;
> > +        if (mv2->pred_flag & mask) {
> > +            mvc->mv[i].x += mv2->mv[i].x;
> > +            mvc->mv[i].y += mv2->mv[i].y;
> > +            ff_vvc_round_mv(mvc->mv + i, 0, 1);
> > +        }
> > +    }
> >  }
> >
> >  static void pred_affine_blk(VVCLocalContext *lc)
>
> This adds branches; the UB alone could also be fixed/suppressed with
> casting to unsigned. Would the latter cause any problems or are the
> values set here unused lateron?
>
It will not be used later.
sent a new patch based on your suggestion.
thank you

>
> - Andreas
>
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/vvc/vvc_inter.c b/libavcodec/vvc/vvc_inter.c
index d5be32aa14..48c566b097 100644
--- a/libavcodec/vvc/vvc_inter.c
+++ b/libavcodec/vvc/vvc_inter.c
@@ -816,13 +816,16 @@  static void derive_affine_mvc(MvField *mvc, const VVCFrameContext *fc, const MvF
     const int hs = fc->ps.sps->hshift[1];
     const int vs = fc->ps.sps->vshift[1];
     const MvField* mv2 = ff_vvc_get_mvf(fc, x0 + hs * sbw, y0 + vs * sbh);
+
     *mvc = *mv;
-    mvc->mv[0].x += mv2->mv[0].x;
-    mvc->mv[0].y += mv2->mv[0].y;
-    mvc->mv[1].x += mv2->mv[1].x;
-    mvc->mv[1].y += mv2->mv[1].y;
-    ff_vvc_round_mv(mvc->mv + 0, 0, 1);
-    ff_vvc_round_mv(mvc->mv + 1, 0, 1);
+    for (int i = L0; i <= L1; i++) {
+        PredFlag mask = 1 << i;
+        if (mv2->pred_flag & mask) {
+            mvc->mv[i].x += mv2->mv[i].x;
+            mvc->mv[i].y += mv2->mv[i].y;
+            ff_vvc_round_mv(mvc->mv + i, 0, 1);
+        }
+    }
 }
 
 static void pred_affine_blk(VVCLocalContext *lc)