1225352 - global-buffer-overflow past nsTArrayHeader::sEmptyHdr [@ mozilla::dom::SpeechSynthesis::Pause]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Debug:
  Assertion failure: aIndex < Length() (invalid array index), at nsTArray.h:984
  (Called by mozilla::dom::SpeechSynthesis::Pause)  

ASan:
  global-buffer-overflow past nsTArrayHeader::sEmptyHdr
  [@ mozilla::dom::SpeechSynthesis::Pause]

Release:
  Does not crash for me

Comment 1

[Jesse Ruderman]

Attachment: stack (ASan)

Comment 2

[Jesse Ruderman]

Attachment: stack (debug)

Comment 3

[Boris Zbarsky [:bzbarsky]]

So Pause() does:

199   if (mCurrentTask &&
200       mSpeechQueue.ElementAt(0)->GetState() != SpeechSynthesisUtterance::STATE_ENDED) {

Sounds like mSpeechQueue is empty?

Comment 4

[Eitan Isaacson [:eeejay]]

I was trying to look at the attachment, and the browser kept freezing. doh!

Comment 5

[Andrew McCreight [:mccr8]]

It sounds like it isn't an unbounded read, but better safe than sorry, so I'm going to mark it sec-high.

Comment 6

[Eitan Isaacson [:eeejay]]

Attachment: Check that mSpeechQueue is not empty before referencing first element.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8688703 [details] [diff] [review]
Check that mSpeechQueue is not empty before referencing first element.

oops. Looks like we do check IsEmpty in other cases in that file.

But, should we do something to the mCurrentTask?
Like, should SpeechSynthesis::Cancel() set mCurrentTask to nullptr after 
mCurrentTask->Cancel() call or something?
Though, I guess OnEnd's  MOZ_ASSERT(mCurrentTask == aTask); might fire then.
However, we're missing to clear mCurrentTask somewhere, right?

Hmm, do we end up using InitIndirectAudio() so nsSpeechTask::Cancel() never ends up triggering OnEnd() (under DispatchEndInner() ) ?

Or am I missing something here?

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 8688703 [details] [diff] [review]
> Check that mSpeechQueue is not empty before referencing first element.
> 
> oops. Looks like we do check IsEmpty in other cases in that file.
> 
> But, should we do something to the mCurrentTask?
> Like, should SpeechSynthesis::Cancel() set mCurrentTask to nullptr after 
> mCurrentTask->Cancel() call or something?

Cancel() is only a request of the engine to end the utterance. The utterance/task should still be considered active until we get a call for OnEnd.

> Though, I guess OnEnd's  MOZ_ASSERT(mCurrentTask == aTask); might fire then.
> However, we're missing to clear mCurrentTask somewhere, right?
> 

Right, it is cleared on OnEnd.

> Hmm, do we end up using InitIndirectAudio() so nsSpeechTask::Cancel() never
> ends up triggering OnEnd() (under DispatchEndInner() ) ?
> 

Indirect audio services must call task->DispatchEnd themselves after receiving a cancel request.

> Or am I missing something here?

Did I answer your question? Not sure I did..

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

So DispatchEnd may happen async and we may have cleared the array already at that point?

Comment 10

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #9)
> So DispatchEnd may happen async and we may have cleared the array already at
> that point?

We clear the array on Cancel() (if it didn't start speaking yet), but we only clear mCurrentTask in OnEnd(). So in this case, Pause() is called in between those two calls where mCurrentTask still exists but the array is cleared.

Comment 11

[Eitan Isaacson [:eeejay]]

(In reply to Olli Pettay [:smaug] from comment #9)
> So DispatchEnd may happen async and we may have cleared the array already at
> that point?

The array is cleared immediately, DispatchEnd is async and clears mCurrentTask.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8688703 [details] [diff] [review]
Check that mSpeechQueue is not empty before referencing first element.

Ok, I guess I can live with this then.

Comment 13

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/77fc4877bd06

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/77fc4877bd06

Comment 15

[Daniel Veditz [:dveditz]]

The patch for this sec-high security bug landed without sec-approval
https://wiki.mozilla.org/Security/Bug_Approval_Process

This is marked as a regression, and Boris put it as blocking 1187105. Is that the regressing bug? If so that means this bug affects shipping Firefox 42.
* Is that the correct regressor?
* Where else do we need to back-port this?

Filling out the sec-approval form would have asked these and other questions we need to manage security releases and assess risk should someone reverse-engineer an exploit from the patch. Please request retroactive approval so we can finish that process.

Comment 16

[Eitan Isaacson [:eeejay]]

This is only preffed on by default in Nightly. So I don't know  how much of a risk this is or isn't.

This patch is very easily back-ported since it is a one-liner, so it might not hurt..