1087368 - segfault in AudioDeviceLinuxPulse::ReadRecordedData on WebRTC pages

Status: RESOLVED FIXED

(Core :: WebRTC, defect, major)

Description

[Giuseppe Bilotta]

Attachment: Patch to handle holes in PulseAudio buffer in the stream reading callback

Visit any WebRTC-enabled page when using the PulseAudio backend. For example, go to http://appear.in/firefox and enable webcam/microphone sharing.

Firefox crashes more or less instantly, with a segfault in the memcpy() at the end of AudioDeviceLinuxPulse::ReadRecordedData().

This is caused by the fact that the passed buffer size may be non-zero even when the buffer pointer is NULL. This happens if the PA buffer has “holes”. A patch to fix this issue is attached: following the PA documentation, we do a stream drop in case of holes.

Comment 1

[Giuseppe Bilotta]

Oh, I forgot to mention: this bug affects both Firefox 32 and the current trunk, and probably all versions inbetween.

Comment 2

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

It's not related to recording module, so I put this bug to webrtc.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8509518 [details] [diff] [review]
Patch to handle holes in PulseAudio buffer in the stream reading callback

Review of attachment 8509518 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  please update the patch (and add r=jesup to the patch description) and mark checkin-needed

::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc
@@ +2473,4 @@
>          return;
>      }
>  
> +    // If there _tempSampleDataSize is non-zero and _tempSampleData is NULL,

remove the word 'there'.  Consider commenting how this could occur so someone reading this will understand the cause.  (Comment changes and nit-fixes like indents, etc don't need re-review).

@@ +2475,5 @@
>  
> +    // If there _tempSampleDataSize is non-zero and _tempSampleData is NULL,
> +    // there's a hole that needs to be skipped
> +    if (_tempSampleDataSize && !_tempSampleData) {
> +	    LATE(pa_stream_drop)(_recStream);

wrong indent

Comment 4

[Giuseppe Bilotta]

Attachment: Patch to handle holes in PulseAudio recording streams, v2

Thanks for the review. Fixed the indent (sorry for the noise).

I expanded the comment a bit, but honestly there isn't any solid documentation on the reasons why holes may happen, just that they can (there's some vague overrun hypothesis which is not even remotely confirmed by the experience of anyone actually experiencing these holes). I've kept it vague, hope it's fine just the same.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8385b7e5a3
Bug number is wrong in the checkin comment; has an extra 9 in it (oops)
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/2cf3e0953608
Reland: https://hg.mozilla.org/integration/mozilla-inbound/rev/60deb05c4c31

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/60deb05c4c31

Comment 7

[Carsten Book [:Tomcat]]

Comment on attachment 8511531 [details] [diff] [review]
Patch to handle holes in PulseAudio recording streams, v2

see comment 5 for checkin