<div dir="ltr"><div dir="ltr"><div>Thanks. Can we push this change to x264 upstream then to fix UndefinedBehaviorSanitizer error ?<br><div>---</div><div> encoder/analyse.c | 3 ++-</div><div> 1 file changed, 2 insertions(+), 1 deletion(-)</div><div><br></div><div>diff --git a/encoder/analyse.c b/encoder/analyse.c</div><div>index fb241a7d..f04cbb52 100644</div><div>--- a/encoder/analyse.c</div><div>+++ b/encoder/analyse.c</div><div>@@ -786,7 +786,8 @@ static void mb_analyse_intra( x264_t *h, x264_mb_analysis_t *a, int i_satd_inter</div><div> ALIGNED_ARRAY_16( int32_t, satd,[9] );</div><div> h->pixf.intra_mbcmp_x3_8x8( p_src_by, edge, satd );</div><div> int favor_vertical = satd[I_PRED_4x4_H] > satd[I_PRED_4x4_V];</div><div>- satd[i_pred_mode] -= 3 * lambda;</div><div>+ if( i_pred_mode < 3 )</div><div>+ satd[i_pred_mode] -= 3 * lambda;</div><div> for( int i = 2; i >= 0; i-- )</div><div> {</div><div> int cost = satd[i];</div><div>-- </div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Mar 6, 2019 at 9:28 AM BugMaster <<a href="mailto:BugMaster@narod.ru">BugMaster@narod.ru</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, 6 Mar 2019 20:24:33 +0300, BugMaster wrote:<br>
> On Wed, 6 Mar 2019 11:37:14 -0500, Jeremy Dorfman wrote:<br>
>> While running x264 under Clang's UndefinedBehaviorSanitizer, I<br>
>> found a few videos and settings that trigger an integer overflow in<br>
>> mb_analyse_intra. The following line can occasionally try to take<br>
>> satd[i_pred_mode] below INT_MIN (presently encoder/analyse.c:907):<br>
>> satd[i_pred_mode] -= 3 * lambda;<br>
<br>
<br>
>> I've locally worked around this by using an int64_t and clamping<br>
>> the result back to an int32_t, however I'd imagine that's not<br>
>> desirable for 32-bit targets. That said, I've appended the patch. If<br>
>> this looks good, can someone merge it? If not, can someone help me<br>
>> fix this in a more appropriate way?<br>
<br>
<br>
>> Thanks,<br>
>> -Jeremy<br>
<br>
<br>
>> diff --git a/encoder/analyse.c b/encoder/analyse.c<br>
>> index fb241a7d..564fe73a 100644<br>
>> --- a/encoder/analyse.c<br>
>> +++ b/encoder/analyse.c<br>
>> @@ -904,7 +904,8 @@ static void mb_analyse_intra( x264_t *h,<br>
>> x264_mb_analysis_t *a, int i_satd_inter<br>
>> ALIGNED_ARRAY_16( int32_t, satd,[9] );<br>
>> h->pixf.intra_mbcmp_x3_4x4( p_src_by, p_dst_by, satd );<br>
>> int favor_vertical = satd[I_PRED_4x4_H] > satd[I_PRED_4x4_V];<br>
>> - satd[i_pred_mode] -= 3 * lambda;<br>
>> + int64_t new_satd = (int64_t)satd[i_pred_mode] - 3 * (int64_t)lambda;<br>
>> + satd[i_pred_mode] = X264_MAX(INT_MIN, new_satd);<br>
>> i_best = satd[I_PRED_4x4_DC];<br>
>> a->i_predict4x4[idx] = I_PRED_4x4_DC;<br>
>> COPY2_IF_LT( i_best, satd[I_PRED_4x4_H],<br>
>> a->i_predict4x4[idx], I_PRED_4x4_H );<br>
>> COPY2_IF_LT( i_best, satd[I_PRED_4x4_V],<br>
>> a->i_predict4x4[idx], I_PRED_4x4_V );<br>
<br>
> Hi.<br>
<br>
> Most probably this only happen with (i_pred_mode >= 3) where<br>
> satd[i_pred_mode] can be uninitialized because intra_mbcmp_x3_4x4<br>
> initialize only values 0..2. This was made intentional as optimization<br>
> (uncoditional subtraction) because latter only values 0..2 are used in<br>
> loop.<br>
<br>
> So I am not sure if this really need fix because there is no bug here.<br>
<br>
But if you want to shut up UndefinedBehaviorSanitizer than it is<br>
probably better to make:<br>
int favor_vertical = satd[I_PRED_4x4_H] > satd[I_PRED_4x4_V];<br>
- satd[i_pred_mode] -= 3 * lambda;<br>
+ if( i_pred_mode < 3 )<br>
+ satd[i_pred_mode] -= 3 * lambda;<br>
i_best = satd[I_PRED_4x4_DC];<br>
<br>
_______________________________________________<br>
x264-devel mailing list<br>
<a href="mailto:x264-devel@videolan.org" target="_blank">x264-devel@videolan.org</a><br>
<a href="https://mailman.videolan.org/listinfo/x264-devel" rel="noreferrer" target="_blank">https://mailman.videolan.org/listinfo/x264-devel</a><br>
</blockquote></div></div></div>