Closed
Bug 1087368
Opened 8 years ago
Closed 8 years ago
segfault in AudioDeviceLinuxPulse::ReadRecordedData on WebRTC pages
Categories
(Core :: WebRTC, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: giuseppe.bilotta, Assigned: giuseppe.bilotta)
Details
Attachments
(1 file, 1 obsolete file)
1.64 KB,
patch
|
cbook
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Oh, I forgot to mention: this bug affects both Firefox 32 and the current trunk, and probably all versions inbetween.
Assignee | ||
Updated•8 years ago
|
Component: General → Video/Audio: Recording
Product: Firefox → Core
Version: unspecified → Trunk
Comment 2•8 years ago
|
||
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 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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?
Comment 5•8 years ago
|
||
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•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/60deb05c4c31
Assignee: nobody → giuseppe.bilotta
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 7•8 years ago
|
||
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.
Description
•