Bug 153727 – REGRESSION(r195770): Use-after-free in ResourceLoaderOptions::cachingPolicy

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Alexey Proskuryakov]

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

[Radar WebKit Bug Importer]

<rdar://problem/24429886>

[Darin Adler]

Simplest fix is to move the !deleted check before the allowsCaching() check inside the if statement.

[Darin Adler]

It’s just an && and we need to short circuit before calling allowsCaching() if deleted is true.

[Jer Noble]

Looking into this now.

[Jer Noble]

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

[Jer Noble]

Created attachment 270388 [details]
Patch

[Chris Dumez]

Comment on attachment 270388 [details]
Patch

r=me

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 270388 [details]
Patch

Clearing flags on attachment: 270388

Committed r195965: <http://trac.webkit.org/changeset/195965>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

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.

[Jer Noble]

Reopening to attach new patch.

[Jer Noble]

Created attachment 270491 [details]
Follow up patch

[Jer Noble]

(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)")

[WebKit Commit Bot]

Comment on attachment 270491 [details]
Follow up patch

Clearing flags on attachment: 270491

Committed r196367: <http://trac.webkit.org/changeset/196367>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.