Bug 140194 – Make bmalloc work with ASan

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Geoffrey Garen]

Make bmalloc work with ASan

[Geoffrey Garen]

Created attachment 244180 [details]
Patch

[Geoffrey Garen]

<rdar://problem/19401435>

[Geoffrey Garen]

Created attachment 244181 [details]
Patch

[WebKit Commit Bot]

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.

[Mark Rowe (bdash)]

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>?

[Mark Lam]

Comment on attachment 244181 [details]
Patch

r=me

[Mark Lam]

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

[Alexey Proskuryakov]

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.

[Geoffrey Garen]

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

[Mark Lam]

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

[Mark Lam]

(In reply to comment #9)
> Does that sound reasonable?

Oops ... accidentally committed last comment.  Meant to say ...

Sounds good to me.

[Alexey Proskuryakov]

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?

[Geoffrey Garen]

> 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.

[Alexey Proskuryakov]

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

[David Kilzer (:ddkilzer)]

Another option is to teach ASan about bmalloc so everything just works, although that would also rely on bmalloc exposing a stable API.

[Geoffrey Garen]

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

[Geoffrey Garen]

Committed r178144: <http://trac.webkit.org/changeset/178144>

[Geoffrey Garen]

Committed r178221: <http://trac.webkit.org/changeset/178221>