search

ffvvc / FFmpeg

VVC Decoder for ffmpeg
Other
48 stars 12 forks source link

Fix integer overflow #231

Closed QSXW closed 4 weeks ago

QSXW commented 1 month ago

@nuomi2021 The issue is caused by integer overflow when adding up the sum. Not sure if we need to fix it if the filter from vvc will not trigger the issue.

frankplow commented 1 month ago

@QSXW I think the failure for fuzz ID 35 is unrelated, as it also failed here: https://github.com/ffvvc/FFmpeg/actions/runs/9159404017. Sorry, missed it in 7b8dfd as it is intermittent. Try with https://github.com/ffvvc/tests/pull/37.

nuomi2021 commented 1 month ago

Hi @QSXW , thank you for the patch. Why the ffvvc/main code has no such issue? is any bug in unit test like not clip to pixel?

QSXW commented 1 month ago

@QSXW I think the failure for fuzz ID 35 is unrelated, as it also failed here: https://github.com/ffvvc/FFmpeg/actions/runs/9159404017. Sorry, missed it in 7b8dfd as it is intermittent. Try with ffvvc/tests#37.

Okay. Thanks for this.

QSXW commented 1 month ago

Hi @QSXW , thank you for the patch. Why the ffvvc/main code has no such issue? is any bug in unit test like not clip to pixel?

I tested and the ffvvc/main code can reproduce the issue with the specific random provided by James.

The filter generated by specific key will cause the sum to be a huge number that is bigger than 32769, a signed word.

nuomi2021 commented 1 month ago

Hi @QSXW , thank you for the patch. Why the ffvvc/main code has no such issue? is any bug in unit test like not clip to pixel?

I tested and the ffvvc/main code can reproduce the issue with the specific random provided by James.

The filter generated by specific key will cause the sum to be a huge number that is bigger than 32769, a signed word.

👍, please send to upstream. C code has no such issue, right?

thank you

QSXW commented 1 month ago

Hi @QSXW , thank you for the patch. Why the ffvvc/main code has no such issue? is any bug in unit test like not clip to pixel?

I tested and the ffvvc/main code can reproduce the issue with the specific random provided by James. The filter generated by specific key will cause the sum to be a huge number that is bigger than 32769, a signed word.

👍, please send to upstream. C code has no such issue, right?

thank you

Yes. If you convert the sum to int16_t before add up to the curr, you will find it match the alf asm.

if (!is_near_vb)
    sum = (sum + offset) >> shift;
else
    sum = (sum + (1 << ((shift + 3) - 1))) >> (shift + 3);
sum += curr;
dst[j] = CLIP(sum);
nuomi2021 commented 4 weeks ago

merged as https://github.com/FFmpeg/FFmpeg/commit/442e94e5e476710dfbb9f611a86ac3c86156394c thank you