Bugzilla@Mozilla – Bug 523816
valgrind - Invalid Write of size 1 in oggplay_data_handle_cmml_data
Last modified: 2010-01-24 10:24:54 PST
Summon comment box
valgrind mochitest linux mozilla-central. valgrind 3.2.1, built with --enable-valgrind, not run with --smc-check=all Invalid write of size 1 terminating record data string. http://mxr.mozilla.org/mozilla-central/source/media/liboggplay/src/liboggplay/oggplay_data.c#391 Occurs after content/media/test/test_playback_errors.html Invalid write of size 1 at 0x71A4D35: oggplay_data_handle_cmml_data (oggplay_data.c:391) by 0x71A32C8: oggplay_callback_cmml (oggplay_callback.c:396) by 0x71B780C: oggz_read_sync (oggz_read.c:491) by 0x71B7C0A: oggz_read (oggz_read.c:612) by 0x71A23BA: oggplay_step_decoding (oggplay.c:744) by 0x71E7A2D: nsOggDecodeStateMachine::DecodeFrame() (nsOggDecoder.cpp:748) by 0x71E9F05: nsOggDecodeStateMachine::Run() (nsOggDecoder.cpp:1434) by 0x42FE99E: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527) by 0x428607C: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:230) by 0x42FF4AF: nsThread::ThreadFunc(void*) (nsThread.cpp:254) by 0x439C261: _pt_root (ptthread.c:228) by 0x8FD49A: start_thread (in /lib/libpthread-2.5.so) Address 0x10E245F6 is 0 bytes after a block of size 62 alloc'd at 0x40046FF: calloc (vg_replace_malloc.c:279) by 0x71A4CD2: oggplay_data_handle_cmml_data (oggplay_data.c:378) by 0x71A32C8: oggplay_callback_cmml (oggplay_callback.c:396) by 0x71B780C: oggz_read_sync (oggz_read.c:491) by 0x71B7C0A: oggz_read (oggz_read.c:612) by 0x71A23BA: oggplay_step_decoding (oggplay.c:744) by 0x71E7A2D: nsOggDecodeStateMachine::DecodeFrame() (nsOggDecoder.cpp:748) by 0x71E9F05: nsOggDecodeStateMachine::Run() (nsOggDecoder.cpp:1434) by 0x42FE99E: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527) by 0x428607C: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.cpp:230) by 0x42FF4AF: nsThread::ThreadFunc(void*) (nsThread.cpp:254) by 0x439C261: _pt_root (ptthread.c:228)
Seems like maybe the "size += 1" should be removed, and instead "size + 1" should be passed to oggplay_check_add_overflow.
I wonder if this could be the cause of bug 523575.
It probably is. It also reads out of bounds from the data passed in, which fixing the size calculation as you suggest will fix.
Created attachment 407862 [details] [review] patch v0 Fix calculation of record size. Fixes a read past the end of data, and a write past the end of record->data. This also fixes bug 523575. I'll ask roc for review since doublec is away for the long weekend.
Created attachment 407865 [details] [review] patch v1 Same, but add a valid testfile, since the only file we have that trigger this previously is invalid and the handling of invalid files might change so that we don't exercise this code path.
CCing Viktor to take this patch upstream.
Crud, the duration specified in manifest.js for the test file I added is incorrect. I'll fix that before I check in the patch.
I don't see how this could possibly block a 1.9.0.x release, assuming dbaron meant to nominate for blocking1.9.1
http://hg.mozilla.org/mozilla-central/rev/6cd78a2719be
Created attachment 408091 [details] [review] patch v2 With fixed test, for approval.
Comment on attachment 408091 [details] [review] patch v2 Approved for 1.9.1.5, a=dveditz for release-drivers
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b0edfcf3c84
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14dd26404792
No test for the 1.9.1 branch?
The media test framework is quite different between 1.92/trunk and 1.9.1, so we haven't been backporting all of the tests. In this case, you can manually test by loading the test file attached to bug 523575.
Thanks. Verified using the testcase in bug 523575 for 1.9.1. On Windows, I verified the crash on 1.9.1.5 and then the lack of a crash with 1.9.1.6 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729)). On Linux, with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.6pre) Gecko/20091120 Shiretoko/3.5.6pre.