Issue metadata
Sign in to add a comment
|
webkit_tests failing on chromium.webkit/WebKit Linux Trusty ASAN |
||||||||||||||||||||
Issue descriptionwebkit_tests failing on chromium.webkit/WebKit Linux Trusty ASAN Type: build-failure Builders failed on: - WebKit Linux Trusty ASAN: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty%20ASAN Based on the ASAN report below, it looks like there's an issue with https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp?q=CSSInterpolationType.cpp&sq=package:chromium&l=305 ; it looks like the const AtomicString& is not OK... although I don't really understand why. Even more mysterious to me is why this didn't show up earlier, if it is an issue with alancutter's semi-recent CL in CSSInterpolationType. 14:08:30.562 30808 ==1==ERROR: AddressSanitizer: stack-use-after-scope on address 0x7fdae93726c0 at pc 0x0000082fa5a6 bp 0x7ffeba517250 sp 0x7ffeba517248 14:08:30.562 30808 READ of size 8 at 0x7fdae93726c0 thread T0 (content_shell) 14:08:30.562 30808 #0 0x82fa5a5 in get third_party/WebKit/Source/wtf/RefPtr.h:72:41 14:08:30.562 30808 #1 0x82fa5a5 in impl third_party/WebKit/Source/wtf/text/WTFString.h:112:0 14:08:30.562 30808 #2 0x82fa5a5 in impl third_party/WebKit/Source/wtf/text/AtomicString.h:90:0 14:08:30.562 30808 #3 0x82fa5a5 in hash third_party/WebKit/Source/wtf/text/AtomicStringHash.h:39:0 14:08:30.562 30808 #4 0x82fa5a5 in hash<WTF::AtomicString> third_party/WebKit/Source/wtf/HashTable.h:550:0 14:08:30.562 30808 #5 0x82fa5a5 in lookup<WTF::IdentityHashTranslator<WTF::AtomicStringHash>, WTF::AtomicString> third_party/WebKit/Source/wtf/HashTable.h:1035:0 14:08:30.562 30808 #6 0x82fa5a5 in lookup<WTF::IdentityHashTranslator<WTF::AtomicStringHash>, const WTF::AtomicString &> third_party/WebKit/Source/wtf/HashTable.h:1011:0 14:08:30.562 30808 #7 0x82fa5a5 in lookup third_party/WebKit/Source/wtf/HashTable.h:776:0 14:08:30.562 30808 #8 0x82fa5a5 in get third_party/WebKit/Source/wtf/HashMap.h:593:0 14:08:30.562 30808 #9 0x82fa5a5 in registration third_party/WebKit/Source/core/css/PropertyRegistry.cpp:24:0 14:08:30.562 30808 #10 0xdb6478d in getRegistration third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:166:22 14:08:30.562 30808 #11 0xdb6478d in applyCustomPropertyValue third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:307:0 14:08:30.562 30808 #12 0x809f17c in applyStack third_party/WebKit/Source/core/animation/InvalidatableInterpolation.cpp:244:28 14:08:30.562 30808 #13 0x84971fc in applyAnimatedProperties<blink::CSSPropertyPriority::ResolveVariables> third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1186:7 14:08:30.562 30808 #14 0x8493f08 in applyCustomProperties third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp:1694:5 ...[trimmed stack] 14:08:30.563 30808 #86 0x7fdaef37ff44 in __libc_start_main /build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287:0 14:08:30.563 30808 14:08:30.563 30808 Address 0x7fdae93726c0 is located in stack of thread T0 (content_shell) at offset 1728 in frame 14:08:30.563 30808 #0 0xdb6429f in applyCustomPropertyValue third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp:287:0 14:08:30.563 30808 14:08:30.563 30808 This frame has 11 object(s): 14:08:30.563 30808 [32, 328) 'ref.tmp.i.i.i68' 14:08:30.563 30808 [400, 696) 'ref.tmp.i.i.i56' 14:08:30.563 30808 [768, 1064) 'ref.tmp.i.i.i40' 14:08:30.563 30808 [1136, 1432) 'ref.tmp.i.i.i33' 14:08:30.563 30808 [1504, 1512) 'stringValue:298' 14:08:30.563 30808 [1536, 1608) 'tokenizer:299' 14:08:30.563 30808 [1648, 1656) 'ref.tmp:302' 14:08:30.563 30808 [1680, 1696) 'ref.tmp4:302' 14:08:30.563 30808 [1712, 1736) 'ref.tmp8:305' <== Memory access at offset 1728 is inside this variable 14:08:30.563 30808 [1776, 1784) 'agg.tmp' 14:08:30.563 30808 [1808, 1816) 'agg.tmp13' 14:08:30.563 30808 HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext 14:08:30.563 30808 (longjmp and C++ exceptions *are* supported) 14:08:30.563 30808 SUMMARY: AddressSanitizer: stack-use-after-scope (/b/c/b/linux_layout/src/out/Release/content_shell+0x82fa5a5) 14:08:30.563 30808 Shadow bytes around the buggy address: 14:08:30.563 30808 0x0ffbdd266480: f8 f8 f8 f8 f8 f2 f2 f2 f2 f2 f2 f2 f2 f2 f8 f8 14:08:30.563 30808 0x0ffbdd266490: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 14:08:30.563 30808 0x0ffbdd2664a0: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 14:08:30.563 30808 0x0ffbdd2664b0: f8 f8 f8 f2 f2 f2 f2 f2 f2 f2 f2 f2 00 f2 f2 f2 14:08:30.563 30808 0x0ffbdd2664c0: 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 f8 f2 14:08:30.563 30808 =>0x0ffbdd2664d0: f2 f2 f8 f8 f2 f2 f8 f8[f8]f2 f2 f2 f2 f2 00 f2 14:08:30.563 30808 0x0ffbdd2664e0: f2 f2 00 f3 f3 f3 f3 f3 00 00 00 00 00 00 00 00 14:08:30.563 30808 0x0ffbdd2664f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 14:08:30.563 30808 0x0ffbdd266500: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 14:08:30.563 30808 0x0ffbdd266510: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 14:08:30.563 30808 0x0ffbdd266520: f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 f5 14:08:30.563 30808 Shadow byte legend (one shadow byte represents 8 application bytes): 14:08:30.563 30808 Addressable: 00 14:08:30.563 30808 Partially addressable: 01 02 03 04 05 06 07 14:08:30.563 30808 Heap left redzone: fa 14:08:30.563 30808 Freed heap region: fd 14:08:30.563 30808 Stack left redzone: f1 14:08:30.563 30808 Stack mid redzone: f2 14:08:30.563 30808 Stack right redzone: f3 14:08:30.564 30808 Stack after return: f5 14:08:30.564 30808 Stack use after scope: f8 14:08:30.564 30808 Global redzone: f9 14:08:30.564 30808 Global init order: f6 14:08:30.564 30808 Poisoned by user: f7 14:08:30.564 30808 Container overflow: fc 14:08:30.564 30808 Array cookie: ac 14:08:30.564 30808 Intra object redzone: bb 14:08:30.564 30808 ASan internal: fe 14:08:30.564 30808 Left alloca redzone: ca 14:08:30.564 30808 Right alloca redzone: cb 14:08:30.564 30808 ==1==ABORTING
,
Jan 23 2017
Interesting, the CQ did not hit this one. Thank you for the report, I will work on it right now.
,
Jan 23 2017
I wonder how I missed that. Anyways, awesome! Removing sheriff label.
,
Jan 23 2017
Reproduced with
$ gn gen out/asan '--args=is_debug=false symbol_level=1 is_asan=true is_component_build=false' --check
$ ninja -C out/asan blink_tests
$ python third_party/WebKit/Tools/Scripts/run-webkit-tests -t asan animations
The bug is in the line 305, as correctly pointed out by waffles@:
const AtomicString& propertyName = getProperty().customPropertyName();
const PropertyRegistry::Registration* registration =
getRegistration(state, propertyName);
getProperty() returns InterpolationType value that is allocated on the stack as a temporary variable with the scope of one statement. Then customPropertyName returns a reference to this temp variable. Once the statement is over, InterpolationType value goes out of scope and destroyed. By the time it calls getRegistration, propertyName already points to a potentially reused stack address (and even if it's not yet reused, the object is destroyed, so the value is invalid anyway)
The fix is to either inline getProperty().customPropertyName() into getRegistration call, or store AtomicString value on the stack. In this case, inlining seems like a better option:
const PropertyRegistry::Registration* registration =
getRegistration(state, getProperty().customPropertyName());
,
Jan 23 2017
Ah, no, inlining won't work. As propertyName is used 3 times, not just one.
,
Jan 23 2017
So, I just saved PropertyHandle value in a local variable with a large enough scope and then all the discovered webkit_tests failures were fixed. The CL is sent for a review: https://codereview.chromium.org/2649903005/
,
Jan 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a6de90795ea5ee5c080e3266dceedd98f66b045e commit a6de90795ea5ee5c080e3266dceedd98f66b045e Author: krasin <krasin@chromium.org> Date: Tue Jan 24 00:25:37 2017 blink: fix use-after-scope issues in CSSInterpolationType. The problem was in the following line: const AtomicString& propertyName = getProperty().customPropertyName(); <consequent use of propertyName> getProperty() returns PropertyHandle value that is allocated on the stack as a temporary variable with the scope of one statement. Then customPropertyName() returns a reference to this temp variable. Once the statement is over, PropertyHandle value goes out of scope and destroyed. By the time propertyName is used, it already points to a potentially reused stack address. And even if it's not yet reused, the object is destroyed, so the value is invalid anyway. The fix is to save ProperyHandle value in a local variable with the large enough scope to cover all propertyName uses. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 , 683459 Review-Url: https://codereview.chromium.org/2649903005 Cr-Commit-Position: refs/heads/master@{#445559} [modify] https://crrev.com/a6de90795ea5ee5c080e3266dceedd98f66b045e/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
,
Jan 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9461af221d3d56769f21b6755a8afbef00d6d7d commit a9461af221d3d56769f21b6755a8afbef00d6d7d Author: krasin <krasin@chromium.org> Date: Tue Jan 24 17:17:58 2017 Enable use-after-scope check in ASAN configs. This is a second attempt to land this change. Previous attempts failed on some ChromeOS bots using old version of Clang, on Clang-CL Win bots and also there were a couple of webkit_tests failed due to a real use-after-scope issue. The use-after-scope issue is now fixed by https://codereview.chromium.org/2649903005/, Windows and ChromeOS are temporarily blacklisted. BUG= 681136 , 683459 ,683966, 683445 Review-Url: https://codereview.chromium.org/2654623002 Cr-Commit-Position: refs/heads/master@{#445747} [modify] https://crrev.com/a9461af221d3d56769f21b6755a8afbef00d6d7d/build/config/sanitizers/BUILD.gn
,
Jan 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/269942eb79516fad019284211999bc92d382c138 commit 269942eb79516fad019284211999bc92d382c138 Author: Alan Cutter <alancutter@chromium.org> Date: Wed Jan 25 01:52:46 2017 blink: fix use-after-scope issues in CSSInterpolationType. The problem was in the following line: const AtomicString& propertyName = getProperty().customPropertyName(); <consequent use of propertyName> getProperty() returns PropertyHandle value that is allocated on the stack as a temporary variable with the scope of one statement. Then customPropertyName() returns a reference to this temp variable. Once the statement is over, PropertyHandle value goes out of scope and destroyed. By the time propertyName is used, it already points to a potentially reused stack address. And even if it's not yet reused, the object is destroyed, so the value is invalid anyway. The fix is to save ProperyHandle value in a local variable with the large enough scope to cover all propertyName uses. The bug was found by AddressSanitizer with use-after-free check enabled. It's currently being rolled out into Chrome, and this CL is a part of a larger cleanup of existing failures. BUG= 649897 , 683459 , 683493 Review-Url: https://codereview.chromium.org/2649903005 Cr-Commit-Position: refs/heads/master@{#445559} (cherry picked from commit a6de90795ea5ee5c080e3266dceedd98f66b045e) Review-Url: https://codereview.chromium.org/2655663005 . Cr-Commit-Position: refs/branch-heads/2987@{#79} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/269942eb79516fad019284211999bc92d382c138/third_party/WebKit/Source/core/animation/CSSInterpolationType.cpp
,
Jan 26 2017
|
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by dgozman@chromium.org
, Jan 21 2017Owner: krasin@chromium.org
Status: Assigned (was: Available)