Pointer overflow in libavcodec/mpegvideo.c

[Will Dietz]

Summary of the bug:

mpegvideo.c invokes undefined behavior by causing a pointer to overflow.

This occurs in libavcodec/mpegvideo.c:3010:47 of ffmpeg 2.0.2 and at
libavcodec/mpegvideo.c:3018:47 on latest git as of commit f1f0b01c4700ae342bb245efcc00a724fd270c14.

Here's the error report, produced by coming-soon-to-you -fsanitize=pointer-overflow in clang:

libavcodec/mpegvideo.c:3018:47: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffff0

This occurs during execution of the "vsynth1-svq1" test (and only this test) during execution of the FATE test suite.

How to reproduce:

• Build ffmpeg with clang using -fsanitize=pointer-overflow -fno-sanitize-recover
• Run fate test-suite
• Observe test failure, look in "./tests/data/fate/vsynth1-svq1.err" for an error report like the above.

Alternatively, since this sanitizer is not yet included in clang mainline, simply add a check to mpegvideo.c:3018 to report if the LHS is zero when the RHS is negative.

Please let me know if more information is required, thanks!

[Carl Eugen Hoyos]

Line 3018 in libavcodec/mpegvideo.c is a closing bracket here.
Please provide the patch with the check that allows to reproduce the problem.

[Will Dietz]

Ah, the code has changed.

Re-running with latest git (commit d5d29ae3b0375320a7a34f85a5a90e8362550dbb) gives this new location:

libavcodec/mpegvideo.c:3049:47: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xfffffffffffffff0

Attaching patch that reports the issue to help reproduce the issue.

[Carl Eugen Hoyos]

s->current_picture.f.data[0] and s->mb_x are both 0 when running fate-vsynth1-svq1, the line in question is:

s->dest[0] = s->current_picture.f.data[0] + ((s->mb_x - 1) <<  mb_size);

Does "undefined behaviour" mean that s->dest[0] has no defined content after the operation or that the operation may eat your cat?

[Will Dietz]

No guarantee the operation won't eat your cat, unfortunately (undefined, not implementation-defined). Note that indexing from NULL even in the positive direction is also undefined despite not overflowing.

Unfortunately this is not purely an academic concern, compilers have been known to take advantage of the assumption that pointer overflow cannot occur (although I'm unsure of what optimization might be made here).

As an aside it looks like ff_update_block_index wraps s->dest[0] around again, in case that's useful for devising a solution.

Hopefully these checks make it into -fsanitize=undefined soon to facilitate finding and correcting these issues!

[Michael Niedermayer]

Fixed in a81ee2651fafd4412d0f8df159a3a11f6365ba70