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

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P4)

Description

[Jason Smith [:jsmith]]

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

Comment 1

[Adrian Cruceru]

Attachment: nullwrite.patch

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.

Comment 2

[Adrian Cruceru]

Note: would be interesting to actually find a scenario / test case as this might just delay crash to a function later on.

Comment 3

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

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!

Comment 4

[Adrian Cruceru]

Attachment: nullwritefix.patch

Request review, please add any other persons that need to review this.

Comment 5

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

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())

Comment 6

[Adrian Cruceru]

Attachment: nullwritefix_review.patch

Updated patch after review

Comment 7

[Adrian Cruceru]

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.

Comment 8

[Adrian Cruceru]

WebRTC bug: http://code.google.com/p/webrtc/issues/detail?id=2617&thanks=2617&ts=1384261210

Comment 9

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

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!

Comment 10

[Adrian Cruceru]

Thanks for the review and guidance, don't have permission to add "checkin-needed" yet - if anyone has rights, feel free to add it

Comment 11

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

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.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b670aeb3be8

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b670aeb3be8

Comment 14

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Can't see any instances of this crash in Socorro [1] for the past 4 weeks, in builds after this fix landed.

[1] - 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