485642 - Crash in nsAudioStream::Write while expanding mBufferOverflow

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

On a trunk x86_64 build, I'm seeing crashes with nsAudioStream::Write on the stack when playing video.

Comment 1

[Matthew Gregan [:kinetik]]

snd_pcm_avail_update in sa_stream_get_write_size is returning an error (-32, EPIPE, which means we've either overrun or underrun (EPIPE can mean either, depending on the ALSA function called, and for snd_pcm_avail_update it's not explicitly documented)).  sa_stream_get_write_size doesn't check for error and blindly attempts to convert the result from frames to bytes, resulting in a huge unsigned value.

nsAudioStream::Available divides this by two, then returns it as a signed value.  nsAudioStream::Write uses this to extend mBufferOverflow with AppendElements:

with available = -32:
      mBufferOverflow.AppendElements(s_data.get() + available, (count - available));

...which is obviously bad.

Comment 2

[cajbir (:cajbir)]

I think this is fixed in the latest libsydneyaudio changes I made? See patch in bug 485433.

Basically the code changed to use snd_pcm_status. The result of this is checked and an error returned. If it succeeds, snd_pcm_status_get_avail is used to get the available bytes from the returned status structure:

if ((r = snd_pcm_status(s->output_unit, status)) < 0) {
  *size = 0;
  return SA_ERROR_SYSTEM;
}

avail = snd_pcm_status_get_avail(status);
*size = snd_pcm_frames_to_bytes(s->output_unit, avail);

Comment 3

[Matthew Gregan [:kinetik]]

It'd fix it if snd_pcm_status returned an error.  snd_pcm_status_get_avail can return an error, and the new code doesn't check for and handle that case, so it seems like this could still be a problem.

Comment 4

[cajbir (:cajbir)]

I'll do a libsydneyaudio fix and update bug 485433 with the patch.

Comment 5

[cajbir (:cajbir)]

Hmm, the docs for snd_pcm_status_get_avail don't mention it returning any error details. In fact, the return value is unsigned so it can't return a negative value. I think the result is valid if the call to snd_pcm_status succeeded.

Comment 6

[Matthew Gregan [:kinetik]]

Oops, I was looking at the wrong ALSA function when I checked the docs.  You're right.

Comment 7

[Matthew Gregan [:kinetik]]

Fixed by bug 485433 landing.