Bug 209149 – Crash under WebCookieCache::clearForHost()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Crash under WebCookieCache::clearForHost():
Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0xffffffffffffffff)
[  0] 0x00007fff45ec6c2a WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::StringImpl::ref() at StringImpl.h:1098:16

     0x00007fff45ec6c1f:     movq %rax, %r15
     0x00007fff45ec6c22:     movq (%r14), %rbx
     0x00007fff45ec6c25:    testq %rbx, %rbx
     0x00007fff45ec6c28:       je 0x525c2d             ; <+143> [inlined] WTF::VectorBufferBase<WTF::String, WTF::FastMalloc>::VectorBufferBase() at Vector.h:383
 ->  0x00007fff45ec6c2a:     addl $0x2, (%rbx)
     0x00007fff45ec6c2d:    xorps %xmm0, %xmm0
     0x00007fff45ec6c30:   movaps %xmm0, -0x50(%rbp)
     0x00007fff45ec6c34:     movl $0x1, -0x48(%rbp)
     0x00007fff45ec6c3b:     movl $0x8, %edi

[  0] 0x00007fff45ec6c2a WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] void WTF::refIfNotNull<WTF::StringImpl>(WTF::StringImpl*) + 5 at RefPtr.h:38
[  0] 0x00007fff45ec6c25 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::RefPtr(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > const&) + 3 at RefPtr.h:59
[  0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::RefPtr(WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > const&) at RefPtr.h:59
[  0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::String::String(WTF::String const&) at WTFString.h:132
[  0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) [inlined] WTF::String::String(WTF::String const&) at WTFString.h:132
[  0] 0x00007fff45ec6c22 WebKit`WebKit::WebCookieCache::clearForHost(WTF::String const&) + 132 at WebCookieCache.cpp:101
[  1] 0x00007fff45ec7614 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const [inlined] WebKit::WebCookieCache::pruneCacheIfNecessary() + 63 at WebCookieCache.cpp:113:9
[  1] 0x00007fff45ec75d5 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const [inlined] WebKit::WebCookieCache::cookiesForDOM(WTF::URL const&, WebCore::SameSiteInfo const&, WTF::URL const&, WTF::ObjectIdentifier<WebCore::FrameIdentifierType>, WTF::ObjectIdentifier<WebCore::PageIdentifierType>, WebCore::IncludeSecureCookies) + 626 at WebCookieCache.cpp:55
[  1] 0x00007fff45ec7363 WebKit`WebKit::WebCookieJar::cookies(WebCore::Document&, WTF::URL const&) const + 1133 at WebCookieJar.cpp:127
[  2] 0x00007fff447799d6 WebCore`WebCore::Document::cookie() + 438 at Document.cpp:4997:49

[Chris Dumez]

<rdar://problem/60453086>

[Chris Dumez]

Created attachment 393681 [details]
Patch

[Alex Christensen]

Comment on attachment 393681 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/CookiePrivateBrowsing.mm:138
> +    for (int i = 0; i < 100; i++) {

unsigned

[Chris Dumez]

Created attachment 393684 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 393684 [details]
Patch

Clearing flags on attachment: 393684

Committed r258521: <https://trac.webkit.org/changeset/258521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 393684 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123
> +        String hostToRemove = *m_hostsWithInMemoryStorage.random();
> +        clearForHost(hostToRemove);

To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes.

One kind of fix is to change the function argument to String rather than const String&.

I am interested in our programming discipline to avoid such things, going beyond any one bug fix.

[Chris Dumez]

Comment on attachment 393684 [details]
Patch

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

>> Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123
>> +        clearForHost(hostToRemove);
> 
> To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes.
> 
> One kind of fix is to change the function argument to String rather than const String&.
> 
> I am interested in our programming discipline to avoid such things, going beyond any one bug fix.

I considered several fixes but this is the one I liked best in the end. You are right that I could have simply passed in a 'String' instead of a 'const String&' but this seems to go against our usual coding practices where we ask developers to pass by reference to const as much as possible to avoid refcounting churn. I chose not to make the fix in clearForHost() because clearForHost() has no way to know that the host it is being provided is part of the HashSet it is about to modify. As a matter of fact, in the common case (not the pruning case) the String does not come from the HashSet.

I see your point though that it would be safer for clearForHost() to not expect its parameters to be safe to reuse after they make any state changes. Either way, you could imagine cases where it is not super obvious. For e.g.:
1. I get a string from set of Object A and call a method on Object B passing the string as parameter
2. Object B does some modification of the set of object A (via a method call for e.g.)
-> The String is no longer valid, even though Object A did not modify its own state.

Passing a String instead of a const String& makes it less error-prone for sure but then we do ref-counting churn that will not be necessary in a lot of cases. In the past, we have tried avoiding that.

[Darin Adler]

Comment on attachment 393684 [details]
Patch

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

>>> Source/WebKit/WebProcess/WebPage/WebCookieCache.cpp:123
>>> +        clearForHost(hostToRemove);
>> 
>> To me this seems like a workaround. The fix would be in the clearForHost function. Generally speaking functions that take references or pointers or const references — including const String& — have no right to assume the reference is safe to re-use after they make state changes.
>> 
>> One kind of fix is to change the function argument to String rather than const String&.
>> 
>> I am interested in our programming discipline to avoid such things, going beyond any one bug fix.
> 
> I considered several fixes but this is the one I liked best in the end. You are right that I could have simply passed in a 'String' instead of a 'const String&' but this seems to go against our usual coding practices where we ask developers to pass by reference to const as much as possible to avoid refcounting churn. I chose not to make the fix in clearForHost() because clearForHost() has no way to know that the host it is being provided is part of the HashSet it is about to modify. As a matter of fact, in the common case (not the pruning case) the String does not come from the HashSet.
> 
> I see your point though that it would be safer for clearForHost() to not expect its parameters to be safe to reuse after they make any state changes. Either way, you could imagine cases where it is not super obvious. For e.g.:
> 1. I get a string from set of Object A and call a method on Object B passing the string as parameter
> 2. Object B does some modification of the set of object A (via a method call for e.g.)
> -> The String is no longer valid, even though Object A did not modify its own state.
> 
> Passing a String instead of a const String& makes it less error-prone for sure but then we do ref-counting churn that will not be necessary in a lot of cases. In the past, we have tried avoiding that.

Seems great for fixing this one bug. Not sure it’s scalable.

To be clear, when I said "make state changes" I did not mean an object modifying its own state, I meant generally doing any nontrivial operation.

[Chris Dumez]

Reopening for alternative fix.

[Chris Dumez]

Created attachment 393698 [details]
Alternative fix

[Chris Dumez]

Comment on attachment 393698 [details]
Alternative fix

Do you like this version of the fix better?

[Darin Adler]

Comment on attachment 393698 [details]
Alternative fix

I do like that better

[Alex Christensen]

If we use link time optimization and use String everywhere we currently use const String&, would it remove all the unnecessary ref/deref pairs?

[WebKit Commit Bot]

Comment on attachment 393698 [details]
Alternative fix

Clearing flags on attachment: 393698

Committed r258530: <https://trac.webkit.org/changeset/258530>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.