Closed Bug 881742 Opened 6 years ago Closed 4 years ago

crash in _VEC_memcpy | webrtc::Plane::Copy(int, int, unsigned char const*)

Categories

(Core :: WebRTC, defect, P4, critical)

22 Branch
x86
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox22 --- affected
firefox41 --- verified
Blocking Flags:

People

(Reporter: jsmith, Assigned: cruceru.adrian)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

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
OS: Windows NT → Windows 7
Version: Trunk → 22 Branch
Whiteboard: [WebRTC],[blocking-webrtc-]
Attached patch nullwrite.patchSplinter Review
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.
Flags: needinfo?
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!
Flags: needinfo?
Attached patch nullwritefix.patch (obsolete) — Splinter Review
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
Attachment #830702 - Attachment is obsolete: true
Attachment #830737 - Flags: review?(rjesup)
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+
Rank: 45
Priority: -- → P4
Whiteboard: [WebRTC],[blocking-webrtc-]
https://hg.mozilla.org/mozilla-central/rev/4b670aeb3be8
Assignee: nobody → cruceru.adrian
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.