[x264-devel] Integer overflow in mb_analyse_intra

[BugMaster]

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.