184335 2018-04-05 11:11:21 -0700 Add necessary colon to CFNetwork selector 2018-04-09 16:39:36 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=184433 InRadar P2 Normal --- 1 wilander wilander bfulgham commit-queue darin webkit-bug-importer oldest_to_newest 1412110 0 wilander 2018-04-05 11:11:21 -0700 There needs to be a colon in the CFNetwork selector we're testing for if the selector takes a parameter. 1412111 1 webkit-bug-importer 2018-04-05 11:11:57 -0700 <rdar://problem/39213124> 1412113 2 337280 wilander 2018-04-05 11:17:40 -0700 Created attachment 337280 Patch 1412141 3 337280 wilander 2018-04-05 12:35:09 -0700 Comment on attachment 337280 Patch Mac-WK2 build successful. 1412142 4 wilander 2018-04-05 12:35:23 -0700 Thanks for the review, Brent! 1412149 5 337280 commit-queue 2018-04-05 13:02:24 -0700 Comment on attachment 337280 Patch Clearing flags on attachment: 337280 Committed r230311: <https://trac.webkit.org/changeset/230311> 1412150 6 commit-queue 2018-04-05 13:02:25 -0700 All reviewed patches have been landed. Closing bug. 1412736 7 337280 darin 2018-04-08 10:36:28 -0700 Comment on attachment 337280 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337280&action=review > Source/WebCore/PAL/pal/spi/cf/CFNetworkSPI.h:350 > + if ([request respondsToSelector:@selector(_setIgnoreHSTS:)]) > + [request performSelector:@selector(_setIgnoreHSTS:) withObject:[NSNumber numberWithBool:ignoreHSTS]]; This code is absolutely *not* correct. It is passing an object to a function that takes a boolean. I will provide a correct example shortly. 1412737 8 darin 2018-04-08 10:55:57 -0700 There are three problems with these HSTS functions: 1) These functions do not belong in the CFNetworkSPI.h, since they are not CFNetwork SPI, but rather convenience functions to make it easy to call the SPI. That’s not what should be in our SPI.h headers. 2) Two of the functions use the performSelector: return value incorrectly. The performSelector: method's return value only works if the return type is an object, or technically if it’s some non-object pointer type or if the return value is ignored. It is *not* correct to use if the return type is BOOL. 3) One of the functions uses performSelector:withObject: to pass an argument that is not an. Passing an NSNumber when the function takes a boolean won’t work properly; part of the NSNumber pointer is likely to be interpreted as the BOOL. There are multiple correct ways to do this, none of which involve the performSelector: methods. One is to simply use the methods declared in the CFNetworkSPI.h header and be careful only to call if if the method is implemented. That’s probably the preferred one and easiest to get right. Another is to use <wtf/ObjcRuntimeExtras.h>, which contains correct alternatives to performSelector: for cases like these. A third is to use NSInvocation, which requires passing in the method signature. Here is a sketch of the the first solution, leaving the code in a style suitable for a header (although I don’t think it should be in a header, and certainly not in CFNetworkSPI.h): static bool schemeWasUpgradedDueToDynamicHSTS(NSURLRequest *request) { #if !USE(CFNETWORK_IGNORE_HSTS) UNUSED_PARAM(request); return false; #else return [request respondsToSelector:@selector(_schemeWasUpgradedDueToDynamicHSTS)] && [request _schemeWasUpgradedDueToDynamicHSTS]; #endif } static void setIgnoreHSTS(NSMutableURLRequest *request, bool ignoreHSTS) { #if !USE(CFNETWORK_IGNORE_HSTS) UNUSED_PARAM(request); #else if ([request respondsToSelector:@selector(_setIgnoreHSTS:)]) [request _setIgnoreHSTS:ignoreHSTS]; #endif } static bool ignoreHSTS(NSURLRequest *request) { #if !USE(CFNETWORK_IGNORE_HSTS) UNUSED(request); return false; #else return [request respondsToSelector:@selector(_ignoreHSTS)] && [request _ignoreHSTS]; #endif } I suggest filing a new bug and dealing with these issues. 1412738 9 darin 2018-04-08 10:58:37 -0700 The ObjcRuntimeExtras.h versions would look something like this: static bool schemeWasUpgradedDueToDynamicHSTS(NSURLRequest *request) { return [request respondsToSelector:@selector(_schemeWasUpgradedDueToDynamicHSTS)] && wtfObjcMsgSend<BOOL>(request, @selector(_schemeWasUpgradedDueToDynamicHSTS)); } static void setIgnoreHSTS(NSMutableURLRequest *request, bool ignoreHSTS) { BOOL ignoreHSTSArgument = ignoreHSTS; // Must be BOOL instead of bool. if ([request respondsToSelector:@selector(_setIgnoreHSTS:)]) wtfObjcMsgSend<void>(request, @selector(_setIgnoreHSTS:), ignoreHSTSArgument); } 1412739 10 darin 2018-04-08 11:02:24 -0700 Testing will show what does and does not work in practice. I suspect that the current argument problem in the current setIgnoreHSTS function without my suggested fix always sets the flag to true, but there is a chance it might behave differently on different architectures (ARM vs. Intel, 32-bit vs. 64-bit). The return value part might behave correctly by accident on some architectures, and incorrectly on others. Or might be harmless everywhere although incorrect; not sure. 1412906 11 wilander 2018-04-09 10:00:09 -0700 Thanks for your comments, Darin. I will look into them today, including further tests of the existing code. 1413074 12 wilander 2018-04-09 16:39:36 -0700 Follow-up work tracked in https://bugs.webkit.org/show_bug.cgi?id=184433. 337280 2018-04-05 11:17:40 -0700 2018-04-05 13:02:24 -0700 Patch bug-184335-20180405111739.patch text/plain 1512 wilander SW5kZXg6IFNvdXJjZS9XZWJDb3JlL1BBTC9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNl L1dlYkNvcmUvUEFML0NoYW5nZUxvZwkocmV2aXNpb24gMjMwMzA4KQorKysgU291cmNlL1dlYkNv cmUvUEFML0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE4LTA0 LTA1ICBKb2huIFdpbGFuZGVyICA8d2lsYW5kZXJAYXBwbGUuY29tPgorCisgICAgICAgIEFkZCBu ZWNlc3NhcnkgY29sb24gdG8gQ0ZOZXR3b3JrIHNlbGVjdG9yCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODQzMzUKKyAgICAgICAgPHJkYXI6Ly9wcm9i bGVtLzM5MjEzMTI0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogcGFsL3NwaS9jZi9DRk5ldHdvcmtTUEkuaDoKKyAgICAgICAgKHNldElnbm9yZUhT VFMpOgorCiAyMDE4LTA0LTA0ICBQZXIgQXJuZSBWb2xsYW4gIDxwdm9sbGFuQGFwcGxlLmNvbT4K IAogICAgICAgICBUaGUgbGF5b3V0IHRlc3QgZmFzdC9jYW52YXMvY2FudmFzLWJsZW5kaW5nLWds b2JhbC1hbHBoYS5odG1sIGlzIGZhaWxpbmcgd2hlbiB0aGUgV2ViQ29udGVudCBwcm9jZXNzIGRv ZXMgbm90IGhhdmUgV2luZG93U2VydmVyIGFjY2Vzcy4KSW5kZXg6IFNvdXJjZS9XZWJDb3JlL1BB TC9wYWwvc3BpL2NmL0NGTmV0d29ya1NQSS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3Jl L1BBTC9wYWwvc3BpL2NmL0NGTmV0d29ya1NQSS5oCShyZXZpc2lvbiAyMjk5OTApCisrKyBTb3Vy Y2UvV2ViQ29yZS9QQUwvcGFsL3NwaS9jZi9DRk5ldHdvcmtTUEkuaAkod29ya2luZyBjb3B5KQpA QCAtMzQ2LDggKzM0Niw4IEBAIHN0YXRpYyBib29sIHNjaGVtZVdhc1VwZ3JhZGVkRHVlVG9EeW5h bWkKIAogc3RhdGljIHZvaWQgc2V0SWdub3JlSFNUUyhOU011dGFibGVVUkxSZXF1ZXN0ICpyZXF1 ZXN0LCBib29sIGlnbm9yZUhTVFMpCiB7Ci0gICAgaWYgKFtyZXF1ZXN0IHJlc3BvbmRzVG9TZWxl Y3RvcjpAc2VsZWN0b3IoX3NldElnbm9yZUhTVFMpXSkKLSAgICAgICAgW3JlcXVlc3QgcGVyZm9y bVNlbGVjdG9yOkBzZWxlY3Rvcihfc2V0SWdub3JlSFNUUykgd2l0aE9iamVjdDpbTlNOdW1iZXIg bnVtYmVyV2l0aEJvb2w6aWdub3JlSFNUU11dOworICAgIGlmIChbcmVxdWVzdCByZXNwb25kc1Rv U2VsZWN0b3I6QHNlbGVjdG9yKF9zZXRJZ25vcmVIU1RTOildKQorICAgICAgICBbcmVxdWVzdCBw ZXJmb3JtU2VsZWN0b3I6QHNlbGVjdG9yKF9zZXRJZ25vcmVIU1RTOikgd2l0aE9iamVjdDpbTlNO dW1iZXIgbnVtYmVyV2l0aEJvb2w6aWdub3JlSFNUU11dOwogfQogCiBzdGF0aWMgYm9vbCBpZ25v cmVIU1RTKE5TVVJMUmVxdWVzdCAqcmVxdWVzdCkK