Bug 209149 - Crash under WebCookieCache::clearForHost()
Summary: Crash under WebCookieCache::clearForHost()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-16 14:11 PDT by Chris Dumez
Modified: 2020-03-16 17:04 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.20 KB, patch)
2020-03-16 14:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.21 KB, patch)
2020-03-16 14:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Alternative fix (2.62 KB, patch)
2020-03-16 16:00 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-03-16 14:11:50 PDT
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
Comment 1 Chris Dumez 2020-03-16 14:12:06 PDT
<rdar://problem/60453086>
Comment 2 Chris Dumez 2020-03-16 14:16:01 PDT
Created attachment 393681 [details]
Patch
Comment 3 Alex Christensen 2020-03-16 14:41:30 PDT
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
Comment 4 Chris Dumez 2020-03-16 14:45:16 PDT
Created attachment 393684 [details]
Patch
Comment 5 WebKit Commit Bot 2020-03-16 15:04:07 PDT
Comment on attachment 393684 [details]
Patch

Clearing flags on attachment: 393684

Committed r258521: <https://trac.webkit.org/changeset/258521>
Comment 6 WebKit Commit Bot 2020-03-16 15:04:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2020-03-16 15:29:18 PDT
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.
Comment 8 Chris Dumez 2020-03-16 15:47:59 PDT
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.
Comment 9 Darin Adler 2020-03-16 15:51:09 PDT
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.
Comment 10 Chris Dumez 2020-03-16 15:59:50 PDT
Reopening for alternative fix.
Comment 11 Chris Dumez 2020-03-16 16:00:13 PDT
Created attachment 393698 [details]
Alternative fix
Comment 12 Chris Dumez 2020-03-16 16:00:40 PDT
Comment on attachment 393698 [details]
Alternative fix

Do you like this version of the fix better?
Comment 13 Darin Adler 2020-03-16 16:09:33 PDT
Comment on attachment 393698 [details]
Alternative fix

I do like that better
Comment 14 Alex Christensen 2020-03-16 16:14:45 PDT
If we use link time optimization and use String everywhere we currently use const String&, would it remove all the unnecessary ref/deref pairs?
Comment 15 WebKit Commit Bot 2020-03-16 17:04:02 PDT
Comment on attachment 393698 [details]
Alternative fix

Clearing flags on attachment: 393698

Committed r258530: <https://trac.webkit.org/changeset/258530>
Comment 16 WebKit Commit Bot 2020-03-16 17:04:04 PDT
All reviewed patches have been landed.  Closing bug.