202753 2019-10-09 09:32:13 -0700 [Cocoa] IPC::decode should gracefully handle a nil allowed class 2019-10-09 14:30:16 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 aestes aestes cdumez commit-queue ggaren timothy webkit-bug-importer oldest_to_newest 1578262 0 aestes 2019-10-09 09:32:13 -0700 [Cocoa] IPC::decode should gracefully handle a nil allowed class 1578264 1 380537 aestes 2019-10-09 09:42:45 -0700 Created attachment 380537 Patch 1578275 2 380537 ggaren 2019-10-09 10:01:14 -0700 Comment on attachment 380537 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380537&action=review r=me > Source/WebKit/ChangeLog:20 > + rdar://problem/55839467 was resolved by fixing the build misconfiguration, but this patch > + improves IPC::decode so that a nil allowed class results in a message decoding failure > + rather than a maybe-caught NSException. > + What do message decoding failures do at runtime? Would this error we experienced still be obvious, or would it become more subtle? 1578285 3 380537 timothy 2019-10-09 10:24:13 -0700 Comment on attachment 380537 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380537&action=review >> Source/WebKit/ChangeLog:20 >> + > > What do message decoding failures do at runtime? Would this error we experienced still be obvious, or would it become more subtle? When the allowedClasses array is empty, we assert in debug builds and always return nil for the decoded object. So it would depend on the caller to handle that. 1578304 4 aestes 2019-10-09 11:10:13 -0700 (In reply to Geoffrey Garen from comment #2) > Comment on attachment 380537 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380537&action=review > > r=me > > > Source/WebKit/ChangeLog:20 > > + rdar://problem/55839467 was resolved by fixing the build misconfiguration, but this patch > > + improves IPC::decode so that a nil allowed class results in a message decoding failure > > + rather than a maybe-caught NSException. > > + > > What do message decoding failures do at runtime? Would this error we > experienced still be obvious, or would it become more subtle? The new failure mode is that a call to sendSync() with an infinite timeout now returns false when decoding fails in the remote process (rather than hanging). For cases like rdar://problem/55839467, returning false causes PaymentCoordinator to throw a JavaScript exception telling the webpage that the payment UI could not be presented. We also have a LOG_ERROR (but not a RELEASE_LOG_ERROR) for when secure coding-style decoding fails. 1578308 5 aestes 2019-10-09 11:29:10 -0700 (In reply to Andy Estes from comment #4) > returning false causes > PaymentCoordinator to throw a JavaScript exception telling the webpage that > the payment UI could not be presented. We also have a LOG_ERROR (but not a > RELEASE_LOG_ERROR) for when secure coding-style decoding fails. ... which I think is at least as obvious a symptom as a hang, in that the Apple Pay UI does not appear in response to button activation in either case. 1578387 6 380537 commit-queue 2019-10-09 14:29:26 -0700 Comment on attachment 380537 Patch Clearing flags on attachment: 380537 Committed r250934: <https://trac.webkit.org/changeset/250934> 1578388 7 commit-queue 2019-10-09 14:29:28 -0700 All reviewed patches have been landed. Closing bug. 1578390 8 webkit-bug-importer 2019-10-09 14:30:16 -0700 <rdar://problem/56130523> 380537 2019-10-09 09:42:45 -0700 2019-10-09 14:29:26 -0700 Patch bug-202753-20191009094245.patch text/plain 2203 aestes U3VidmVyc2lvbiBSZXZpc2lvbjogMjUwOTEyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGIyOGZjYjZiYjA5MTYzZDU0 MzM0YTMyNGU2NTcxYWY1YjBjOTk1YTkuLmE2NmY0ZjE4NjhjNWVlN2YzODQxNDYzOGNjMjY3Zjk3 NTM1Njk4NTkgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjYgQEAKKzIwMTktMTAtMDkgIEFuZHkgRXN0 ZXMgIDxhZXN0ZXNAYXBwbGUuY29tPgorCisgICAgICAgIFtDb2NvYV0gSVBDOjpkZWNvZGUgc2hv dWxkIGdyYWNlZnVsbHkgaGFuZGxlIGEgbmlsIGFsbG93ZWQgY2xhc3MKKyAgICAgICAgaHR0cHM6 Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIwMjc1MworCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIElmIElQQzo6ZGVjb2RlIGlzIGNhbGxl ZCB3aXRoIGEgbmlsIGFsbG93ZWQgY2xhc3MsIGFuIE5TSW52YWxpZEFyZ3VtZW50RXhjZXB0aW9u IHdpbGwgYmUKKyAgICAgICAgdGhyb3duIHdoZW4gdHJ5aW5nIHRvIGNyZWF0ZSBhbiBOU0FycmF5 IGxpdGVyYWwgd2l0aCBhIG5pbCB2YWx1ZS4gRGVwZW5kaW5nIG9uIHdobyBjYWxscworICAgICAg ICBJUEM6OmRlY29kZSwgdGhpcyBleGNlcHRpb24gbWlnaHQgb3IgbWlnaHQgbm90IGJlIGNhdWdo dCwgbGVhZGluZyB0byBkcm9wcGVkIG1lc3NhZ2VzIG9yCisgICAgICAgIGNyYXNoZXMuCisKKyAg ICAgICAgT25lIGNhc2Ugb2YgdGhpcyBoYXBwZW5pbmcgaXMgdHJhY2tlZCBieSByZGFyOi8vcHJv YmxlbS81NTgzOTQ2Ny4gSW4gdGhpcyBjYXNlLCB0aGUgbmlsCisgICAgICAgIGFsbG93ZWQgY2xh c3Mgd2FzIGR1ZSB0byBhIGJ1aWxkIG1pc2NvbmZpZ3VyYXRpb24sIGFuZCB0aGUgZXhjZXB0aW9u IGNhdXNlZCB0aGUgVUkgcHJvY2VzcworICAgICAgICB0byBub3QgcmVzcG9uZCB0byBhIHN5bmNo cm9ub3VzIElQQyBtZXNzYWdlLCBoYW5naW5nIHRoZSBXZWJDb250ZW50IHByb2Nlc3MuCisKKyAg ICAgICAgcmRhcjovL3Byb2JsZW0vNTU4Mzk0Njcgd2FzIHJlc29sdmVkIGJ5IGZpeGluZyB0aGUg YnVpbGQgbWlzY29uZmlndXJhdGlvbiwgYnV0IHRoaXMgcGF0Y2gKKyAgICAgICAgaW1wcm92ZXMg SVBDOjpkZWNvZGUgc28gdGhhdCBhIG5pbCBhbGxvd2VkIGNsYXNzIHJlc3VsdHMgaW4gYSBtZXNz YWdlIGRlY29kaW5nIGZhaWx1cmUKKyAgICAgICAgcmF0aGVyIHRoYW4gYSBtYXliZS1jYXVnaHQg TlNFeGNlcHRpb24uCisKKyAgICAgICAgKiBTaGFyZWQvQ29jb2EvQXJndW1lbnRDb2RlcnNDb2Nv YS5oOgorICAgICAgICAoSVBDOjpkZWNvZGUpOgorCiAyMDE5LTEwLTA5ICB5b3Vlbm4gZmFibGV0 ICA8eW91ZW5uQGFwcGxlLmNvbT4KIAogICAgICAgICBSZW1vdmUgdGVzdFJ1bm5lci5zZXRXZWJS VENVbmlmaWVkUGxhbkVuYWJsZWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvU2hhcmVkL0Nv Y29hL0FyZ3VtZW50Q29kZXJzQ29jb2EuaCBiL1NvdXJjZS9XZWJLaXQvU2hhcmVkL0NvY29hL0Fy Z3VtZW50Q29kZXJzQ29jb2EuaAppbmRleCA2ZDYxNTFjYmQyYjdiNDRkZDU0ZGYzZTE4MmZlYjk2 ZWNkODIwYjhkLi45YThiNTc4MTFiZTUxMzYxMTllMmE2YWI2NzM5NGEwN2ZjOTI2OTI3IDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViS2l0L1NoYXJlZC9Db2NvYS9Bcmd1bWVudENvZGVyc0NvY29hLmgK KysrIGIvU291cmNlL1dlYktpdC9TaGFyZWQvQ29jb2EvQXJndW1lbnRDb2RlcnNDb2NvYS5oCkBA IC04NCw3ICs4NCw3IEBAIE9wdGlvbmFsPFJldGFpblB0cjxUPj4gZGVjb2RlKERlY29kZXImIGRl Y29kZXIsIE5TQXJyYXk8Q2xhc3M+ICphbGxvd2VkQ2xhc3NlcykKIHRlbXBsYXRlPHR5cGVuYW1l IFQsIHR5cGVuYW1lPgogT3B0aW9uYWw8UmV0YWluUHRyPFQ+PiBkZWNvZGUoRGVjb2RlciYgZGVj b2RlciwgQ2xhc3MgYWxsb3dlZENsYXNzKQogewotICAgIHJldHVybiBkZWNvZGU8VD4oZGVjb2Rl ciwgQFsgYWxsb3dlZENsYXNzIF0pOworICAgIHJldHVybiBkZWNvZGU8VD4oZGVjb2RlciwgYWxs b3dlZENsYXNzID8gQFsgYWxsb3dlZENsYXNzIF0gOiBAWyBdKTsKIH0KIAogdGVtcGxhdGU8dHlw ZW5hbWUgVD4gc3RydWN0IEFyZ3VtZW50Q29kZXI8VCAqPiB7Cg==