As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 752668 - Wrong array access in media/libtheora/lib/decode.c causes crash
: Wrong array access in media/libtheora/lib/decode.c causes crash
: crash
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla15
Assigned To: Timothy B. Terriberry (:derf)
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
Blocks: 507554
  Show dependency treegraph
Reported: 2012-05-07 13:45 PDT by Octoploid
Modified: 2012-10-18 12:23 PDT (History)
11 users (show)
tterribe: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Fix pp_harp_mod calculation (2.01 KB, patch)
2012-05-08 01:17 PDT, Timothy B. Terriberry (:derf)
no flags Details | Diff | Splinter Review
Fix pp_sharp_mod calculation v2 (3.43 KB, patch)
2012-05-08 01:19 PDT, Timothy B. Terriberry (:derf)
kinetik: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Octoploid 2012-05-07 13:45:09 PDT
Firefox compiled with gcc-4.8.0 crashes on the following site , 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/
#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,
 383      one byte for extra-bits for each token, plus one more byte for the
 384      EOB run, just in case it's the very last token and has a run length
 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 User image Mats Palmgren (:mats) 2012-05-07 18:11:14 PDT
Marking Security-Sensitive until it has been analyzed.
Comment 2 User image Timothy B. Terriberry (:derf) 2012-05-07 19:53:24 PDT
(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.
Comment 3 User image Timothy B. Terriberry (:derf) 2012-05-08 01:17:45 PDT
Created attachment 621908 [details] [diff] [review]
Fix pp_harp_mod calculation
Comment 4 User image Timothy B. Terriberry (:derf) 2012-05-08 01:19:38 PDT
Created attachment 621909 [details] [diff] [review]
Fix pp_sharp_mod calculation v2

Whoops, forgot to actually hg add the patch needed by
Comment 5 User image Timothy B. Terriberry (:derf) 2012-05-08 07:29:46 PDT
Comment 6 User image Timothy B. Terriberry (:derf) 2012-05-08 10:08:48 PDT
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:

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.
Comment 7 User image Timothy B. Terriberry (:derf) 2012-05-08 10:13:17 PDT
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).
Comment 8 User image Matthew Gregan [:kinetik] 2012-05-08 14:17:01 PDT
Opening up based on comment 7.
Comment 9 User image Ryan VanderMeulen [:RyanVM] 2012-05-08 18:40:42 PDT
Comment 10 User image Alex Keybl [:akeybl] 2012-05-11 16:39:44 PDT
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.
Comment 11 User image Alex Keybl [:akeybl] 2012-05-11 16:40:11 PDT
That being said, adding qawanted to make sure we test the patch once it's landed.
Comment 13 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-15 12:54:49 PDT
Removing qawanted as we'll test this as part of our normal verifications. Is there a testcase we can use for this?
Comment 14 User image Ralph Giles (:rillian) | needinfo me 2012-05-15 13:05:27 PDT
We don't have anything in the test suite. The file from the original crash report is
Comment 15 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-15 13:17:06 PDT
That should work, thanks Ralph.
Comment 16 User image Virgil Dicu [:virgil] [QA] 2012-05-16 05:09:22 PDT
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 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-04 14:22:17 PDT
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 User image Ralph Giles (:rillian) | needinfo me 2012-06-04 17:04:08 PDT
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 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-04 17:29:00 PDT
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.
Comment 20 User image Ralph Giles (:rillian) | needinfo me 2012-06-05 11:16:01 PDT
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 21 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-05 11:18:09 PDT
Thank you very much Ralph!
Comment 22 User image Ralph Giles (:rillian) | needinfo me 2012-06-05 13:10:59 PDT
Same result for firefox 14 beta, built with gcc 4.8 from mozilla-beta hg rev 95980:206e57393758.
Comment 23 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-05 14:37:41 PDT
Thanks Ralph.

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