523816 - valgrind - Invalid Write of size 1 in oggplay_data_handle_cmml_data

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bob Clary [:bc] (inactive)]

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)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems like maybe the "size += 1" should be removed, and instead "size + 1" should be passed to oggplay_check_add_overflow.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I wonder if this could be the cause of bug 523575.

Comment 3

[Matthew Gregan [:kinetik]]

It probably is.  It also reads out of bounds from the data passed in, which fixing the size calculation as you suggest will fix.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: 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.

Comment 6

[Matthew Gregan [:kinetik]]

CCing Viktor to take this patch upstream.

Comment 7

[Matthew Gregan [:kinetik]]

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.

Comment 8

[Daniel Veditz [:dveditz]]

I don't see how this could possibly block a 1.9.0.x release, assuming dbaron meant to nominate for blocking1.9.1

Comment 9

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/6cd78a2719be

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: patch v2

With fixed test, for approval.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 408091 [details] [diff] [review]
patch v2

Approved for 1.9.1.5, a=dveditz for release-drivers

Comment 12

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b0edfcf3c84

Comment 13

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14dd26404792

Comment 14

[Al Billings [:abillings - ex-MoCo]]

No test for the 1.9.1 branch?

Comment 15

[Matthew Gregan [:kinetik]]

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.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

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.