Bug 153727 - REGRESSION(r195770): Use-after-free in ResourceLoaderOptions::cachingPolicy
Summary: REGRESSION(r195770): Use-after-free in ResourceLoaderOptions::cachingPolicy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jer Noble
URL:
Keywords: EasyFix, InRadar
Depends on:
Blocks:
 
Reported: 2016-01-31 11:16 PST by Alexey Proskuryakov
Modified: 2016-02-10 09:23 PST (History)
7 users (show)

See Also:

Attachments
Patch (3.42 KB, patch)
2016-02-01 09:10 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
Follow up patch (2.03 KB, patch)
2016-02-02 09:01 PST, Jer Noble 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2016-01-31 11:16:10 PST 
Application Specific Information:
================================================================
==86812==ERROR: AddressSanitizer: heap-use-after-free on address 0x61b00026ff9c at pc 0x0001084cb848 bp 0x7fff5c7b33f0 sp 0x7fff5c7b33e8
READ of size 1 at 0x61b00026ff9c thread T0
    #0 0x1084cb847 in WebCore::ResourceLoaderOptions::cachingPolicy() const (WebCore.framework/Versions/A/WebCore+0x69b847)
    #1 0x1084c99ef in WebCore::CachedResource::allowsCaching() const (WebCore.framework/Versions/A/WebCore+0x6999ef)
    #2 0x107fa1668 in WebCore::CachedResource::removeClient(WebCore::CachedResourceClient*) (WebCore.framework/Versions/A/WebCore+0x171668)
    #3 0x10801deeb in WebCore::ImageLoader::updateFromElement() (WebCore.framework/Versions/A/WebCore+0x1edeeb)
    #4 0x108d986aa in WebCore::HTMLImageElement::selectImageSource() (WebCore.framework/Versions/A/WebCore+0xf686aa)
    #5 0x10801cfa7 in WebCore::HTMLImageElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&) (WebCore.framework/Versions/A/WebCore+0x1ecfa7)
    #6 0x108a8ff8e in WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason) (WebCore.framework/Versions/A/WebCore+0xc5ff8e)
    #7 0x108a97981 in WebCore::Element::didModifyAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&) (WebCore.framework/Versions/A/WebCore+0xc67981)
    #8 0x107fa3a05 in WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute) (WebCore.framework/Versions/A/WebCore+0x173a05)
    #9 0x10940ec48 in WebCore::setJSHTMLImageElementSrc(JSC::ExecState*, JSC::JSObject*, long long, long long) (WebCore.framework/Versions/A/WebCore+0x15dec48)
    #10 0x106c13cec in JSC::JSObject::putInlineSlow(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xe58cec)
    #11 0x10604121b in llint_slow_path_put_by_id (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x28621b)
    #12 0x106d6acf0 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xfafcf0)
    #13 0x106d67c6a in vmEntryToJavaScript (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xfacc6a)
    #14 0x106b0099d in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xd4599d)
    #15 0x105e21b64 in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x66b64)
    #16 0x105e217c1 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x667c1)
    #17 0x10638aa21 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x5cfa21)
    #18 0x10638ad56 in JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x5cfd56)
    #19 0x109162fce in WebCore::JSMainThreadExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (WebCore.framework/Versions/A/WebCore+0x1332fce)
    #20 0x107fe5169 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) (WebCore.framework/Versions/A/WebCore+0x1b5169)
    #21 0x108add101 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) (WebCore.framework/Versions/A/WebCore+0xcad101)
    #22 0x108adcb11 in WebCore::EventTarget::fireEventListeners(WebCore::Event&) (WebCore.framework/Versions/A/WebCore+0xcacb11)
    #23 0x108ab9618 in WebCore::EventContext::handleLocalEvents(WebCore::Event&) const (WebCore.framework/Versions/A/WebCore+0xc89618)
    #24 0x108abb333 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&, WebCore::WindowEventContext&) (WebCore.framework/Versions/A/WebCore+0xc8b333)
    #25 0x108aba95e in WebCore::EventDispatcher::dispatchEvent(WebCore::Node*, WebCore::Event&) (WebCore.framework/Versions/A/WebCore+0xc8a95e)
    #26 0x108028ec3 in WebCore::HTMLImageLoader::dispatchLoadEvent() (WebCore.framework/Versions/A/WebCore+0x1f8ec3)
    #27 0x108028ce6 in WebCore::ImageLoader::dispatchPendingLoadEvent() (WebCore.framework/Versions/A/WebCore+0x1f8ce6)
    #28 0x108028c04 in WebCore::ImageLoader::dispatchPendingEvent(WebCore::EventSender<WebCore::ImageLoader>*) (WebCore.framework/Versions/A/WebCore+0x1f8c04)
    #29 0x107f1aaaa in WebCore::EventSender<WebCore::ImageLoader>::dispatchPendingEvents() (WebCore.framework/Versions/A/WebCore+0xeaaaa)
    #30 0x107e50c74 in WebCore::ThreadTimers::sharedTimerFiredInternal() (WebCore.framework/Versions/A/WebCore+0x20c74)
    #31 0x107e50a8f in WebCore::timerFired(__CFRunLoopTimer*, void*) (WebCore.framework/Versions/A/WebCore+0x20a8f)
    #32 0x7fff88184bc3 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xb4bc3)
    #33 0x7fff88184852 in __CFRunLoopDoTimer (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xb4852)
    #34 0x7fff88202e69 in __CFRunLoopDoTimers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x132e69)
    #35 0x7fff8813fcd0 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x6fcd0)
    #36 0x7fff8813f337 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x6f337)
    #37 0x7fff97b2c934 in RunCurrentEventLoopInMode (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x30934)
    #38 0x7fff97b2c76e in ReceiveNextEventCommon (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x3076e)
    #39 0x7fff97b2c5ae in _BlockUntilNextEventMatchingListInModeWithFilter (/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox+0x305ae)
    #40 0x7fff8a2420ed in _DPSNextEvent (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x8a0ed)
    #41 0x7fff8a60e942 in -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x456942)
    #42 0x7fff8a237fc7 in -[NSApplication run] (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x7ffc7)
    #43 0x7fff8a1ba51f in NSApplicationMain (/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit+0x251f)
    #44 0x7fff9c563f6b in _xpc_objc_main (/usr/lib/system/libxpc.dylib+0x16f6b)
    #45 0x7fff9c5656ba in xpc_main (/usr/lib/system/libxpc.dylib+0x186ba)
    #46 0x103448b1b in ?? (WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development+0x100001b1b)
    #47 0x7fff9a8325ac in start (/usr/lib/system/libdyld.dylib+0x35ac)
    #48 0x0  (<unknown module>)
 
