1436117 - (CVE-2018-5160) A WrappedI420Buffer in MediaPipeline might outlive its buffer

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

One use of a WrappedI420Buffer in MediaPipeline [1] gets passed on further into the stack, but nothing keeps the PlanarYCbCrImage that owns the actual pixel buffer alive.

The WrappedI420Buffer outliving the PlanarYCbCrImage would be bad, but I hope and think we only end up reading that data as an image in a webrtc encoder and not doing any writing.

[1] https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#345-356

Comment 1

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1436117 - Keep aImage around until the WrappedI420Buffer is released.

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

Comment 2

[Dan Minor [:dminor]]

Comment on attachment 8953919 [details]
Bug 1436117 - Keep aImage around until the WrappedI420Buffer is released.

https://reviewboard.mozilla.org/r/223070/#review229072

lgtm

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca97ef07a045
Keep aImage around until the WrappedI420Buffer is released. r=dminor

Comment 4

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/ca97ef07a045

Comment 5

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8953919 [details]
Bug 1436117 - Keep aImage around until the WrappedI420Buffer is released.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1341285
[User impact if declined]: There's a risk of the webrtc encoder accessing uninitialized memory. However we haven't seen or heard reports about this so I gather it's a very low risk.
[Is this code covered by automated tests?]: Yes, though not explicitly testing what this fix is for, as it is speculative.
[Has the fix been verified in Nightly?]: It's speculative, so no
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It may extend the lifetime of some internal images. The worst that could happen would be shutdown leaks.
[String changes made/needed]: None

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8953919 [details]
Bug 1436117 - Keep aImage around until the WrappedI420Buffer is released.

I'd like to let this ride with 60 unless you feel strongly about fixing it in 59.