| Message ID | 20190203172102.21339-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 4cde7e62dbaa63eda173e8d24a97d273890f282c |
| Headers | show |
2019-02-03 18:21 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: > Fixes: 1377/clusterfuzz-testcase-minimized-5487049807233024 > Fixes: assertion failure in sbr_sum_square_c() This changes the output, no? Why are the asm implementations not affected? Carl Eugen
On Wed, Feb 06, 2019 at 01:57:37PM +0100, Carl Eugen Hoyos wrote: > 2019-02-03 18:21 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: > > Fixes: 1377/clusterfuzz-testcase-minimized-5487049807233024 > > Fixes: assertion failure in sbr_sum_square_c() > > This changes the output, no? in normal cases, no it should not It may be that one can construct a case thats not yet in the range where the old code was undefined, the new code is defined by 1 in a Quintillion differently and then the decoder output which is not 64bit integers still has a off by 1 difference. That could be, or it might be not, i dont know You can certainly have a differenceof the old code crashing with assertion failure though if thst is considered "different" > Why are the asm implementations not affected? Please correct me if iam wrong but i think only the float but not the integer version of this have been optimized. thx [...]
2019-02-06 20:17 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: > On Wed, Feb 06, 2019 at 01:57:37PM +0100, Carl Eugen Hoyos wrote: >> 2019-02-03 18:21 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: >> > Fixes: 1377/clusterfuzz-testcase-minimized-5487049807233024 >> > Fixes: assertion failure in sbr_sum_square_c() >> >> This changes the output, no? > > in normal cases, no it should not > It may be that one can construct a case thats not yet in the range > where the old code was undefined, the new code is defined by > 1 in a Quintillion differently and then the decoder output which is > not 64bit integers still has a off by 1 difference. > That could be, or it might be not, i dont know > > You can certainly have a differenceof the old code crashing with > assertion failure though if thst is considered "different" > >> Why are the asm implementations not affected? > > Please correct me if iam wrong but > i think only the float but not the integer version > of this have been optimized. Thank you for the explanation! Carl Eugen
On Wed, Feb 06, 2019 at 08:23:15PM +0100, Carl Eugen Hoyos wrote: > 2019-02-06 20:17 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: > > On Wed, Feb 06, 2019 at 01:57:37PM +0100, Carl Eugen Hoyos wrote: > >> 2019-02-03 18:21 GMT+01:00, Michael Niedermayer <michael@niedermayer.cc>: > >> > Fixes: 1377/clusterfuzz-testcase-minimized-5487049807233024 > >> > Fixes: assertion failure in sbr_sum_square_c() > >> > >> This changes the output, no? > > > > in normal cases, no it should not > > It may be that one can construct a case thats not yet in the range > > where the old code was undefined, the new code is defined by > > 1 in a Quintillion differently and then the decoder output which is > > not 64bit integers still has a off by 1 difference. > > That could be, or it might be not, i dont know > > > > You can certainly have a differenceof the old code crashing with > > assertion failure though if thst is considered "different" > > > >> Why are the asm implementations not affected? > > > > Please correct me if iam wrong but > > i think only the float but not the integer version > > of this have been optimized. > > Thank you for the explanation! will apply patchset thanks [...]
diff --git a/libavcodec/sbrdsp_fixed.c b/libavcodec/sbrdsp_fixed.c index 57d98da979..91fa664c08 100644 --- a/libavcodec/sbrdsp_fixed.c +++ b/libavcodec/sbrdsp_fixed.c @@ -34,32 +34,36 @@ static SoftFloat sbr_sum_square_c(int (*x)[2], int n) { SoftFloat ret; - uint64_t accu, round; + uint64_t accu = 0, round; uint64_t accu0 = 0, accu1 = 0, accu2 = 0, accu3 = 0; int i, nz, nz0; unsigned u; + nz = 0; for (i = 0; i < n; i += 2) { - // Larger values are inavlid and could cause overflows of accu. - av_assert2(FFABS(x[i + 0][0]) >> 30 == 0); accu0 += (int64_t)x[i + 0][0] * x[i + 0][0]; - av_assert2(FFABS(x[i + 0][1]) >> 30 == 0); accu1 += (int64_t)x[i + 0][1] * x[i + 0][1]; - av_assert2(FFABS(x[i + 1][0]) >> 30 == 0); accu2 += (int64_t)x[i + 1][0] * x[i + 1][0]; - av_assert2(FFABS(x[i + 1][1]) >> 30 == 0); accu3 += (int64_t)x[i + 1][1] * x[i + 1][1]; + if ((accu0|accu1|accu2|accu3) > UINT64_MAX - INT32_MIN*(int64_t)INT32_MIN || i+2>=n) { + accu0 >>= nz; + accu1 >>= nz; + accu2 >>= nz; + accu3 >>= nz; + while ((accu0|accu1|accu2|accu3) > (UINT64_MAX - accu) >> 2) { + accu0 >>= 1; + accu1 >>= 1; + accu2 >>= 1; + accu3 >>= 1; + accu >>= 1; + nz ++; + } + accu += accu0 + accu1 + accu2 + accu3; + accu0 = accu1 = accu2 = accu3 = 0; + } } - nz0 = 15; - while ((accu0|accu1|accu2|accu3) >> 62) { - accu0 >>= 1; - accu1 >>= 1; - accu2 >>= 1; - accu3 >>= 1; - nz0 --; - } - accu = accu0 + accu1 + accu2 + accu3; + nz0 = 15 - nz; u = accu >> 32; if (u) {
Fixes: 1377/clusterfuzz-testcase-minimized-5487049807233024 Fixes: assertion failure in sbr_sum_square_c() Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/sbrdsp_fixed.c | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-)