Bug 190239 – Ref<FetchResponse> use-after-move in DOMCache::put()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Ref<FetchResponse> use-after-move in DOMCache::put()

[Zan Dobersek]

Created attachment 351492 [details]
Patch

[Michael Catanzaro]

Comment on attachment 351492 [details]
Patch

What a footgun :(

[youenn fablet]

Comment on attachment 351492 [details]
Patch

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

> Source/WebCore/Modules/cache/DOMCache.cpp:354
> +        responseRef.consumeBodyReceivedByChunk([promise = WTFMove(promise), request = WTFMove(request), response = WTFMove(response), data = SharedBuffer::create(), pendingActivity = makePendingActivity(*this), this](auto&& result) mutable {

Is it really a bug on compilers?
Shouldn't they all do:
1. Call -> operator on the Ref<FetchResponse>
2. Call consumeBodyReceivedByChunk or move the Ref<FetchResponse> in the lambda
3. Move the Ref<FetchResponse> in the lambda or call consumeBodyReceivedByChunk

[Zan Dobersek]

(In reply to youenn fablet from comment #3)
> Comment on attachment 351492 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351492&action=review
> 
> > Source/WebCore/Modules/cache/DOMCache.cpp:354
> > +        responseRef.consumeBodyReceivedByChunk([promise = WTFMove(promise), request = WTFMove(request), response = WTFMove(response), data = SharedBuffer::create(), pendingActivity = makePendingActivity(*this), this](auto&& result) mutable {
> 
> Is it really a bug on compilers?
> Shouldn't they all do:
> 1. Call -> operator on the Ref<FetchResponse>
> 2. Call consumeBodyReceivedByChunk or move the Ref<FetchResponse> in the
> lambda
> 3. Move the Ref<FetchResponse> in the lambda or call
> consumeBodyReceivedByChunk

It's a bug in our code, but compilers are free to choose the desired order of evaluating the call expression as well as each argument:
https://en.cppreference.com/w/cpp/language/operator_other#Built-in_function_call_operator

This is apparently changing in C++17, guaranteeing the function-name expression being evaluated before the arguments.

[Zan Dobersek]

Comment on attachment 351492 [details]
Patch

Clearing flags on attachment: 351492

Committed r236789: <https://trac.webkit.org/changeset/236789>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44969033>

[youenn fablet]

> This is apparently changing in C++17, guaranteeing the function-name
> expression being evaluated before the arguments.

C++17 seems tempting then!