1439655 - Wild pointer read in copy_and_extend_plane

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

I found an ASAN stack in copy_and_extend_plane here:

https://public-artifacts.taskcluster.net/IGDo9g-0Stq6N4h90ae8sA/0/public/logs/live_backing.log

There are lots of oranges right now that feature this function, and we had hypothesized that we were seeing a buffer overrun, but the ASAN output indicates a wild pointer.

Here's the relevant snippet:

[task 2018-02-14T18:42:25.714Z] 18:42:25     INFO - GECKO(3137) | AddressSanitizer:DEADLYSIGNAL
[task 2018-02-14T18:42:25.715Z] 18:42:25     INFO - GECKO(3137) | =================================================================
[task 2018-02-14T18:42:25.717Z] 18:42:25    ERROR - GECKO(3137) | ==3192==ERROR: AddressSanitizer: SEGV on unknown address 0x7f56c0727a40 (pc 0x7f570fe464b0 bp 0x7f56b7910ff0 sp 0x7f56b7910798 T2358)
[task 2018-02-14T18:42:25.719Z] 18:42:25     INFO - GECKO(3137) | ==3192==The signal is caused by a READ memory access.
[task 2018-02-14T18:42:25.723Z] 18:42:25     INFO - GECKO(3137) | [3192:Socket Thread]: W/signaling [Socket Thread|MediaPipeline] MediaPipeline.cpp:1242: Dropping incoming RTCP packet; filtered out
[task 2018-02-14T18:42:25.724Z] 18:42:25     INFO - GECKO(3137) | [3192:Socket Thread]: W/signaling [Socket Thread|MediaPipeline] MediaPipeline.cpp:1242: Dropping incoming RTCP packet; filtered out
[task 2018-02-14T18:42:25.734Z] 18:42:25     INFO - GECKO(3137) | [3192:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for 0x6190000f5080
[task 2018-02-14T18:42:25.758Z] 18:42:25     INFO - GECKO(3137) | [3192:Main Thread]: W/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:1184: GetTransmitPipelinesMatching: none found for 0x619000ffaf80
[task 2018-02-14T18:42:26.091Z] 18:42:26     INFO - GECKO(3137) | [3192:Socket Thread]: W/signaling [Socket Thread|MediaPipeline] MediaPipeline.cpp:1242: Dropping incoming RTCP packet; filtered out
[task 2018-02-14T18:42:26.148Z] 18:42:26     INFO - GECKO(3137) |     #0 0x7f570fe464af  /build/glibc-Cl5G7W/glibc-2.23/string/../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:79
[task 2018-02-14T18:42:26.149Z] 18:42:26     INFO - GECKO(3137) |     #1 0x4be112 in __asan_memcpy /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.cc:23:3
[task 2018-02-14T18:42:26.327Z] 18:42:26     INFO - GECKO(3137) | [3192:Socket Thread]: W/signaling [Socket Thread|MediaPipeline] MediaPipeline.cpp:1242: Dropping incoming RTCP packet; filtered out
[task 2018-02-14T18:42:26.517Z] 18:42:26     INFO - GECKO(3137) | [3192:Socket Thread]: W/signaling [Socket Thread|MediaPipeline] MediaPipeline.cpp:1242: Dropping incoming RTCP packet; filtered out
[task 2018-02-14T18:42:26.810Z] 18:42:26     INFO - GECKO(3137) |     #2 0x7f56f8ac5194 in copy_and_extend_plane /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/common/extend.c:38:5
[task 2018-02-14T18:42:26.812Z] 18:42:26     INFO - GECKO(3137) |     #3 0x7f56f8ac5194 in vp8_copy_and_extend_frame /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/common/extend.c:73
[task 2018-02-14T18:42:26.813Z] 18:42:26     INFO - GECKO(3137) |     #4 0x7f56f8b3b8d2 in vp8_lookahead_push /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/encoder/lookahead.c:139:5
[task 2018-02-14T18:42:26.813Z] 18:42:26     INFO - GECKO(3137) |     #5 0x7f56f8b5b8d2 in vp8_receive_raw_frame /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/encoder/onyx_if.c:4818:7
[task 2018-02-14T18:42:26.814Z] 18:42:26     INFO - GECKO(3137) |     #6 0x7f56f8bacdb1 in vp8e_encode /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/vp8_cx_iface.c:871:13
[task 2018-02-14T18:42:26.814Z] 18:42:26     INFO - GECKO(3137) |     #7 0x7f56f8d98903 in vpx_codec_encode /builds/worker/workspace/build/src/media/libvpx/libvpx/vpx/src/vpx_encoder.c:209:13
[task 2018-02-14T18:42:26.819Z] 18:42:26     INFO - GECKO(3137) |     #8 0x7f56fb291a4a in webrtc::VP8EncoderImpl::Encode(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*, std::vector<webrtc::FrameType, std::allocator<webrtc::FrameType> > const*) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:828:15
[task 2018-02-14T18:42:26.830Z] 18:42:26     INFO - GECKO(3137) |     #9 0x7f56fae3df82 in webrtc::VCMGenericEncoder::Encode(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*, std::vector<webrtc::FrameType, std::allocator<webrtc::FrameType> > const&) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/modules/video_coding/generic_encoder.cc:70:30
[task 2018-02-14T18:42:26.845Z] 18:42:26     INFO - GECKO(3137) |     #10 0x7f56fae830e1 in webrtc::vcm::VideoSender::AddVideoFrame(webrtc::VideoFrame const&, webrtc::CodecSpecificInfo const*) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/modules/video_coding/video_sender.cc:350:17
[task 2018-02-14T18:42:26.854Z] 18:42:26     INFO - GECKO(3137) |     #11 0x7f56fb230dfc in webrtc::ViEEncoder::EncodeVideoFrame(webrtc::VideoFrame const&, long) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/video/vie_encoder.cc:584:19
[task 2018-02-14T18:42:26.856Z] 18:42:26     INFO - GECKO(3137) |     #12 0x7f56fb239cfc in webrtc::ViEEncoder::EncodeTask::Run() /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/video/vie_encoder.cc:105:21
[task 2018-02-14T18:42:26.859Z] 18:42:26     INFO - GECKO(3137) |     #13 0x7f56fb05d23b in rtc::TaskQueue::OnWakeup(int, short, void*) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/base/task_queue_libevent.cc:300:18
[task 2018-02-14T18:42:26.867Z] 18:42:26     INFO - GECKO(3137) |     #14 0x7f56f1325d77 in event_persist_closure /builds/worker/workspace/build/src/ipc/chromium/src/third_party/libevent/event.c:1580:9
[task 2018-02-14T18:42:26.868Z] 18:42:26     INFO - GECKO(3137) |     #15 0x7f56f1325d77 in event_process_active_single_queue /builds/worker/workspace/build/src/ipc/chromium/src/third_party/libevent/event.c:1639
[task 2018-02-14T18:42:26.870Z] 18:42:26     INFO - GECKO(3137) |     #16 0x7f56f131dc75 in event_process_active /builds/worker/workspace/build/src/ipc/chromium/src/third_party/libevent/event.c
[task 2018-02-14T18:42:26.871Z] 18:42:26     INFO - GECKO(3137) |     #17 0x7f56f131dc75 in event_base_loop /builds/worker/workspace/build/src/ipc/chromium/src/third_party/libevent/event.c:1961
[task 2018-02-14T18:42:26.872Z] 18:42:26     INFO - GECKO(3137) |     #18 0x7f56fb05cb38 in rtc::TaskQueue::ThreadMain(void*) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/base/task_queue_libevent.cc:269:5
[task 2018-02-14T18:42:26.873Z] 18:42:26     INFO - GECKO(3137) |     #19 0x7f56fb01ff69 in rtc::PlatformThread::Run() /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/base/platform_thread.cc:296:10
[task 2018-02-14T18:42:26.874Z] 18:42:26     INFO - GECKO(3137) |     #20 0x7f56fb01f8fc in rtc::PlatformThread::StartThread(void*) /builds/worker/workspace/build/src/media/webrtc/trunk/webrtc/base/platform_thread.cc:208:40
[task 2018-02-14T18:42:26.888Z] 18:42:26     INFO - GECKO(3137) |     #21 0x7f5710d666b9 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76b9)
[task 2018-02-14T18:42:26.890Z] 18:42:26     INFO - GECKO(3137) |     #22 0x7f570fdef41c in clone /build/glibc-Cl5G7W/glibc-2.23/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:109

Comment 1

[Byron Campen [:bwc]]

Looking at the code, it is very strange that we would see a wildpointer read on line 38 but not line 37. Perhaps ASAN is confused somehow?

Comment 2

[Byron Campen [:bwc]]

I see crashes on crash-stats as far back as 56, but I guess it could have been happening for longer given that we purged all but the fairly recent crash-stats data. No crashes on esr52 though.

The YV12_BUFFER_CONFIG that contains the buffer pointer, stride, and height is defined as a stack variable in vp8e_encode, which is filled in based on the vpx_image_t *img arg. It seems unlikely that something has stomped/invalidated it while copy_and_extend_plane is executing (because we would probably see other crashes besides this specific one), which likely means it was garbage to begin with.

The vpx_image_t* that was used to fill out this struct points at the first element of VP8EncoderImpl::raw_images_ (a std::vector<vpx_image_t>), which opens up the possibility of data races perhaps. It does not look like this array (or its elements) are modified in many places, so maybe this will be an easy analysis (provided one can wade through the thicket of wrapper code).

Comment 3

[Daniel Veditz [:dveditz]]

Is our copy of libvpx up-to-date with upstream? This could be something Google has found and fixed already.

Comment 4

[Byron Campen [:bwc]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Is our copy of libvpx up-to-date with upstream? This could be something
> Google has found and fixed already.

If there is a flaw, it is likely not in libvpx proper. It would either be in webrtc.org, or our usage of it, depending on who is doing the finger-pointing.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

There are a bunch of bugs on this:
bug 1430067, bug 1429038, bug 1422959, bug 1252488, bug 1263384, bug 1292976
Also bug 1437274 (see my comment there)

One hypothesis is that the input is being freed while we're in encode(); or a reconfig perhaps.

Comment 6

[Nils Ohlmeier [:drno]]

P1's need owners, so assigning to Byron for now.

I wouldn't be surprised if the libvpx code is written with undocumented assumption from how Chrome handles the encoding.

Does anyone know if Chrome uses the same setup of a dedicated thread for the libvpx encoding, plus the webrtc TaskQueue which crashes here?
Or in other words: does Chrome maybe not use a dedicated thread thread for libvpx?

Comment 7

[Andreas Pehrson [:pehrsons]]

So a (likely) theory is that this was fixed recently by bug 1436117, see [1] for crash stats. I filed and fixed that purely speculatively at the time on the assumption that we had no crashes in the wild, so I didn't consider marking it as a sec bug. That appears to have been wrong.

jesup connected the dots here and pointed out that this regressed with the webrtc.org update to v57 back in Firefox 56, [2].

ESR 52 is safe as it uses a specific allocation + copy [3] instead of the wrapper that caused this.

At worst this could encode some sensitive data and send it to a remote peer.
If that data is not an image it's questionable if enough would be left after decoding to garner some value.
If that data is a cross-origin image it could very well be interpretable on the remote side.
Allocating one in the place of the freed legitimate one would be hard but is plausible.
Allocation patterns of both these freed and other cross-origin images vary depending on the source that produced them. Some use buffer pools (less risk) while some use explicit per-frame allocations.

Dan, FYI, and are there further mitigations we can take here?


[1] https://is.gd/ZxSEen
[2] https://searchfox.org/mozilla-central/diff/450c4d90a1e6e98788b999e80466e682823281d2/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#333-344
[3] https://dxr.mozilla.org/mozilla-esr52/rev/c3e447e07077412b9cfaacb2ea91974655ed753b/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#325-328,335-337

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

IMHO I would not chemspill for this.  I would consider it (given the bake time and the simplicity of the patch) for a ride-along on any 59.0.2

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Once this has shipped and is open, we can close some of the non-sec bugs references above

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Andreas Pehrson [:pehrsons] from comment #7)
> Dan, FYI, and are there further mitigations we can take here?

Just let it ride the trains.

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Daniel Veditz [:dveditz] PTO until March 26 from comment #10)
> (In reply to Andreas Pehrson [:pehrsons] from comment #7)
> > Dan, FYI, and are there further mitigations we can take here?
> 
> Just let it ride the trains.

Ok, then I'm closing this as fixed by bug 1436117.