Closed Bug 1426171 Opened 3 years ago Closed 3 years ago

Potential crash if GraphRate is greater than 48kHz in WebrtcAudioConduit::GetAudioFrame

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Keywords: regression)

Crash Data

Attachments

(1 file)

similar to bug 1423770.
https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#2236

Following bug 1397793, we pull from the Audioconduit data at the same rate as the MSG rate.
This assumes that the MSG rate is always less or equal to 48kHz.

Unlike 1423770 however, this is no longer a security issue following bug 1404997 which added strong assertions that the audio buffer was always sufficient in size.
Rank: 25
Priority: -- → P2
Comment on attachment 8937963 [details]
Bug 1426171 - Only use the graph's rate if supported by the AudioConduit.

https://reviewboard.mozilla.org/r/208686/#review214524

Looks good.

If you could add a test that would be terrific. Unfortunately we're not really in a good place wrt tests here. There's [1] and [2] but I think you'll get into some trouble trying to stand up enough to test this though. Feel free to try.

[1] https://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/mediapipeline_unittest.cpp
[2] https://searchfox.org/mozilla-central/source/media/webrtc/signaling/gtest/mediaconduit_unittests.cpp

::: commit-message-e0b8f:4
(Diff revision 1)
> +Bug 1426171 - Only use the graph's rate if supported by the AudioConduit. r?pehrsons
> +
> +Otherwise we will use 48kHz as default, the MSG will resample as needed.
> +It would be possible to allow all frequencies in the AudioConduit has the webrtc backend supports it all really, however it would require more changes and likely heap allocation that we're trying to limit in this part of the code.

The "has" needs to be looked over in "to allow all frequencies in the AudioConduit has the webrtc backend supports it".

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:2179
(Diff revision 1)
> +    , mRate(mSource ? (static_cast<AudioSessionConduit*>(mConduit.get())
> +                           ->IsSamplingFreqSupported(mSource->GraphRate())
> +                         ? mSource->GraphRate()
> +                         : WEBRTC_MAX_SAMPLE_RATE)
> +                    : WEBRTC_MAX_SAMPLE_RATE)

It's an invariant that mSource is non-null so no need to cater for that case.
Attachment #8937963 - Flags: review?(apehrson) → review+
Comment on attachment 8937963 [details]
Bug 1426171 - Only use the graph's rate if supported by the AudioConduit.

https://reviewboard.mozilla.org/r/208686/#review214524

i don't see how.. will open a followup bug then.

> It's an invariant that mSource is non-null so no need to cater for that case.

as it is, it's not yet...
it may be called that way, but nothing in the code states so, we even have tests later that mSource isn't null, it will abort otherwise.
See Also: → 1426486
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/179ac22f4d3a
Only use the graph's rate if supported by the AudioConduit. r=pehrsons
Duplicate of this bug: 1426511
Crash Signature: [@ mozilla::MediaPipelineReceiveAudio::PipelineListener::NotifyPullImpl]
https://hg.mozilla.org/mozilla-central/rev/179ac22f4d3a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8937963 [details]
Bug 1426171 - Only use the graph's rate if supported by the AudioConduit.

https://reviewboard.mozilla.org/r/208686/#review214524

> as it is, it's not yet...
> it may be called that way, but nothing in the code states so, we even have tests later that mSource isn't null, it will abort otherwise.

In this very constructor you have `MOZ_ASSERT(mSource);`
You need to log in before you can comment on or make changes to this bug.