Opened 8 years ago

Closed 8 years ago

#5809 closed defect (fixed)

av_parser_parse2 overflows input buffer (mpegaudio)

Reported by: driegel Owned by:
Priority: minor Component: documentation
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: yes
Analyzed by developer: no

Description

Summary of the bug:
When parsing mpegaudio, av_parser_parse2 doesn't respect input buffer size and read beyond its limit. This overflow can lead to crashes.

FFmpeg version:
n3.1.2 (built from sources)

How to reproduce:
Compile with make (adjusted the Makefile if necessary), and run output binary with valgrind.

Valgrind output:

$ valgrind ./test 
==9820== Memcheck, a memory error detector
==9820== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==9820== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==9820== Command: ./test
==9820== 
==9820== Invalid read of size 8
==9820==    at 0x4C326C8: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x55CED85: memcpy (string3.h:53)
==9820==    by 0x55CED85: ff_combine_frame (parser.c:298)
==9820==    by 0x55657F8: mpegaudio_parse (mpegaudio_parser.c:111)
==9820==    by 0x55CE9A0: av_parser_parse2 (parser.c:182)
==9820==    by 0x400D0A: main (test.c:67)
==9820==  Address 0x76a2258 is 0 bytes after a block of size 40 alloc'd
==9820==    at 0x4C2FFC6: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x4C300D1: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x63016CF: av_malloc (mem.c:97)
==9820==    by 0x400C4D: main (test.c:53)
==9820== 
==9820== Invalid read of size 8
==9820==    at 0x4C326D6: memcpy@@GLIBC_2.14 (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x55CED85: memcpy (string3.h:53)
==9820==    by 0x55CED85: ff_combine_frame (parser.c:298)
==9820==    by 0x55657F8: mpegaudio_parse (mpegaudio_parser.c:111)
==9820==    by 0x55CE9A0: av_parser_parse2 (parser.c:182)
==9820==    by 0x400D0A: main (test.c:67)
==9820==  Address 0x76a2f80 is 8 bytes after a block of size 40 alloc'd
==9820==    at 0x4C2FFC6: memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x4C300D1: posix_memalign (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9820==    by 0x63016CF: av_malloc (mem.c:97)
==9820==    by 0x400C4D: main (test.c:53)
==9820== 
==9820== 
==9820== HEAP SUMMARY:
==9820==     in use at exit: 40 bytes in 1 blocks
==9820==   total heap usage: 734 allocs, 733 frees, 67,939 bytes allocated
==9820== 
==9820== LEAK SUMMARY:
==9820==    definitely lost: 0 bytes in 0 blocks
==9820==    indirectly lost: 0 bytes in 0 blocks
==9820==      possibly lost: 0 bytes in 0 blocks
==9820==    still reachable: 40 bytes in 1 blocks
==9820==         suppressed: 0 bytes in 0 blocks
==9820== Rerun with --leak-check=full to see details of leaked memory
==9820== 
==9820== For counts of detected and suppressed errors, rerun with: -v
==9820== ERROR SUMMARY: 92 errors from 2 contexts (suppressed: 0 from 0)

Quick link to checkout the code and run test case:
https://github.com/d-k-c/ffmpeg-av_parser_parse2-bug

Attachments (4)

encoded_audio.c (164.8 KB ) - added by driegel 8 years ago.
mp2-encoded audio dump
test.c (1.6 KB ) - added by driegel 8 years ago.
test case to reproduce the bug
Makefile (455 bytes ) - added by driegel 8 years ago.
patchparserpadding.diff (995 bytes ) - added by Carl Eugen Hoyos 8 years ago.

Download all attachments as: .zip

Change History (10)

by driegel, 8 years ago

Attachment: encoded_audio.c added

mp2-encoded audio dump

by driegel, 8 years ago

Attachment: test.c added

test case to reproduce the bug

by driegel, 8 years ago

Attachment: Makefile added

comment:1 by Carl Eugen Hoyos, 8 years ago

Component: avcodecundetermined
Resolution: invalid
Status: newclosed

I believe the documentation requires you to pad the input buffer.
For future tickets, please remember to test current FFmpeg git head before reporting issues here.

in reply to:  1 comment:2 by driegel, 8 years ago

Replying to cehoyos:

I believe the documentation requires you to pad the input buffer.

The documentation doesn't state any requirement about the input buffer. Compilation was broken with git head and the file (mpegaudio_parser.c) hasn't changed since the release I used, so I supposed it was a good approximate.

comment:3 by Carl Eugen Hoyos, 8 years ago

Resolution: invalid
Status: closedreopened

The documentation of av_parser_parse2() doesn't mention padding, I don't know if there is an issue.

comment:4 by Carl Eugen Hoyos, 8 years ago

Not reproducible with ffmpeg because av_grow_packet() does indeed pad each AVPacket with AV_INPUT_BUFFER_PADDING_SIZE.

by Carl Eugen Hoyos, 8 years ago

Attachment: patchparserpadding.diff added

comment:5 by Michael Niedermayer, 8 years ago

IIRC the parsers always were written on the assumtation that the input is padded. So to me this looks like a documentation issue

Of course if someone wants to change all parsers to not require padding that would e welcome if its clean and has no speed penalty

comment:6 by Carl Eugen Hoyos, 8 years ago

Component: undetermineddocumentation
Priority: normalminor
Reproduced by developer: set
Resolution: fixed
Status: reopenedclosed
Version: unspecifiedgit-master

Documentation improved in 7a8e5ff1fd06c0363ed4bb26cda5262fcd925b74, thank you for the report!

Note: See TracTickets for help on using tickets.