Bug 146297

Summary: Address Sanitizer does not play well with memcpy in JSC::MachineThreads::tryCopyOtherThreadStack.
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aj355, commit-queue, keith_miller, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176874
Attachments:
Description Flags
Patch
none
Patch
msaboff: review+
Patch none

Description Keith Miller 2015-06-24 16:48:10 PDT
WebViews inside apps built with Address sanitizer will sometimes crash in JSC::MachineThreads::tryCopyOtherThreadStack.
Comment 1 Keith Miller 2015-06-24 17:04:58 PDT
Created attachment 255530 [details]
Patch
Comment 2 Alexey Proskuryakov 2015-06-24 22:07:51 PDT
Comment on attachment 255530 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255530&action=review

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:543
> +// Since we cannot blacklist the system memcpy we must use our own naive implementation,

There is no context here to understand that blackisting is about ASan.  Please update the comment to make it clear that this is needed to use ASan with both instrumented and non-instrumented builds.
Comment 3 Keith Miller 2015-06-25 10:30:50 PDT
Created attachment 255563 [details]
Patch

Updated comments do note that blacklisting was related to ASan.
Comment 4 Keith Miller 2015-06-25 10:50:04 PDT
rdar://problem/21330174
Comment 5 Michael Saboff 2015-06-25 11:00:00 PDT
Comment on attachment 255563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=255563&action=review

r=me with a couple of comments.

> Source/JavaScriptCore/ChangeLog:3
> +        Address Sanitizer finds a problem in JavaScriptCore.

Change the bug title to reflect the issue.

> Source/JavaScriptCore/heap/MachineStackMarker.cpp:522
> +void copyMemory(void*, const void*, size_t);

Don't think this declaration serves much purpose with the definition below.
Comment 6 Keith Miller 2015-06-25 11:31:02 PDT
Created attachment 255565 [details]
Patch

Removed the declaration for copyMemory changed the patch name to be more descriptive.
Comment 7 WebKit Commit Bot 2015-06-25 12:23:13 PDT
Comment on attachment 255565 [details]
Patch

Clearing flags on attachment: 255565

Committed r185957: <http://trac.webkit.org/changeset/185957>
Comment 8 WebKit Commit Bot 2015-06-25 12:23:17 PDT
All reviewed patches have been landed.  Closing bug.