Bug 140631

Summary: Make ASan do bounds checks for WTF::Vector
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Web Template FrameworkAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, cmarcelo, commit-queue, darin, ggaren
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch darin: review+

Alexey Proskuryakov
Reported 2015-01-19 11:44:11 PST
We have some code to perform out of bounds checks in non-ASan builds, ASan checks would complement that. Built-in checks are better, because they detect all overruns and underruns, while ASan only detects small ones, which make Vector access a memory address in a redzone. Also, they can be made either debug only or release too, depending on performance requirements. ASan checks are better, because they check even those memory accesses that are performed via a raw pointer returned from data() or begin(). They have zero performance cost in non-ASan builds.
Attachments
proposed patch (17.27 KB, patch)
2015-01-19 11:47 PST, Alexey Proskuryakov
darin: review+
Alexey Proskuryakov
Comment 1 2015-01-19 11:46:16 PST
Alexey Proskuryakov
Comment 2 2015-01-19 11:47:16 PST
Created attachment 244913 [details] proposed patch
WebKit Commit Bot
Comment 3 2015-01-19 11:50:10 PST
Attachment 244913 [details] did not pass style-queue: ERROR: Source/WTF/wtf/Vector.h:510: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 4 2015-01-20 09:21:09 PST
Comment on attachment 244913 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=244913&action=review Do you know why the Win EWS bot is red on this patch? > Source/WTF/wtf/Vector.h:756 > + if (this == &other) // ASan will crash if we try to restrict access to the same buffer twice. > + return; It’s irritating to add this extra branch for all builds just to make ASAN work, especially since it only optimizes something that we should never be doing. Can we do better? Maybe just put this inside #if ASAN_ENABLED?
Alexey Proskuryakov
Comment 5 2015-01-20 10:00:41 PST
I don't yet understand why Windows failed to build; will need to figure it out before landing. > Maybe just put this inside #if ASAN_ENABLED? Will do.
Alexey Proskuryakov
Comment 6 2015-01-20 10:28:50 PST
Committed <http://trac.webkit.org/r178722>. Windows build was failing because of an incorrect macro value. Changed ASAN_ENABLED fallback value from false to 0 to fix this.
Note You need to log in before you can comment on or make changes to this bug.