[x264-devel] Integer overflow in mb_analyse_intra

[BugMaster]

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];