0x61b00026ff9c is located 284 bytes inside of 1456-byte region [0x61b00026fe80,0x61b000270430)
freed by thread T0 here:
    #0 0x104b71109 in wrap_free (/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.11.xctoolchain/usr/lib/clang/7.0.0/lib/darwin/libclang_rt.asan_osx_dynamic.dylib+0x43109)
    #1 0x1070dc2b0 in bmalloc::Deallocator::deallocateSlowCase(void*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x13212b0)
    #2 0x107fd59b6 in WebCore::CachedResource::deleteIfPossible() (WebCore.framework/Versions/A/WebCore+0x1a59b6)
    #3 0x107fa165d in WebCore::CachedResource::removeClient(WebCore::CachedResourceClient*) (WebCore.framework/Versions/A/WebCore+0x17165d)
    #4 0x10801deeb in WebCore::ImageLoader::updateFromElement() (WebCore.framework/Versions/A/WebCore+0x1edeeb)
    #5 0x108d986aa in WebCore::HTMLImageElement::selectImageSource() (WebCore.framework/Versions/A/WebCore+0xf686aa)
    #6 0x10801cfa7 in WebCore::HTMLImageElement::parseAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&) (WebCore.framework/Versions/A/WebCore+0x1ecfa7)
    #7 0x108a8ff8e in WebCore::Element::attributeChanged(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&, WebCore::Element::AttributeModificationReason) (WebCore.framework/Versions/A/WebCore+0xc5ff8e)
    #8 0x108a97981 in WebCore::Element::didModifyAttribute(WebCore::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&) (WebCore.framework/Versions/A/WebCore+0xc67981)
    #9 0x107fa3a05 in WebCore::Element::setAttributeInternal(unsigned int, WebCore::QualifiedName const&, WTF::AtomicString const&, WebCore::Element::SynchronizationOfLazyAttribute) (WebCore.framework/Versions/A/WebCore+0x173a05)
    #10 0x10940ec48 in WebCore::setJSHTMLImageElementSrc(JSC::ExecState*, JSC::JSObject*, long long, long long) (WebCore.framework/Versions/A/WebCore+0x15dec48)
    #11 0x106c13cec in JSC::JSObject::putInlineSlow(JSC::ExecState*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xe58cec)
    #12 0x10604121b in llint_slow_path_put_by_id (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x28621b)
    #13 0x106d6acf0 in llint_entry (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xfafcf0)
    #14 0x106d67c6a in vmEntryToJavaScript (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xfacc6a)
    #15 0x106b0099d in JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0xd4599d)
    #16 0x105e21b64 in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x66b64)
    #17 0x105e217c1 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x667c1)
    #18 0x10638aa21 in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x5cfa21)
    #19 0x10638ad56 in JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (JavaScriptCore.framework/Versions/A/JavaScriptCore+0x5cfd56)
    #20 0x109162fce in WebCore::JSMainThreadExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (WebCore.framework/Versions/A/WebCore+0x1332fce)
    #21 0x107fe5169 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) (WebCore.framework/Versions/A/WebCore+0x1b5169)
    #22 0x108add101 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) (WebCore.framework/Versions/A/WebCore+0xcad101)
    #23 0x108adcb11 in WebCore::EventTarget::fireEventListeners(WebCore::Event&) (WebCore.framework/Versions/A/WebCore+0xcacb11)
    #24 0x108ab9618 in WebCore::EventContext::handleLocalEvents(WebCore::Event&) const (WebCore.framework/Versions/A/WebCore+0xc89618)
    #25 0x108abb333 in WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&, WebCore::WindowEventContext&) (WebC
