1640416 - scaleResolutionDownBy to super-small resolution tab-crashes Fenix

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

+++ This bug was initially created as a clone of Bug #1640413 +++

STRs:

1. In Firefox Nightly Preview for Android, open https://blog.mozilla.org/webrtc/fiddle-week-downscale-video-peerconnection
2. Click "Result" tab and share camera.
3. Drag "Height" slider all the way to the left, i.e. to zero (it only goes to 2, but that's enough).
4. Keep dragging the slider up and down, to try to trigger a crash.

Expected result:

• tiny but live video

Actual result:

• Video freezes before it gets too small
• Crashes after a few seconds bp-6bbb559c-7b7b-4540-a381-5ad3f0200523

Suggests heap corruption. Stack points to here.

Comment 1

[Dan Minor [:dminor]]

A similar problem was fixed in Bug 1620660.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:jib, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I don't think this happens often enough to warrant S2, but feel free to disagree.

This appears to be in AndroidDataEncoder::Encode on the MediaThreadType::PLATFORM_ENCODER thread. John or Alastor, would either of you be able to take a look?

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, it looks like I misfiled this as Fenix when it is Fennec (I'm trying to remember what I was thinking here 2 weeks ago).

I think the remaining question is whether we think this is also fixed by bug 1620660, and given that it appears to be a sec bug if this changes our opinion on bug 1640413 comment 2, or if there's anything we can do to mitigate this, or what we want to do.

Comment 5

[Dan Minor [:dminor]]

The Fennec crash you filed (Bug 1640413) shows up as FennecAndroid 68.8 in the crash report, but the crash report attached here is FennecAndroid 78.0a1. Since Bug 1620660 landed in Firefox 75, I'd say there is a separate problem here. Since Fennec should be based off ESR 68, this seems like it is in fact a Fenix bug. But I'm not familiar enough with how Fenix versioning works to be certain.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Makes sense to me. Thanks for clearing that up Dan, I should have trusted my original memory instead of being confused by the Fennec label and my comment 0! I vaguely remember testing both, and that being why I filed two bugs. Changing it back to Fenix and updating comment 0!

Comment 7

[Nils Ohlmeier [:drno]]

Sec-high needs an assignee.

Comment 8

[Maire Reavy [:mreavy]]

Hey Jan-Ivar, This bug looks actionable, but hasn't gone anywhere in 3 months. Nils assigned it to you, but do you feel comfortable taking it? Hae you been able to make progress? Based on comment 3, it looks like you feel John or Alastor would be better suited to tackle this. Is that accurate? Thanks.

Comment 9

[Maire Reavy [:mreavy]]

Hey John and Alastor -- I believe Jan-Ivar would appreciate help on this bug. Can you work with him to find the best owner who can move this forward? It's a sec-high and appears to be very actionable. Thanks!

Comment 10

[Alastor Wu [:alwu]]

John is the author of AndroidDataEncoder, so I think he know that well than me.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks Maire for catching that. Assigning to John. John, let me know if you need help reproing. It still crashes for me.

Note you'll run into bug 1616729 trying to repro this, so I've copied the repro fiddle here: https://jan-ivar.github.io/dummy/scaledown.html

Comment 12

[John Lin [:jhlin][:jolin]]

I can reproduce it in latest nightly too. It looks like the software H.264 encoder will be used when the input is smaller than 64x64 and we don't handle the returned error properly. Will see what needs to be fixed.

Comment 13

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1640416 - p1: input size check before creating video encoder. r?alwu

On Android, software H.264 video encoder accepts some sizes when configuring
but will crash later after frames are sent to the encoder. Use method provided
in API 21+ to validate the input size and avoid using software encoder on
earlier versions to prevent crashes.

Comment 14

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1640416 - p3: add test cases for video encoder config size. r?alwu

Depends on D94461

Comment 15

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1640416 - p4: use assignment operator rather than Maybe::emplace(). r?alwu

calling emplace() on instance with existing value will cause assertion.
Since Error() can be called multiple times, assignment operator is the correct way.

Depends on D94462

Comment 16

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1640416 - p5: run WebrtcMediaDataEncoder and the platform encoder in different thread pools/task queues. r?alwu

Some WebrtcMediaDataEncoder methods are blocking and wait for platform encoder
operations to complete. Running them in one thread pool/task queue will lead
to dead lock.

Depends on D94463

Comment 17

[John Lin [:jhlin][:jolin]]

Attachment: Bug 1640416 - p2: sanity-check video encoder config size arguements. r?alwu

Depends on D94461

Comment 18

[John Lin [:jhlin][:jolin]]

Comment on attachment 9183211 [details]
Bug 1640416 - p1: input size check before creating video encoder. r?alwu

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's easy to crash the content process by simply requesting small size WebRTC video, but since the crash is caused by MOZ_ASSERT() (addressed in p4) I am not sure how easy it could be exploited.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: Bug 1592140
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Because MOZ_ASSERT() is no-op in non-debug builds, only Nightly will have this issue. The isolated Java codec process will crash in other builds but it's a very low security risk.
• How likely is this patch to cause regressions; how much testing does it need?: There shouldn't be regressions and gtest cases are added in p3.

Comment 19

[Daniel Veditz [:dveditz]]

The original crash report is gone and for some reason my phone won't share camera with Firefox so I can't try myself, but Jan-Ivar was worried about heap corruption which should have looked very different from a MOZ_ASSERT() crash.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 9183211 [details]
Bug 1640416 - p1: input size check before creating video encoder. r?alwu

sec-approval+

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 9183212 [details]
Bug 1640416 - p3: add test cases for video encoder config size. r?alwu

We should hold off landing the test part until questions about exploitability are answered. If it's really sec-high then a gtest is not a good idea. If the approval request in comment 18 is correct then it could be fine.

Jan-Ivar: can you still trigger this in a Release build, and if so will you do so, report the crash, and link the crash report back here? Thanks.

Comment 22

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

for some reason my phone won't share camera with Firefox so I can't try myself,

That's bug 1616729. To work around it use https://jan-ivar.github.io/dummy/scaledown.html instead.

Jan-Ivar: can you still trigger this in a Release build, and if so will you do so, report the crash, and link the crash report back here? Thanks.

Yes Fenix 80.1.3 bp-8d77875a-596a-400d-9952-093470201102 and 82.1.1 bp-bb0cd601-3a88-4103-ac3e-485ae0201102

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Hi, we're running out of time to get this landed in time to uplift to 83. What's the status here?

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 9183212 [details]
Bug 1640416 - p3: add test cases for video encoder config size. r?alwu

I was able to crash with the testcase in comment 22, bp-11036079-8ef0-456e-aade-73e090201104. I'm not too worried about what that diagnostic assert gives away, nor the Java stacks in the two Release builds Jan-Ivar tried in comment 22. It's OK to land the gtest now, too.

Comment 25

[John Lin [:jhlin][:jolin]]

Thanks for the approval. The crashes in comment 22 are the in remote Java codec process, which should be recoverable.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/11a9aa1e6305f8cd761a2b0d47d67b0a05f91072
https://hg.mozilla.org/integration/autoland/rev/03803698f66b2ba8d7f5841463f191b7765f560e
https://hg.mozilla.org/integration/autoland/rev/9e3a6392f9c3e548489603d5135f649d409ffa47
https://hg.mozilla.org/integration/autoland/rev/e4aea52866d0411fb43ab7925cd77cc39724baa2
https://hg.mozilla.org/integration/autoland/rev/9c2b7e6fc88a8cce9e8192a07d98904b35807ece
https://hg.mozilla.org/mozilla-central/rev/11a9aa1e6305
https://hg.mozilla.org/mozilla-central/rev/03803698f66b
https://hg.mozilla.org/mozilla-central/rev/9e3a6392f9c3
https://hg.mozilla.org/mozilla-central/rev/e4aea52866d0
https://hg.mozilla.org/mozilla-central/rev/9c2b7e6fc88a

Comment 27

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jhlin, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 28

[John Lin [:jhlin][:jolin]]

It only crashes the content process in debug builds and IMHO is not necessary to uplift.

Comment 29

[Andrei Bodea[:andrei]]

Hello, even tho I saw that the status was changed to wontfix I can confirm that I had no issues with the latest Nightly from 11/10 with Google Pixel 4 XL (11) I didn't encounter any crashes following the steps from the description.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.