Closed
Bug 752668
Opened 12 years ago
Closed 12 years ago
Wrong array access in media/libtheora/lib/decode.c causes crash
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: octoploid, Assigned: derf)
References
Details
(Keywords: crash, Whiteboard: [qa-])
Attachments
(1 file, 1 obsolete file)
3.43 KB,
patch
|
kinetik
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Firefox compiled with gcc-4.8.0 crashes on the following site http://archive.org/details/Eisenstein-October , when one starts the movie.: Program received signal SIGSEGV, Segmentation fault. [Switching to Thread 0x7fffdd171700 (LWP 5185)] 0x00007ffff5e21f1e in oc_dec_init (_setup=0x7fffd31e2800, _info=<optimized out>, _dec=0x7fffd6e56010) at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:403 403 qsum+=_dec->state.dequant_tables[qti][pli][qi][12]+ (gdb) bt #0 0x00007ffff5e21f1e in oc_dec_init (_setup=0x7fffd31e2800, _info=<optimized out>, _dec=0x7fffd6e56010) at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:403 #1 th_decode_alloc (_info=<optimized out>, _setup=0x7fffd31e2800) at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:1963 #2 0x00007ffff5727dbc in Init (this=0x7fffdb902c00) at /var/tmp/mozilla-central/content/media/ogg/nsOggCodecState.cpp:282 #3 nsTheoraState::Init (this=0x7fffdb902c00) at /var/tmp/mozilla-central/content/media/ogg/nsOggCodecState.cpp:264 #4 0x00007ffff572dfa2 in nsOggReader::ReadMetadata (this=0x7fffd9b28000, aInfo=0x7fffdd170ce8) at /var/tmp/mozilla-central/content/media/ogg/nsOggReader.cpp:268 #5 0x00007ffff571d81c in nsBuiltinDecoderStateMachine::DecodeMetadata (this=this@entry=0x7fffd6e48460) at /var/tmp/mozilla-central/content/media/nsBuiltinDecoderStateMachine.cpp:1792 #6 0x00007ffff571e1aa in nsBuiltinDecoderStateMachine::DecodeThreadRun (this=0x7fffd6e48460) at /var/tmp/mozilla-central/content/media/nsBuiltinDecoderStateMachine.cpp:507 #7 0x00007ffff4f58bd7 in nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run (this=<optimized out>) at ../../../dist/include/nsThreadUtils.h:345 #8 0x00007ffff5ca1a8e in nsThread::ProcessNextEvent (this=0x7fffd7855710, mayWait=<optimized out>, result=0x7fffdd170e0f) at /var/tmp/mozilla-central/xpcom/threads/nsThread.cpp:656 #9 0x00007ffff5c62d72 in NS_ProcessNextEvent_P (thread=<optimized out>, mayWait=<optimized out>) at /var/tmp/mozilla-central/moz-build-dir/xpcom/build/nsThreadUtils.cpp:245 #10 0x00007ffff5ca1349 in nsThread::ThreadFunc (arg=0x7fffd7855710) at /var/tmp/mozilla-central/xpcom/threads/nsThread.cpp:289 #11 0x00007ffff4604bc3 in ?? () from /usr/lib64/libnspr4.so #12 0x00007ffff7bc8dff in start_thread (arg=0x7fffdd171700) at pthread_create.c:304 #13 0x00007ffff72a495d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114 From media/libtheora/lib/decode.c: 368 static int oc_dec_init(oc_dec_ctx *_dec,const th_info *_info, 369 const th_setup_info *_setup){ 370 int qti; 371 int pli; 372 int qi; 373 int ret; 374 ret=oc_state_init(&_dec->state,_info,3); 375 if(ret<0)return ret; 376 ret=oc_huff_trees_copy(_dec->huff_tables, 377 (const ogg_int16_t *const *)_setup->huff_tables); 378 if(ret<0){ 379 oc_state_clear(&_dec->state); 380 return ret; 381 } 382 /*For each fragment, allocate one byte for every DCT coefficient token, plus 383 one byte for extra-bits for each token, plus one more byte for the long 384 EOB run, just in case it's the very last token and has a run length of 385 one.*/ 386 _dec->dct_tokens=(unsigned char *)_ogg_malloc((64+64+1)* 387 _dec->state.nfrags*sizeof(_dec->dct_tokens[0])); 388 if(_dec->dct_tokens==NULL){ 389 oc_huff_trees_clear(_dec->huff_tables); 390 oc_state_clear(&_dec->state); 391 return TH_EFAULT; 392 } 393 for(qi=0;qi<64;qi++)for(pli=0;pli<3;pli++)for(qti=0;qti<2;qti++){ 394 _dec->state.dequant_tables[qi][pli][qti]= 395 _dec->state.dequant_table_data[qi][pli][qti]; 396 } 397 oc_dequant_tables_init(_dec->state.dequant_tables,_dec->pp_dc_scale, 398 &_setup->qinfo); 399 for(qi=0;qi<64;qi++){ 400 int qsum; 401 qsum=0; 402 for(qti=0;qti<2;qti++)for(pli=0;pli<3;pli++){ 403 qsum+=_dec->state.dequant_tables[qti][pli][qi][12]+ 404 _dec->state.dequant_tables[qti][pli][qi][17]+ 405 _dec->state.dequant_tables[qti][pli][qi][18]+ 406 _dec->state.dequant_tables[qti][pli][qi][24]<<(pli==0); 407 } 408 _dec->pp_sharp_mod[qi]=-(qsum>>11); 409 } In the for loop (starting on line 402) "state.dequant_tables[qti][pli][qi]" is wrong and should read: "state.dequant_tables[qi][pli][qti]": 402 for(qti=0;qti<2;qti++)for(pli=0;pli<3;pli++){ 403 qsum+=_dec->state.dequant_tables[qi][pli][qti][12]+ 404 _dec->state.dequant_tables[qi][pli][qti][17]+ 405 _dec->state.dequant_tables[qi][pli][qti][18]+ 406 _dec->state.dequant_tables[qi][pli][qti][24]<<(pli==0); 407 }
Comment 1•12 years ago
|
||
Marking Security-Sensitive until it has been analyzed.
Assignee | ||
Updated•12 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Mats Palmgren [:mats] from comment #1) > Marking Security-Sensitive until it has been analyzed. Thanks for the report. Fix committed upstream in r18268. I'll get a patch landed on inbound as soon as I get off the plane I just got on.
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #621908 -
Flags: review?(kinetik)
Assignee | ||
Comment 4•12 years ago
|
||
Whoops, forgot to actually hg add the patch needed by update.sh.
Attachment #621908 -
Attachment is obsolete: true
Attachment #621908 -
Flags: review?(kinetik)
Attachment #621909 -
Flags: review?(kinetik)
Assignee | ||
Updated•12 years ago
|
OS: Linux → All
Hardware: x86_64 → All
Updated•12 years ago
|
Attachment #621909 -
Flags: review?(kinetik) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #621909 -
Attachment description: Fix pp_harp_mod calculation v2 → Fix pp_sharp_mod calculation v2
Assignee | ||
Comment 5•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/81eed0782699
Flags: in-testsuite-
Target Milestone: --- → mozilla15
Assignee | ||
Comment 6•12 years ago
|
||
Comment on attachment 621909 [details] [diff] [review] Fix pp_sharp_mod calculation v2 [Approval Request Comment] Regression caused by (bug #): 507554 User impact if declined: Crash on any page with a Theora file if the browser is compiled with gcc 4.8.0 or later. Testing completed (on m-c, etc.): Landed on try and m-i, test results greenish: https://tbpl.mozilla.org/?tree=Try&rev=f9bedd41b1e4 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=81eed0782699 Risk to taking this patch (and alternatives if risky): Low. The calculations affected here are never used, because we don't enable libtheora's post-processing filters. String changes made by this patch: None.
Attachment #621909 -
Flags: approval-mozilla-beta?
Attachment #621909 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 7•12 years ago
|
||
mats: I'm not sure this really needs to be security-sensitive. The invalid accesses here are only reads, not writes, and the result of the calculation is never used, so I don't think this can trigger anything more than a DoS. Even if I'm wrong, we aren't using a gcc this recent on any official builds, and even the most minimal release testing should uncover this problem (just running the media tests we already have should be sufficient to trigger it).
status-firefox-esr10:
--- → affected
status-firefox12:
--- → affected
status-firefox13:
--- → affected
status-firefox14:
--- → affected
status-firefox15:
--- → affected
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/81eed0782699
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Assignee: nobody → tterribe
Comment 10•12 years ago
|
||
Comment on attachment 621909 [details] [diff] [review] Fix pp_sharp_mod calculation v2 [Triage Comment] Very low risk of there being any user visible regressions from this patch, so we'll approve for Aurora and Beta.
Attachment #621909 -
Flags: approval-mozilla-beta?
Attachment #621909 -
Flags: approval-mozilla-beta+
Attachment #621909 -
Flags: approval-mozilla-aurora?
Attachment #621909 -
Flags: approval-mozilla-aurora+
Comment 11•12 years ago
|
||
That being said, adding qawanted to make sure we test the patch once it's landed.
tracking-firefox13:
--- → +
Keywords: qawanted
Updated•12 years ago
|
status-firefox15:
affected → ---
Updated•12 years ago
|
Keywords: checkin-needed
Comment 12•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/12fa346baa18 https://hg.mozilla.org/releases/mozilla-beta/rev/77191fa1fc78
Comment 13•12 years ago
|
||
Removing qawanted as we'll test this as part of our normal verifications. Is there a testcase we can use for this?
Keywords: qawanted
Whiteboard: [qa+]
Comment 14•12 years ago
|
||
We don't have anything in the test suite. The file from the original crash report is http://ia700707.us.archive.org/20/items/Eisenstein-October/Oktyabr1927.ogv
Comment 15•12 years ago
|
||
That should work, thanks Ralph.
Comment 16•12 years ago
|
||
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0 No crash with a normal Firefox build, but I understand this only occurred with a 4.8.0 compiled Firefox. Is there a Firefox build anyone could point me to without requiring to compile Firefox with gcc 4.8.0?
Comment 17•12 years ago
|
||
Spoke with Ralph and he said you need to build Firefox with GCC 4.8 to reproduce the crash. He's going to get a build going now but it will take some time to build.
Comment 18•12 years ago
|
||
I wasn't able to get a copy of gcc 4.8 built today to confirm the issue is resolved for ff13, but the risk is low: we're confident in the patch, no problems have been seen since commit, and 4.8 is an unreleased compiler.
Comment 19•12 years ago
|
||
Thanks for the effort, Ralph. Marking this [qa-] indicating that we won't be exerting anymore effort trying to verify this bug. We're still open to verifying this if it becomes more necessary and easier to verify.
Whiteboard: [qa+] → [qa-]
Comment 20•12 years ago
|
||
Confirmed for firefox 13. I built this morning's mozilla-release tree hg rev 92659:55ba50a35fd9 with yesterday's gcc 4.8 git rev e8802ff73608. Playing the indicated video works with the the build, and segfaults if I reverse-apply the patch. So the fix is both valid and needed in ff 13.
Comment 22•12 years ago
|
||
Same result for firefox 14 beta, built with gcc 4.8 from mozilla-beta hg rev 95980:206e57393758.
Updated•5 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•