Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#3152 closed defect (fixed)

Pointer overflow in libavcodec/mpegvideo.c

Reported by: dtzWill Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords: svq1
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

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!

Attachments (1)

0001-mpegvideo-Add-check-for-pointer-overflow-to-demonstr.patch (1.2 KB) - added by dtzWill 6 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 6 years ago by cehoyos

  • Keywords undefined overflow removed

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

comment:2 Changed 6 years ago by dtzWill

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.

comment:3 Changed 6 years ago by cehoyos

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?

comment:4 Changed 6 years ago by dtzWill

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!

comment:5 Changed 6 years ago by michael

  • Resolution set to fixed
  • Status changed from new to closed

comment:6 Changed 6 years ago by cehoyos

  • Keywords svq1 added
Note: See TracTickets for help on using tickets.