Last Comment Bug 746891 - Opus crash [@opus_decoder_get_nb_samples]
: Opus crash [@opus_decoder_get_nb_samples]
: crash, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: x86_64 Mac OS X
-- critical (vote)
: ---
Assigned To: Ralph Giles (:rillian) | needinfo me
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
Blocks: fuzzing-opus
  Show dependency treegraph
Reported: 2012-04-18 23:14 PDT by Christoph Diehl [:posidron]
Modified: 2012-06-14 10:22 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

callstack (4.43 KB, text/plain)
2012-04-18 23:14 PDT, Christoph Diehl [:posidron]
no flags Details
testcase (6.16 KB, text/plain)
2012-04-18 23:15 PDT, Christoph Diehl [:posidron]
no flags Details
guard ReconstructGranulepos in nsOpusState::PageIn (1.52 KB, patch)
2012-04-27 10:30 PDT, Ralph Giles (:rillian) | needinfo me
no flags Details | Diff | Splinter Review

Description User image Christoph Diehl [:posidron] 2012-04-18 23:14:42 PDT
Created attachment 616465 [details]

Looks like a Opus decoder core bug (null dereference crash)

dec is 0x0

int opus_decoder_get_nb_samples(const OpusDecoder *dec,
      const unsigned char packet[], int len)
   int samples;
   int count = opus_packet_get_nb_frames(packet, len);

   if (count<0)
      return count;

   samples = count*opus_packet_get_samples_per_frame(packet, dec->Fs); // <--
   /* Can't have more than 120 ms */
   if (samples*25 > dec->Fs*3)
      return samples;
Comment 1 User image Christoph Diehl [:posidron] 2012-04-18 23:15:14 PDT
Created attachment 616466 [details]
Comment 2 User image Ralph Giles (:rillian) | needinfo me 2012-04-25 15:12:54 PDT
The author's tell me it is intentional that opus_decoder_get_nb_samples() and friends do not check argument validity, to make it more obvious when the caller isn't being careful.

So I need to find where mDecoder isn't being initialized.
Comment 3 User image Ralph Giles (:rillian) | needinfo me 2012-04-27 10:30:03 PDT
Created attachment 619092 [details] [diff] [review]
guard ReconstructGranulepos in nsOpusState::PageIn

I was failing to guard the ReconstructGranulepos in nsOpusState::PageIn with mDoneReadingHeaders. This ensures the decoder context is available when we ask it to calculate packet lengths.

I didn't realize PageIn was called before the headers are parsed, but nsOggReader::ReadMetadata does so. The other nsOggCodecState classes already guard this way.

Helpful report, thank you!
Comment 4 User image Ralph Giles (:rillian) | needinfo me 2012-05-02 09:10:04 PDT
Rolled into opus-decode10.patch in bug 674225, prior to that patch landing on m-c.

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