[x264-devel] Integer overflow in mb_analyse_intra

[Sasi Inguva]

Thanks. Can we push this change to x264 upstream then to fix
UndefinedBehaviorSanitizer error ?
---
 encoder/analyse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/encoder/analyse.c b/encoder/analyse.c
index fb241a7d..f04cbb52 100644
--- a/encoder/analyse.c
+++ b/encoder/analyse.c
@@ -786,7 +786,8 @@ static void mb_analyse_intra( x264_t *h,
x264_mb_analysis_t *a, int i_satd_inter
                     ALIGNED_ARRAY_16( int32_t, satd,[9] );
                     h->pixf.intra_mbcmp_x3_8x8( p_src_by, edge, satd );
                     int favor_vertical = satd[I_PRED_4x4_H] >
satd[I_PRED_4x4_V];
-                    satd[i_pred_mode] -= 3 * lambda;
+                    if( i_pred_mode < 3 )
+                      satd[i_pred_mode] -= 3 * lambda;
                     for( int i = 2; i >= 0; i-- )
                     {
                         int cost = satd[i];
-- 


On Wed, Mar 6, 2019 at 9:28 AM BugMaster <BugMaster at narod.ru> wrote:

> On Wed, 6 Mar 2019 20:24:33 +0300, BugMaster wrote:
> > On Wed, 6 Mar 2019 11:37:14 -0500, Jeremy Dorfman wrote:
> >> While running x264 under Clang's UndefinedBehaviorSanitizer, I
> >> found a few videos and settings that trigger an integer overflow in
> >> mb_analyse_intra. The following line can occasionally try to take
> >> satd[i_pred_mode] below INT_MIN (presently encoder/analyse.c:907):
> >>     satd[i_pred_mode] -= 3 * lambda;
>
>
> >> I've locally worked around this by using an int64_t and clamping
> >> the result back to an int32_t, however I'd imagine that's not
> >> desirable for 32-bit targets. That said, I've appended the patch. If
> >> this looks good, can someone merge it? If not, can someone help me
> >> fix this in a more appropriate way?
>
>
> >> Thanks,
> >> -Jeremy
>
>
> >> diff --git a/encoder/analyse.c b/encoder/analyse.c
> >> index fb241a7d..564fe73a 100644
> >> --- a/encoder/analyse.c
> >> +++ b/encoder/analyse.c
> >> @@ -904,7 +904,8 @@ static void mb_analyse_intra( x264_t *h,
> >> x264_mb_analysis_t *a, int i_satd_inter
> >>                      ALIGNED_ARRAY_16( int32_t, satd,[9] );
> >>                      h->pixf.intra_mbcmp_x3_4x4( p_src_by, p_dst_by,
> satd );
> >>                      int favor_vertical = satd[I_PRED_4x4_H] >
> satd[I_PRED_4x4_V];
> >> -                    satd[i_pred_mode] -= 3 * lambda;
> >> +                    int64_t new_satd = (int64_t)satd[i_pred_mode] - 3
> * (int64_t)lambda;
> >> +                    satd[i_pred_mode] = X264_MAX(INT_MIN, new_satd);
> >>                      i_best = satd[I_PRED_4x4_DC];
> >> a->i_predict4x4[idx] = I_PRED_4x4_DC;
> >>                      COPY2_IF_LT( i_best, satd[I_PRED_4x4_H],
> >> a->i_predict4x4[idx], I_PRED_4x4_H );
> >>                      COPY2_IF_LT( i_best, satd[I_PRED_4x4_V],
> >> a->i_predict4x4[idx], I_PRED_4x4_V );
>
> > Hi.
>
> > Most probably this only happen with (i_pred_mode >= 3) where
> > satd[i_pred_mode] can be uninitialized because intra_mbcmp_x3_4x4
> > initialize only values 0..2. This was made intentional as optimization
> > (uncoditional subtraction) because latter only values 0..2 are used in
> > loop.
>
> > So I am not sure if this really need fix because there is no bug here.
>
> But if you want to shut up UndefinedBehaviorSanitizer than it is
> probably better to make:
>                      int favor_vertical = satd[I_PRED_4x4_H] >
> satd[I_PRED_4x4_V];
> -                    satd[i_pred_mode] -= 3 * lambda;
> +                    if( i_pred_mode < 3 )
> +                        satd[i_pred_mode] -= 3 * lambda;
>                      i_best = satd[I_PRED_4x4_DC];
>
> _______________________________________________
> x264-devel mailing list
> x264-devel at videolan.org
> https://mailman.videolan.org/listinfo/x264-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/x264-devel/attachments/20190424/6ee492cc/attachment.html>