Closed Bug 881742 Opened 8 years ago Closed 5 years ago
crash in _VEC
_memcpy | webrtc::Plane::Copy(int, int, unsigned char const*)
This bug was filed from the Socorro interface and is report bp-c0304d1f-e6af-4833-9bbb-c8c3c2130606 . ============================================================= Frame Module Signature Source 0 msvcr100.dll _VEC_memcpy 1 xul.dll webrtc::Plane::Copy media/webrtc/trunk/webrtc/common_video/plane.cc:67 2 xul.dll webrtc::I420VideoFrame::CreateFrame media/webrtc/trunk/webrtc/common_video/i420_video_frame.cc:55 3 xul.dll webrtc::VP8DecoderImpl::ReturnFrame media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:884 4 xul.dll webrtc::VP8DecoderImpl::Decode media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc:786 5 xul.dll webrtc::VCMGenericDecoder::Decode media/webrtc/trunk/webrtc/modules/video_coding/main/source/generic_decoder.cc:173 6 xul.dll webrtc::VideoCodingModuleImpl::Decode media/webrtc/trunk/webrtc/modules/video_coding/main/source/video_coding_impl.cc:1052 7 xul.dll webrtc::VideoCodingModuleImpl::Decode media/webrtc/trunk/webrtc/modules/video_coding/main/source/video_coding_impl.cc:914 8 xul.dll webrtc::ViEChannel::ChannelDecodeProcess media/webrtc/trunk/webrtc/video_engine/vie_channel.cc:2215 9 xul.dll webrtc::ThreadWindows::Run media/webrtc/trunk/webrtc/system_wrappers/source/thread_win.cc:170 10 xul.dll webrtc::ThreadWindows::StartThread media/webrtc/trunk/webrtc/system_wrappers/source/thread_win.cc:66 11 msvcr100.dll _callthreadstartex f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314 12 msvcr100.dll _threadstartex f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292 13 kernel32.dll BaseThreadInitThunk 14 ntdll.dll __RtlUserThreadStart 15 ntdll.dll _RtlUserThreadStart
Crash Reason EXCEPTION_ACCESS_VIOLATION_WRITE Crash Address 0x0 Line: memcpy(buffer_.get(), buffer, size); We crash with a write to address 0 so that means that buffer_.get() is NULL. Since we try and re-allocate to proper size before memcpy, issue is with the reallocation. Either size is too big or alignment is invalid, either way, fix is simple, just check that alocation succeeded, if not, return error. Root cause may be malformed video being decoded.
Note: would be interesting to actually find a scenario / test case as this might just delay crash to a function later on.
Ok: as mentioned, this is an upstream bug in how AlignedMalloc() is used in MaybeResize(). AlignedMalloc is fallible, and will cause it to return NULL on failure, but MaybeResize() doesn't appear to check it. Please a) file a bug upstream at webrtc.org, and pose the bug number/link here, b) if you can, propose a patch and post it here. You could propose it to the upstream as a patch (and mark it for reviews!) or do so here and we can propose it upstream later. Also verify that the current trunk of webrtc.org hasn't fixed it (I suspect it hasn't been fixed). Thanks for digging through the backlog!
Request review, please add any other persons that need to review this.
Attachment #830702 - Flags: review?(rjesup)
Comment on attachment 830702 [details] [diff] [review] nullwritefix.patch Review of attachment 830702 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/trunk/webrtc/common_video/plane.cc @@ +47,5 @@ > AlignedMalloc<uint8_t>(new_size, kBufferAlignment)); > + > + if (NULL == new_buffer.get()) > + return -1; > + remove the trailing space. Also, match the norm for the code, which would be: if (!new_buffer.get())
Attachment #830702 - Flags: review?(rjesup) → review+
Updated patch after review
Note: - Checked upstream and issue is not fixed ( http://code.google.com/p/webrtc/source/browse/trunk/webrtc/common_video/plane.cc ) Will raise the bug there with patch in a couple of hours.
Attachment #830737 - Flags: review?(rjesup) → review+
Adrian: the process now is to mark it "checkin-needed"; someone should pick it up and merge it to the tree in relatively short order. Thanks!
Thanks for the review and guidance, don't have permission to add "checkin-needed" yet - if anyone has rights, feel free to add it
Still a bug in upstream. Hadn't noticed this hadn't landed yet, will land momentarily. Note that it's rare-to-impossible (*maybe* due to insanely-large-video decoded) unless you're on the edge of OOM already.
backlog: --- → webRTC+
Priority: -- → P4
Can't see any instances of this crash in Socorro  for the past 4 weeks, in builds after this fix landed.  - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=_VEC_memcpy+%7C+webrtc%3A%3APlane%3A%3ACopy%28int%2C+int%2C+unsigned+char+const%2A%29#tab-sigsummary
You need to log in before you can comment on or make changes to this bug.