Log Message
WKRemoteObjectRegistry _invokeMethod needs to check for nil completionHandlers https://bugs.webkit.org/show_bug.cgi?id=225941
Patch by Julian Gonzalez <[email protected]> on 2021-05-20 Reviewed by Ryosuke Niwa. Source/WebKit: _invokeMethod's argument-checking loop needs to be run even if replyInfo is nil, as otherwise we can perform an invocation if a method signature specifies a completion handler even though none is provided. * Shared/API/Cocoa/_WKRemoteObjectRegistry.mm: (-[_WKRemoteObjectRegistry _invokeMethod:]): Tools: Add an IPC test with a nil (really malformed) completion handler that makes sure the invocation is not performed. * TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm: (-[IPCTestingAPIDelegate sayHello:completionHandler:]): (-[IPCTestingAPIDelegate sayHelloWasCalled]): (TEST):
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (277848 => 277849)
--- trunk/Source/WebKit/ChangeLog 2021-05-21 03:15:28 UTC (rev 277848)
+++ trunk/Source/WebKit/ChangeLog 2021-05-21 04:14:55 UTC (rev 277849)
@@ -1,3 +1,18 @@
+2021-05-20 Julian Gonzalez <[email protected]>
+
+ WKRemoteObjectRegistry _invokeMethod needs to check for nil completionHandlers
+ https://bugs.webkit.org/show_bug.cgi?id=225941
+
+ Reviewed by Ryosuke Niwa.
+
+ _invokeMethod's argument-checking loop needs to be run
+ even if replyInfo is nil, as otherwise we can perform an invocation
+ if a method signature specifies a completion handler even though
+ none is provided.
+
+ * Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:
+ (-[_WKRemoteObjectRegistry _invokeMethod:]):
+
2021-05-20 Aditya Keerthi <[email protected]>
[iOS][FCR] <select> options are unnecessarily truncated
Modified: trunk/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm (277848 => 277849)
--- trunk/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm 2021-05-21 03:15:28 UTC (rev 277848)
+++ trunk/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm 2021-05-21 04:14:55 UTC (rev 277849)
@@ -253,85 +253,90 @@
NSLog(@"Exception caught during decoding of message: %@", exception);
}
- if (auto* replyInfo = remoteObjectInvocation.replyInfo()) {
- NSMethodSignature *methodSignature = invocation.methodSignature;
+ NSMethodSignature *methodSignature = invocation.methodSignature;
+ auto* replyInfo = remoteObjectInvocation.replyInfo();
- // Look for the block argument.
- for (NSUInteger i = 0, count = methodSignature.numberOfArguments; i < count; ++i) {
- const char *type = [methodSignature getArgumentTypeAtIndex:i];
+ // Look for the block argument (if any).
+ for (NSUInteger i = 0, count = methodSignature.numberOfArguments; i < count; ++i) {
+ const char *type = [methodSignature getArgumentTypeAtIndex:i];
- if (strcmp(type, "@?"))
- continue;
+ if (strcmp(type, "@?"))
+ continue;
- // We found the block.
- String wireBlockSignature = [NSMethodSignature signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()]._typeString.UTF8String;
- String expectedBlockSignature = replyBlockSignature([interface protocol], invocation.selector, i);
+ // We found the block.
+ // If the wire had no block signature but we expect one, we drop the message.
+ if (!replyInfo) {
+ NSLog(@"_invokeMethod: Expected reply block, but none provided");
+ return;
+ }
- if (expectedBlockSignature.isNull()) {
- NSLog(@"_invokeMethod: Failed to validate reply block signature: could not find local signature");
- ASSERT_NOT_REACHED();
- continue;
- }
+ String wireBlockSignature = [NSMethodSignature signatureWithObjCTypes:replyInfo->blockSignature.utf8().data()]._typeString.UTF8String;
+ String expectedBlockSignature = replyBlockSignature([interface protocol], invocation.selector, i);
- if (!blockSignaturesAreCompatible(wireBlockSignature, expectedBlockSignature)) {
- NSLog(@"_invokeMethod: Failed to validate reply block signature: %s != %s", wireBlockSignature.utf8().data(), expectedBlockSignature.utf8().data());
- ASSERT_NOT_REACHED();
- continue;
- }
+ if (expectedBlockSignature.isNull()) {
+ NSLog(@"_invokeMethod: Failed to validate reply block signature: could not find local signature");
+ ASSERT_NOT_REACHED();
+ continue;
+ }
- RetainPtr<_WKRemoteObjectRegistry> remoteObjectRegistry = self;
- uint64_t replyID = replyInfo->replyID;
+ if (!blockSignaturesAreCompatible(wireBlockSignature, expectedBlockSignature)) {
+ NSLog(@"_invokeMethod: Failed to validate reply block signature: %s != %s", wireBlockSignature.utf8().data(), expectedBlockSignature.utf8().data());
+ ASSERT_NOT_REACHED();
+ continue;
+ }
- class ReplyBlockCallChecker : public WTF::ThreadSafeRefCounted<ReplyBlockCallChecker> {
- public:
- static Ref<ReplyBlockCallChecker> create(_WKRemoteObjectRegistry *registry, uint64_t replyID) { return adoptRef(*new ReplyBlockCallChecker(registry, replyID)); }
+ RetainPtr<_WKRemoteObjectRegistry> remoteObjectRegistry = self;
+ uint64_t replyID = replyInfo->replyID;
- ~ReplyBlockCallChecker()
- {
- if (m_didCallReplyBlock)
- return;
+ class ReplyBlockCallChecker : public WTF::ThreadSafeRefCounted<ReplyBlockCallChecker> {
+ public:
+ static Ref<ReplyBlockCallChecker> create(_WKRemoteObjectRegistry *registry, uint64_t replyID) { return adoptRef(*new ReplyBlockCallChecker(registry, replyID)); }
- // FIXME: Instead of not sending anything when the remote object registry is null, we should
- // keep track of all reply block checkers and invalidate them (sending the unused reply message) in
- // -[_WKRemoteObjectRegistry _invalidate].
- if (!m_remoteObjectRegistry->_remoteObjectRegistry)
- return;
+ ~ReplyBlockCallChecker()
+ {
+ if (m_didCallReplyBlock)
+ return;
- m_remoteObjectRegistry->_remoteObjectRegistry->sendUnusedReply(m_replyID);
- }
+ // FIXME: Instead of not sending anything when the remote object registry is null, we should
+ // keep track of all reply block checkers and invalidate them (sending the unused reply message) in
+ // -[_WKRemoteObjectRegistry _invalidate].
+ if (!m_remoteObjectRegistry->_remoteObjectRegistry)
+ return;
- void didCallReplyBlock() { m_didCallReplyBlock = true; }
+ m_remoteObjectRegistry->_remoteObjectRegistry->sendUnusedReply(m_replyID);
+ }
- private:
- ReplyBlockCallChecker(_WKRemoteObjectRegistry *registry, uint64_t replyID)
- : m_remoteObjectRegistry(registry)
- , m_replyID(replyID)
- {
- }
+ void didCallReplyBlock() { m_didCallReplyBlock = true; }
- RetainPtr<_WKRemoteObjectRegistry> m_remoteObjectRegistry;
- uint64_t m_replyID = 0;
- bool m_didCallReplyBlock = false;
- };
+ private:
+ ReplyBlockCallChecker(_WKRemoteObjectRegistry *registry, uint64_t replyID)
+ : m_remoteObjectRegistry(registry)
+ , m_replyID(replyID)
+ {
+ }
- RefPtr<ReplyBlockCallChecker> checker = ReplyBlockCallChecker::create(self, replyID);
- id replyBlock = __NSMakeSpecialForwardingCaptureBlock(wireBlockSignature.utf8().data(), [interface, remoteObjectRegistry, replyID, checker](NSInvocation *invocation) {
- auto encoder = adoptNS([[WKRemoteObjectEncoder alloc] init]);
- [encoder encodeObject:invocation forKey:invocationKey];
+ RetainPtr<_WKRemoteObjectRegistry> m_remoteObjectRegistry;
+ uint64_t m_replyID = 0;
+ bool m_didCallReplyBlock = false;
+ };
- if (remoteObjectRegistry->_remoteObjectRegistry)
- remoteObjectRegistry->_remoteObjectRegistry->sendReplyBlock(replyID, WebKit::UserData([encoder rootObjectDictionary]));
- checker->didCallReplyBlock();
- });
+ RefPtr<ReplyBlockCallChecker> checker = ReplyBlockCallChecker::create(self, replyID);
+ id replyBlock = __NSMakeSpecialForwardingCaptureBlock(wireBlockSignature.utf8().data(), [interface, remoteObjectRegistry, replyID, checker](NSInvocation *invocation) {
+ auto encoder = adoptNS([[WKRemoteObjectEncoder alloc] init]);
+ [encoder encodeObject:invocation forKey:invocationKey];
- [invocation setArgument:&replyBlock atIndex:i];
+ if (remoteObjectRegistry->_remoteObjectRegistry)
+ remoteObjectRegistry->_remoteObjectRegistry->sendReplyBlock(replyID, WebKit::UserData([encoder rootObjectDictionary]));
+ checker->didCallReplyBlock();
+ });
- // Make sure that the block won't be destroyed before the invocation.
- objc_setAssociatedObject(invocation, replyBlockKey, replyBlock, OBJC_ASSOCIATION_RETAIN);
- [replyBlock release];
+ [invocation setArgument:&replyBlock atIndex:i];
- break;
- }
+ // Make sure that the block won't be destroyed before the invocation.
+ objc_setAssociatedObject(invocation, replyBlockKey, replyBlock, OBJC_ASSOCIATION_RETAIN);
+ [replyBlock release];
+
+ break;
}
invocation.target = interfaceAndObject.first.get();
Modified: trunk/Tools/ChangeLog (277848 => 277849)
--- trunk/Tools/ChangeLog 2021-05-21 03:15:28 UTC (rev 277848)
+++ trunk/Tools/ChangeLog 2021-05-21 04:14:55 UTC (rev 277849)
@@ -1,3 +1,18 @@
+2021-05-20 Julian Gonzalez <[email protected]>
+
+ WKRemoteObjectRegistry _invokeMethod needs to check for nil completionHandlers
+ https://bugs.webkit.org/show_bug.cgi?id=225941
+
+ Reviewed by Ryosuke Niwa.
+
+ Add an IPC test with a nil (really malformed) completion handler
+ that makes sure the invocation is not performed.
+
+ * TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:
+ (-[IPCTestingAPIDelegate sayHello:completionHandler:]):
+ (-[IPCTestingAPIDelegate sayHelloWasCalled]):
+ (TEST):
+
2021-05-20 Alex Christensen <[email protected]>
Unreviewed, reverting r277606.
Modified: trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm (277848 => 277849)
--- trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm 2021-05-21 03:15:28 UTC (rev 277848)
+++ trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm 2021-05-21 04:14:55 UTC (rev 277849)
@@ -25,12 +25,16 @@
#import "config.h"
+#import "RemoteObjectRegistry.h"
#import "TestWKWebView.h"
#import "Utilities.h"
#import <WebKit/WKNavigationDelegatePrivate.h>
#import <WebKit/WKPreferencesPrivate.h>
#import <WebKit/WKWebView.h>
+#import <WebKit/WKWebViewPrivate.h>
#import <WebKit/_WKInternalDebugFeature.h>
+#import <WebKit/_WKRemoteObjectInterface.h>
+#import <WebKit/_WKRemoteObjectRegistry.h>
#import <wtf/RetainPtr.h>
static bool done = false;
@@ -40,9 +44,12 @@
static RetainPtr<NSString> promptResult;
@interface IPCTestingAPIDelegate : NSObject <WKUIDelegate, WKNavigationDelegate>
+- (BOOL)sayHelloWasCalled;
@end
-@implementation IPCTestingAPIDelegate
+@implementation IPCTestingAPIDelegate {
+ BOOL _didCallSayHello;
+}
- (void)webView:(WKWebView *)webView runJavaScriptAlertPanelWithMessage:(NSString *)message initiatedByFrame:(WKFrameInfo *)frame completionHandler:(void (^)(void))completionHandler
{
@@ -64,6 +71,16 @@
done = true;
}
+- (void)sayHello:(NSString *)hello completionHandler:(void (^)(NSString *))completionHandler
+{
+ _didCallSayHello = YES;
+}
+
+- (BOOL)sayHelloWasCalled
+{
+ return _didCallSayHello;
+}
+
@end
TEST(IPCTestingAPI, IsDisabledByDefault)
@@ -93,6 +110,58 @@
return adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 300, 300) configuration:configuration.get()]);
}
+TEST(IPCTestingAPI, CanDetectNilReplyBlocks)
+{
+ auto webView = createWebViewWithIPCTestingAPI();
+
+ auto delegate = adoptNS([[IPCTestingAPIDelegate alloc] init]);
+ [webView setUIDelegate:delegate.get()];
+
+ _WKRemoteObjectInterface *interface = remoteObjectInterface();
+ [[webView _remoteObjectRegistry] remoteObjectProxyWithInterface:interface];
+ [[webView _remoteObjectRegistry] registerExportedObject:delegate.get() interface:interface];
+
+ done = false;
+ [webView synchronouslyLoadHTMLString:@"<!DOCTYPE html><script>buf = new Uint8Array(["
+ // Strings in this buffer are encoded as follows:
+ // string length, 3 NUL bytes, 0x1 byte, then string contents
+ // For example, this string is 0x14 length (20 bytes), 3 NUL bytes + 0x1, then "RemoteObjectProtocol"
+ "0x14,0x0,0x0,0x0,0x1,0x52,0x65,0x6d,0x6f,0x74,0x65,0x4f,0x62,0x6a,0x65,0x63,0x74,0x50,0x72,0x6f,0x74,0x6f,0x63,0x6f,0x6c,"
+ // padding + "invocation"
+ "0x0,0x0,0x0,0x9,0x0,0x0,0x0,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xa,0x0,0x0,0x0,0x1,0x69,0x6e,0x76,0x6f,0x63,0x61,0x74,0x69,0x6f,0x6e,"
+ // a serialized object + "typeString"
+ "0x0,0x9,0x0,0x0,0x0,0xf5,0xeb,0x54,0xa9,0x3,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xa,0x0,0x0,0x0,0x1,0x74,0x79,0x70,0x65,0x53,0x74,0x72,0x69,0x6e,0x67,0x0,"
+ // a zeroed object + "$string"
+ "0x9,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7,0x0,0x0,0x0,0x1,0x24,0x73,0x74,0x72,0x69,0x6e,0x67,0x15,0x0,0x0,0x0,"
+ // "v@:@.@.?" (an objective-C method signature) + "class"
+ "0x6,0x0,0x0,0x0,0x1,0x76,0x40,0x3a,0x40,0x40,0x3f,0x0,0x6,0x0,0x0,0x0,0x1,0x24,0x63,0x6c,0x61,0x73,0x73,0x0,"
+ // "NSString" + "selector"
+ "0x15,0x0,0x0,0x0,0x8,0x0,0x0,0x0,0x1,0x4e,0x53,0x53,0x74,0x72,0x69,0x6e,0x67,0x0,0x0,0x0,0x8,0x0,0x0,0x0,0x1,0x73,0x65,0x6c,0x65,0x63,0x74,0x6f,0x72,0x0,0x0,0x0,"
+ // a zeroed object + "$string"
+ "0x9,0x0,0x0,0x0,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x7,0x0,0x0,0x0,0x1,0x24,0x73,0x74,0x72,0x69,0x6e,0x67,0x15,0x0,0x0,0x0,"
+ // "sayHello:completionHandler:" (method name we're trying to call)
+ "0x1b,0x0,0x0,0x0,0x1,0x73,0x61,0x79,0x48,0x65,0x6c,0x6c,0x6f,0x3a,0x63,0x6f,0x6d,0x70,0x6c,0x65,0x74,0x69,0x6f,0x6e,0x48,0x61,0x6e,0x64,0x6c,0x65,0x72,0x3a,"
+ // "$class" + "NSString"
+ "0x6,0x0,0x0,0x0,0x1,0x24,0x63,0x6c,0x61,0x73,0x73,0x0,0x15,0x0,0x0,0x0,0x8,0x0,0x0,0x0,0x1,0x4e,0x53,0x53,0x74,0x72,0x69,0x6e,0x67,0x0,0x0,0x0,"
+ // "$class" + "NSInvocation"
+ "0x6,0x0,0x0,0x0,0x1,0x24,0x63,0x6c,0x61,0x73,0x73,0x0,0x15,0x0,0x0,0x0,0xc,0x0,0x0,0x0,0x1,0x4e,0x53,0x49,0x6e,0x76,0x6f,0x63,0x61,0x74,0x69,0x6f,0x6e,0x0,0x0,0x0,"
+ // "$objectStam" + zero object
+ "0xd,0x0,0x0,0x0,0x1,0x24,0x6f,0x62,0x6a,0x65,0x63,0x74,0x53,0x74,0x61,0x6d,0x0,0x0,0x1,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,"
+ // zeroed objects + ".NS.uuidbytes"
+ "0x9,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x2,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0xc,0x0,0x0,0x0,0x91,0x4e,0x53,0x2e,0x75,0x75,0x69,0x64,0x62,0x79,0x74,0x65,0x73,0x0,0x0,0x0,"
+ // some zeroed objects
+ "0x8,0x0,0x0,0x0,0x10,0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x29,0xc5,0x6d,0x2,0x13,0xa,0x4e,0xe7,0xaa,0xac,0x8,0x55,0xf2,0x66,0x2c,0x7c,"
+ // "$class" + "NSUUID"
+ "0x6,0x0,0x0,0x0,0x1,0x24,0x63,0x6c,0x61,0x73,0x73,0x0,0x15,0x0,0x0,0x0,0x6,0x0,0x0,0x0,0x1,0x4e,0x53,0x55,0x55,0x49,0x44,0x0,0x0,0x0,"
+ // mostly zero objects + "v@?c" (objective-C method signature)
+ "0x0,0x0,0x1,0x0,0x0,0x0,0x2c,0x0,0x0,0x0,0x59,0x1,0x0,0x0,0x0,0x9b,0x0,0x0,0x4,0x0,0x0,0x0,0x1,0x76,0x40,0x3f,0x63,0x0,]);"
+ "for(var x=0; x<100; x++) IPC.sendMessage('UI', x, IPC.messages.RemoteObjectRegistry_InvokeMethod.name, [buf]);</script>"];
+ TestWebKitAPI::Util::runFor(&done, 1);
+
+ // Make sure sayHello was not called, as the reply block was nil.
+ EXPECT_FALSE([delegate.get() sayHelloWasCalled]);
+}
+
TEST(IPCTestingAPI, CanSendAlert)
{
auto webView = createWebViewWithIPCTestingAPI();
_______________________________________________ webkit-changes mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-changes
