Closed Bug 1087368 Opened 5 years ago Closed 5 years ago

segfault in AudioDeviceLinuxPulse::ReadRecordedData on WebRTC pages

Categories

(Core :: WebRTC, defect, major)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: giuseppe.bilotta, Assigned: giuseppe.bilotta)

Details

Attachments

(1 file, 1 obsolete file)

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.
Oh, I forgot to mention: this bug affects both Firefox 32 and the current trunk, and probably all versions inbetween.
Component: General → Video/Audio: Recording
Product: Firefox → Core
Version: unspecified → Trunk
It's not related to recording module, so I put this bug to webrtc.
Component: Video/Audio: Recording → WebRTC
Attachment #8509518 - Flags: review?(rjesup)
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
Attachment #8509518 - Flags: review?(rjesup) → review+
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.
Attachment #8509518 - Attachment is obsolete: true
Attachment #8511531 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/60deb05c4c31
Assignee: nobody → giuseppe.bilotta
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8511531 [details] [diff] [review]
Patch to handle holes in PulseAudio recording streams, v2

see comment 5 for checkin
Attachment #8511531 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.