749994 - Opus crash invalid read/write [@nsOggReader::DecodeOpus]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

gdb $ list
450	    PRInt32 skip = mOpusState->mPreSkip;
451	    PRInt32 goodFrames = frames - skip;
452	    NS_ASSERTION(goodFrames > 0, "endTime calculation was wrong");
453	    nsAutoArrayPtr<AudioDataValue> goodBuffer(new AudioDataValue[goodFrames * channels]);
454	    for (int i = 0; i < goodFrames*channels; i++)
455	      goodBuffer[i] = buffer[skip*channels + i];
456	
457	    startTime = mOpusState->Time(endFrame - goodFrames);
458	    duration = endTime - startTime;
459	    frames = goodFrames;
gdb $ p skip
$3 = 65535
gdb $ p channels
$4 = 1
gdb $ p i
$5 = 12679297
gdb $ list 430
425	  nsAutoArrayPtr<AudioDataValue> buffer(new AudioDataValue[frames * channels]);
426	
427	  // Decode to the appropriate sample type.
428	#ifdef MOZ_SAMPLE_TYPE_FLOAT32
429	  int ret = opus_decode_float(mOpusState->mDecoder,
430	                              aPacket->packet, aPacket->bytes,
431	                              buffer, frames, false);
432	#else
433	  int ret = opus_decode(mOpusState->mDecoder,
434	                        aPacket->packet, aPacket->bytes,
gdb $ p frames
$7 = 960
gdb $ p channels
$8 = 1

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Timothy B. Terriberry (:derf)]

(In reply to Christoph Diehl [:cdiehl] from comment #0)
> 450	    PRInt32 skip = mOpusState->mPreSkip;
> 451	    PRInt32 goodFrames = frames - skip;
> 452	    NS_ASSERTION(goodFrames > 0, "endTime calculation was wrong");

Ralph, I really don't understand this code. frames is the number of samples in the current packet, which is generally small: at most 5760 (120 ms worth). skip can be as large as 65535 (1.37 seconds worth). You have an "if (endTime < 0) return NS_OK;" check to see if you need to skip an entire packet... but you aren't keeping track of the amount of audio that was skipped from that packet. Instead you try to subtract the entire skip amount from the first packet that has a granpos at least as large as the skip amount, even though that individual packet may have nowhere near enough audio in it.

Also, while we're here, if frames==granulepos and frames==skip, you won't take the early return to skip an entire packet, but you will trigger the "endTime calculation was wrong" assertion.

Comment 3

[Ralph Giles (:rillian)]

This testcast is handled without error with the patch from bug 758833.

Comment 4

[Ralph Giles (:rillian)]

Specifically, this check in nsOpusCodec::ReconstructOpusGranulepos() which blocks the testcase:

if (!mDoneReadingHeaders && GetOpusDeltaGP(mUnstamped[0]) > gp)
  return false;

Comment 5

[Ralph Giles (:rillian)]

Sorry, comment #4 was meant for bug 750327.

Comment 6

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

Christoph, can you verify that this issue is now fixed?

Comment 7

[Christoph Diehl [:posidron]]

This bug is already marked as resolved/fixed. :)

Comment 8

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

(In reply to Christoph Diehl [:cdiehl] from comment #7)
> This bug is already marked as resolved/fixed. :)

Yes, it is. That means code was checked in to fix the problem. We have a "verified" status to be used when we confirm that the fix addresses the problem by checking. I'm asking you to verify that it is actually fixed on Trunk.

Comment 9

[Christoph Diehl [:posidron]]

Have tested it based comment 3 by myself again - bug is fixed.