Make bmalloc work with ASan
Created attachment 244180 [details] Patch
<rdar://problem/19401435>
Created attachment 244181 [details] Patch
Attachment 244181 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/Environment.cpp:26: Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Is there some advantage to doing the check in this manner rather than using a compile-time check as mentioned at <http://clang.llvm.org/docs/AddressSanitizer.html#has-feature-address-sanitizer>?
Comment on attachment 244181 [details] Patch r=me
(In reply to comment #6) > r=me I missed Mark Rowe's comment. Based on that, I think that it is better for isASanEnabled() to rely on the __has_feature(address_sanitizer) test instead of the _dyld_get_image_name checks.
This patch is right if bmalloc causes problems when used in a client app that is built with ASan. I.e., 1. WebKit is built without ASan (e.g. just using the shipping one in /System/Library). 2. Client app is built with ASan. There are at least two kinds of possible problems: 1. bmalloc would cause false positives. 2. bmalloc would hinder ASan's ability to find bugs in client apps. I do not know if either problem exists in practice.
(In reply to comment #8) > This patch is right if bmalloc causes problems when used in a client app > that is built with ASan. I.e., > 1. WebKit is built without ASan (e.g. just using the shipping one in > /System/Library). > 2. Client app is built with ASan. > > There are at least two kinds of possible problems: > 1. bmalloc would cause false positives. > 2. bmalloc would hinder ASan's ability to find bugs in client apps. > > I do not know if either problem exists in practice. Right -- I originally wrote a compile-time check, but then I realized that a compile-time check is not friendly to a client library or app that wants to build with ASan and then link against WebKit/JavaScriptCore that was built without it. I also plan to make bmalloc independently linkable, for clients that just want a fast malloc, and a compile-time check will be unfriendly to those clients. I don't know of any such clients that have problems right now, but I'd like to make bmalloc friendly to such clients in the future. Does that sound reasonable?
(In reply to comment #9) > (In reply to comment #8) > > This patch is right if bmalloc causes problems when used in a client app > > that is built with ASan. I.e., > > 1. WebKit is built without ASan (e.g. just using the shipping one in > > /System/Library). > > 2. Client app is built with ASan. > > > > There are at least two kinds of possible problems: > > 1. bmalloc would cause false positives. > > 2. bmalloc would hinder ASan's ability to find bugs in client apps. > > > > I do not know if either problem exists in practice. > > Right -- I originally wrote a compile-time check, but then I realized that a > compile-time check is not friendly to a client library or app that wants to > build with ASan and then link against WebKit/JavaScriptCore that was built > without it. > > I also plan to make bmalloc independently linkable, for clients that just > want a fast malloc, and a compile-time check will be unfriendly to those > clients. > > I don't know of any such clients that have problems right now, but I'd like > to make bmalloc friendly to such clients in the future. > > Does that sound reasonable?
(In reply to comment #9) > Does that sound reasonable? Oops ... accidentally committed last comment. Meant to say ... Sounds good to me.
I don't know if the theoretical problems are real, so I can't agree that the substantially more complicated runtime solution is warranted. Are there at least speculative reasons to believe that the problems are real?
> Are there at least speculative reasons to believe that the problems are real? Yes: (1) I make bmalloc a stand-alone .dylib. (2) Client [redacted] adopts bmalloc. (3) Client [redacted] adopts ASan. --> ASan doesn't work because none of [redacted]'s allocations register with ASan's runtime system. --> Client [redacted] sends me a mean email and unadopts bmalloc.
Yes, a bmalloc dylib case seems convincing. We just need to double-check that the dynamic check actually works in this case (unsure what guarantees that ASan dylib will be in dyld image list early enough).
Another option is to teach ASan about bmalloc so everything just works, although that would also rely on bmalloc exposing a stable API.
(In reply to comment #14) > Yes, a bmalloc dylib case seems convincing. We just need to double-check > that the dynamic check actually works in this case (unsure what guarantees > that ASan dylib will be in dyld image list early enough). I verified with a test program that the full list of linked libraries is available to _dyld_get_image_name() at static initialization time.
Committed r178144: <http://trac.webkit.org/changeset/178144>
Committed r178221: <http://trac.webkit.org/changeset/178221>