abort() called
CRASHING TEST: /contentextensions/make-https.html
Comment 1 Radar WebKit Bug Importer 2016-01-31 11:16:25 PST 
<rdar://problem/24429886>
Comment 2 Darin Adler 2016-02-01 08:59:02 PST 
Simplest fix is to move the !deleted check before the allowsCaching() check inside the if statement.
Comment 3 Darin Adler 2016-02-01 08:59:33 PST 
It’s just an && and we need to short circuit before calling allowsCaching() if deleted is true.
Comment 4 Jer Noble 2016-02-01 09:00:19 PST 
Looking into this now.
Comment 5 Jer Noble 2016-02-01 09:02:45 PST 
(In reply to comment #3)
> It’s just an && and we need to short circuit before calling allowsCaching()
> if deleted is true.

Yep. It also looks like this is the only place where we call allowsCaching() after deleteIfPossible().
Comment 6 Jer Noble 2016-02-01 09:10:11 PST 
Created attachment 270388 [details]
Patch
Comment 7 Chris Dumez 2016-02-01 09:16:34 PST 
Comment on attachment 270388 [details]
Patch

r=me
Comment 8 Darin Adler 2016-02-01 09:50:58 PST 
Comment on attachment 270388 [details]
Patch

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

I like the patch. I would have suggested the smaller one that just reorders the delete check, but it’s good to look over all the logic carefully.

Generally I am ever so slightly worried that we have too many allowsCaching() checks.

> Source/WebCore/loader/cache/CachedResource.cpp:486
> +        // `this` object is dead here.

What’s with the peculiar quoting here? I’ve never seen someone use backquotes like this.

> Source/WebCore/loader/cache/CachedResource.cpp:499
> +    if (!m_switchingClientsToRevalidatedResource)
> +        allClientsRemoved();

I don’t understand why it’s OK to skip this step when allowsCaching is false. Nothing in the function name "allClientsRemoved" makes that clear, even if in practice the code isn’t needed.

> Source/WebCore/loader/cache/CachedResource.cpp:500
> +    destroyDecodedDataIfNeeded();

Unclear on the sequence of the code, why it’s best to do this in this order. Also unclear why we want to skip this step when allowsCaching is false.
Comment 9 WebKit Commit Bot 2016-02-01 10:15:31 PST 
Comment on attachment 270388 [details]
Patch

Clearing flags on attachment: 270388

Committed r195965: <http://trac.webkit.org/changeset/195965>
Comment 10 WebKit Commit Bot 2016-02-01 10:15:34 PST 
All reviewed patches have been landed.  Closing bug.
Comment 11 Jer Noble 2016-02-01 11:48:25 PST 
Comment on attachment 270388 [details]
Patch

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

I'll address these comments in a follow up patch.

>> Source/WebCore/loader/cache/CachedResource.cpp:486
>> +        // `this` object is dead here.
> 
> What’s with the peculiar quoting here? I’ve never seen someone use backquotes like this.

It's a GitHub / Markdown ism.  It translates as "[code]this[/code]".  "this" is a ambiguous word, so I just wanted to emphasize that I was talking about the "this" pointer, not some other English-language "this".

>> Source/WebCore/loader/cache/CachedResource.cpp:499
>> +        allClientsRemoved();
> 
> I don’t understand why it’s OK to skip this step when allowsCaching is false. Nothing in the function name "allClientsRemoved" makes that clear, even if in practice the code isn’t needed.

You're right; we should just be protecting access to the memory cache.

>> Source/WebCore/loader/cache/CachedResource.cpp:500
>> +    destroyDecodedDataIfNeeded();
> 
> Unclear on the sequence of the code, why it’s best to do this in this order. Also unclear why we want to skip this step when allowsCaching is false.

Since destroyDecodedDataIfNeeded() just starts a timer, I don't think the order matters here.
Comment 12 Jer Noble 2016-02-02 09:01:31 PST 
Reopening to attach new patch.
Comment 13 Jer Noble 2016-02-02 09:01:34 PST 
Created attachment 270491 [details]
Follow up patch
Comment 14 Jer Noble 2016-02-02 10:37:28 PST 
(iOS build error appears unrelated: "Code Sign error: The file “WebContent-iOS-no-sandbox.entitlements” couldn’t be opened because there is no such file.: (null)")
Comment 15 WebKit Commit Bot 2016-02-10 09:23:24 PST 
Comment on attachment 270491 [details]
Follow up patch

Clearing flags on attachment: 270491

Committed r196367: <http://trac.webkit.org/changeset/196367>
Comment 16 WebKit Commit Bot 2016-02-10 09:23:28 PST 
All reviewed patches have been landed.  Closing bug.