Last Comment Bug 523816 - valgrind - Invalid Write of size 1 in oggplay_data_handle_cmml_data
: valgrind - Invalid Write of size 1 in oggplay_data_handle_cmml_data
Status: RESOLVED FIXED
: [sg:critical?]
: valgrind, verified1.9.1
Product: Core
Classification: Components
Component: Video/Audio
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Matthew Gregan (:kinetik)
: video.audio
:
:
: 523575 524035
  Show dependency treegraph
 
Reported: 2009-10-22 05:52 PDT by Bob Clary [:bc:]
Modified: 2010-01-24 10:24 PST (History)
15 users (show)
roc: blocking1.9.2+
dveditz: wanted1.9.0.x-
bclary: in‑testsuite+
See Also:
Crash Signature:
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta2-fixed
  .6+
  .6-fixed


Attachments
patch v0 (3.42 KB, patch)
2009-10-22 15:13 PDT, Matthew Gregan (:kinetik)
no flags Details | Diff | Splinter Review
patch v1 (55.31 KB, patch)
2009-10-22 15:29 PDT, Matthew Gregan (:kinetik)
roc: review+
Details | Diff | Splinter Review
patch v2 (55.31 KB, patch)
2009-10-23 12:57 PDT, Matthew Gregan (:kinetik)
roc: approval1.9.2+
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review

Summon comment box

Description Bob Clary [:bc:] 2009-10-22 05:52:29 PDT
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] 2009-10-22 06:33:28 PDT
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] 2009-10-22 06:40:56 PDT
I wonder if this could be the cause of bug 523575.
Comment 3 Matthew Gregan (:kinetik) 2009-10-22 12:28:13 PDT
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) 2009-10-22 15:13:30 PDT
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.
Comment 5 Matthew Gregan (:kinetik) 2009-10-22 15:29:15 PDT
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.
Comment 6 Matthew Gregan (:kinetik) 2009-10-22 15:39:48 PDT
CCing Viktor to take this patch upstream.
Comment 7 Matthew Gregan (:kinetik) 2009-10-22 20:24:07 PDT
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 2009-10-23 10:24:47 PDT
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) 2009-10-23 12:55:42 PDT
http://hg.mozilla.org/mozilla-central/rev/6cd78a2719be
Comment 10 Matthew Gregan (:kinetik) 2009-10-23 12:57:03 PDT
Created attachment 408091 [details] [review]
patch v2

With fixed test, for approval.
Comment 11 Daniel Veditz 2009-10-26 14:38:24 PDT
Comment on attachment 408091 [details] [review]
patch v2

Approved for 1.9.1.5, a=dveditz for release-drivers
Comment 12 Matthew Gregan (:kinetik) 2009-10-26 20:05:25 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4b0edfcf3c84
Comment 13 Matthew Gregan (:kinetik) 2009-10-27 15:35:47 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/14dd26404792
Comment 14 Al Billings [:abillings] 2009-11-20 11:03:55 PST
No test for the 1.9.1 branch?
Comment 15 Matthew Gregan (:kinetik) 2009-11-20 11:52:45 PST
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] 2009-11-20 12:07:54 PST
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.

Note You need to log in before you can comment on or make changes to this bug.