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

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

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.

Comment 1

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1426171 - Only use the graph's rate if supported by the AudioConduit.

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.

Review commit: https://reviewboard.mozilla.org/r/208686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/208686/

Comment 2

[Andreas Pehrson [:pehrsons]]

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.

Comment 3

[Jean-Yves Avenard [:jya]]

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.

Comment 4

[Jean-Yves Avenard [:jya]]

Comment on attachment 8937963 [details]
Bug 1426171 - Only use the graph's rate if supported by the AudioConduit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208686/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/179ac22f4d3a
Only use the graph's rate if supported by the AudioConduit. r=pehrsons

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/179ac22f4d3a

Comment 8

[Andreas Pehrson [:pehrsons]]

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);`