190239 2018-10-03 01:22:38 -0700 Ref<FetchResponse> use-after-move in DOMCache::put() 2018-10-03 03:55:07 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 zan zan fred.wang mcatanzaro webkit-bug-importer youennf oldest_to_newest 1465958 0 zan 2018-10-03 01:22:38 -0700 Ref<FetchResponse> use-after-move in DOMCache::put() 1465961 1 351492 zan 2018-10-03 01:29:04 -0700 Created attachment 351492 Patch 1465974 2 351492 mcatanzaro 2018-10-03 02:16:14 -0700 Comment on attachment 351492 Patch What a footgun :( 1465976 3 351492 youennf 2018-10-03 02:43:47 -0700 Comment on attachment 351492 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 1465989 4 zan 2018-10-03 03:08:18 -0700 (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. 1465991 5 351492 zan 2018-10-03 03:09:31 -0700 Comment on attachment 351492 Patch Clearing flags on attachment: 351492 Committed r236789: <https://trac.webkit.org/changeset/236789> 1465992 6 zan 2018-10-03 03:09:35 -0700 All reviewed patches have been landed. Closing bug. 1465994 7 webkit-bug-importer 2018-10-03 03:11:00 -0700 <rdar://problem/44969033> 1466003 8 youennf 2018-10-03 03:55:07 -0700 > This is apparently changing in C++17, guaranteeing the function-name > expression being evaluated before the arguments. C++17 seems tempting then! 351492 2018-10-03 01:29:04 -0700 2018-10-03 03:09:31 -0700 Patch bug-190239-20181003102903.patch text/plain 2256 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMjM2Nzg3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTc3ZmQ5OWU5OTgzM2Zh OTI2MmMyMjFmYzY3MjdjZTM5NGRiZmEwMC4uNmYzMmE3ZWYxMmJiYzRhOGZiZGJhMmVlZDIyZWNi NTZiM2YzOGQwNSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDE4LTEwLTAzICBaYW4g RG9iZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKworICAgICAgICBSZWY8RmV0Y2hSZXNw b25zZT4gdXNlLWFmdGVyLW1vdmUgaW4gRE9NQ2FjaGU6OnB1dCgpCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTAyMzkKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZXRyaWV2ZSByZWZlcmVuY2UgZnJvbSB0 aGUgUmVmPEZldGNoUmVzcG9uc2U+IG9iamVjdCBiZWZvcmUgaXQncworICAgICAgICBtb3ZlLWNh cHR1cmVkIGluIHRoZSBsYW1iZGEgdGhhdCdzIHBhc3NlZCB0byB0aGUKKyAgICAgICAgRmV0Y2hS ZXNwb25zZTo6Y29uc3VtZUJvZHlSZWNlaXZlZEJ5Q2h1bmsoKSBtZXRob2QgdGhhdCBpcyBpbnZv a2VkIG9uCisgICAgICAgIHRoYXQgdmVyeSBzYW1lIG9iamVjdC4gVGhpcyBpcyBhIGNsYXNzaWMg dXNlLWFmdGVyLW1vdmUgYnVnIHRoYXQgcG9wcworICAgICAgICB1cCBvbiBjb21waWxlcnMgd2l0 aCBkaWZmZXJlbnQgQysrIGNhbGxpbmcgY29udmVudGlvbi4KKworICAgICAgICAqIE1vZHVsZXMv Y2FjaGUvRE9NQ2FjaGUuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RE9NQ2FjaGU6OnB1dCk6CisK IDIwMTgtMTAtMDIgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAg Q29weWluZyBjb250ZW50IHdpdGggc2hhZG93IERPTSBkb2Vzbid0IGNvcHkgYW55IGNvbnRlbnRz CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL2NhY2hlL0RPTUNhY2hlLmNwcCBi L1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvY2FjaGUvRE9NQ2FjaGUuY3BwCmluZGV4IDM4ZDVhNjYw OGJkNWY0ZDQ4ODg4MTI2ZjliNThmMjAyZDk3MDIxM2UuLmY3OTJmNWExMDEwNDZlZjExMDAwYTcx MWI2MTQxOTEzMWMyODVmNDMgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvY2Fj aGUvRE9NQ2FjaGUuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvY2FjaGUvRE9NQ2Fj aGUuY3BwCkBAIC0zNTAsNyArMzUwLDggQEAgdm9pZCBET01DYWNoZTo6cHV0KFJlcXVlc3RJbmZv JiYgaW5mbywgUmVmPEZldGNoUmVzcG9uc2U+JiYgcmVzcG9uc2UsIERPTVByb21pc2UKICAgICAg ICAgcmVzcG9uc2UtPnJlYWRhYmxlU3RyZWFtKCpzY3JpcHRFeGVjdXRpb25Db250ZXh0KCktPmV4 ZWNTdGF0ZSgpKTsKIAogICAgIGlmIChyZXNwb25zZS0+aXNCb2R5UmVjZWl2ZWRCeUNodW5rKCkp IHsKLSAgICAgICAgcmVzcG9uc2UtPmNvbnN1bWVCb2R5UmVjZWl2ZWRCeUNodW5rKFtwcm9taXNl ID0gV1RGTW92ZShwcm9taXNlKSwgcmVxdWVzdCA9IFdURk1vdmUocmVxdWVzdCksIHJlc3BvbnNl ID0gV1RGTW92ZShyZXNwb25zZSksIGRhdGEgPSBTaGFyZWRCdWZmZXI6OmNyZWF0ZSgpLCBwZW5k aW5nQWN0aXZpdHkgPSBtYWtlUGVuZGluZ0FjdGl2aXR5KCp0aGlzKSwgdGhpc10oYXV0byYmIHJl c3VsdCkgbXV0YWJsZSB7CisgICAgICAgIGF1dG8mIHJlc3BvbnNlUmVmID0gcmVzcG9uc2UuZ2V0 KCk7CisgICAgICAgIHJlc3BvbnNlUmVmLmNvbnN1bWVCb2R5UmVjZWl2ZWRCeUNodW5rKFtwcm9t aXNlID0gV1RGTW92ZShwcm9taXNlKSwgcmVxdWVzdCA9IFdURk1vdmUocmVxdWVzdCksIHJlc3Bv bnNlID0gV1RGTW92ZShyZXNwb25zZSksIGRhdGEgPSBTaGFyZWRCdWZmZXI6OmNyZWF0ZSgpLCBw ZW5kaW5nQWN0aXZpdHkgPSBtYWtlUGVuZGluZ0FjdGl2aXR5KCp0aGlzKSwgdGhpc10oYXV0byYm IHJlc3VsdCkgbXV0YWJsZSB7CiAKICAgICAgICAgICAgIGlmIChyZXN1bHQuaGFzRXhjZXB0aW9u KCkpIHsKICAgICAgICAgICAgICAgICB0aGlzLT5wdXRXaXRoUmVzcG9uc2VEYXRhKFdURk1vdmUo cHJvbWlzZSksIFdURk1vdmUocmVxdWVzdCksIFdURk1vdmUocmVzcG9uc2UpLCByZXN1bHQucmVs ZWFzZUV4Y2VwdGlvbigpLmlzb2xhdGVkQ29weSgpKTsK