1604497 - UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/api/video/i420_buffer.cc:225:52 in

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 83fc8cf83221.

/builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/api/video/i420_buffer.cc:225:52: runtime error: division by zero
    #0 0x7eff33eb9188 in webrtc::I420Buffer::CropAndScaleFrom(webrtc::I420BufferInterface const&) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/api/video/i420_buffer.cc:225:52
    #1 0x7eff30438816 in mozilla::MediaEngineRemoteVideoSource::DeliverFrame(unsigned char*, mozilla::camera::VideoFrameProperties const&) /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:553:19
    #2 0x7eff3043a2fc in non-virtual thunk to mozilla::MediaEngineRemoteVideoSource::DeliverFrame(unsigned char*, mozilla::camera::VideoFrameProperties const&) /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
    #3 0x7eff302cde44 in mozilla::camera::CamerasChild::RecvDeliverFrame(mozilla::camera::CaptureEngine const&, int const&, mozilla::ipc::Shmem&&, mozilla::camera::VideoFrameProperties const&) /builds/worker/workspace/build/src/dom/media/systemservices/CamerasChild.cpp:515:33
    #4 0x7eff2b723584 in mozilla::camera::PCamerasChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PCamerasChild.cpp:470:28
    #5 0x7eff2ae17dcf in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:5876:32
    #6 0x7eff2a6a2642 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2212:25
    #7 0x7eff2a69d2a4 in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2134:9
    #8 0x7eff2a69f56f in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1973:3
    #9 0x7eff2a6a0470 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2004:13
    #10 0x7eff29483f47 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1241:14
    #11 0x7eff2948e74c in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
    #12 0x7eff2a6afd54 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:332:5
    #13 0x7eff2a5a82b7 in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
    #14 0x7eff2a5a82b7 in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308:3
    #15 0x7eff2a5a82b7 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290:3
    #16 0x7eff2947d06a in nsThread::ThreadFunc(void*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:459:11
    #17 0x7eff4cde325e in _pt_root /builds/worker/workspace/build/src/nsprpub/pr/src/pthreads/ptthread.c:201:5
    #18 0x7eff4ca2b6da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #19 0x7eff4ba0988e in clone /build/glibc-OTsEL5/glibc-2.27/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:95

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/api/video/i420_buffer.cc:225:52 in 

Comment 1

[Dan Minor [:dminor]]

:jib, do you mind having a quick look at this? Looking at the testcase, it is setting a negative width, which if I'm reading the correct part of the spec, shouldn't be allowed: https://w3c.github.io/mediacapture-main/getusermedia.html#dom-mediatrackconstraintset-width.

I'll fix up the webrtc.org code to protect against division by zero, but I wasn't sure if we should also be preventing bad constraints from getting that far. Is it ok to request a zero width or zero height image through constraints?

Comment 2

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

Yeah the spec has since changed and all the numeric constraints are supposed to be unsigned now (they're not in our implementation).

As I recall, we have special code for screen sharing since capabilites are not deterministic there, so we effectively pass width/height (deal and max <<'ed together into a long-word as I recall) constraints down to the low-level code (where it's >>'ed) to figure out the right answer when first frames come in. A simple range-check in the low-level code is probably the simplest, and a good idea until get them turned into unsigned.

Comment 3

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

Hereish.

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1604497 - Avoid division by zero in I420Buffer::CropAndScaleFrom; r=ng!

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1604497 - Ensure minimum frame width and height in MediaEngineRemoteVideoSource; r=jib!

Depends on D58150

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8ac5568a8cb
Avoid division by zero in I420Buffer::CropAndScaleFrom; r=ng
https://hg.mozilla.org/integration/autoland/rev/7fa4146140f2
Ensure minimum frame width and height in MediaEngineRemoteVideoSource; r=jib

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d8ac5568a8cb
https://hg.mozilla.org/mozilla-central/rev/7fa4146140f2

Comment 8

[Ryan VanderMeulen [:RyanVM]]

AFAICT, this fix can ride the 74 train to release. Feel free to nominate for Beta approval if you feel otherwise, however.

Comment 9

[Dan Minor [:dminor]]

Upstream bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=11741
Upstream review: https://webrtc-review.googlesource.com/c/src/+/178440

Comment 10

[Dan Minor [:dminor]]

Upstream patch landed: https://webrtc.googlesource.com/src.git/+/24a53a05a52d37fbb5418958f34bda6d8ebb073d