Bug 153727

Summary:

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

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

Page Loading

Assignee:

Jer Noble <jer.noble>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

beidson, cdumez, commit-queue, darin, japhet, jer.noble, webkit-bug-importer

Priority:

Keywords:

EasyFix, InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Follow up patch	none

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.