Changeset 270559 in webkit

Timestamp:

Dec 8, 2020 3:45:23 PM (3 years ago)

Author:

Chris Dumez

Message:

Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle
​https://bugs.webkit.org/show_bug.cgi?id=219620
Source/WebKit:

<rdar://71551776>

Reviewed by Geoffrey Garen.

Update WKRemoteObjectEncoder to detect cycles when encoding objects. When a cycle is detected, we
first attempt to encode a default-initialized object of the same type instead. If that fails, we
raise a NSInvalidArgumentException.

Based on crashes in the wild, we have evidence that such cycles are occuring and I suspect this is
caused by Norton Safe Web extension somehow.

• Shared/API/Cocoa/WKRemoteObjectCoder.mm:

(-[WKRemoteObjectEncoder init]):
(encodeObject):

Tools:

Reviewed by Geoffrey Garen.

Add API test coverage.

• TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.h:
• TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm:

(TEST):

• TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistryPlugIn.mm:

(-[RemoteObjectRegistryPlugIn takeDictionary:completionHandler:]):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm (modified) (4 diffs)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.h (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm (modified) (2 diffs)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistryPlugIn.mm (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-12-08  Chris Dumez  <cdumez@apple.com>
+
+        Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle
+        https://bugs.webkit.org/show_bug.cgi?id=219620
+        <rdar://71551776>
+
+        Reviewed by Geoffrey Garen.
+
+        Update WKRemoteObjectEncoder to detect cycles when encoding objects. When a cycle is detected, we
+        first attempt to encode a default-initialized object of the same type instead. If that fails, we
+        raise a NSInvalidArgumentException.
+
+        Based on crashes in the wild, we have evidence that such cycles are occuring and I suspect this is
+        caused by Norton Safe Web extension somehow.
+
+        * Shared/API/Cocoa/WKRemoteObjectCoder.mm:
+        (-[WKRemoteObjectEncoder init]):
+        (encodeObject):
+
 2020-12-08  Simon Fraser  <simon.fraser@apple.com>
 

trunk/Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm

@@ -32 +32 @@
 #import "APINumber.h"
 #import "APIString.h"
+#import "Logging.h"
 #import "NSInvocationSPI.h"
 #import "_WKRemoteObjectInterfaceInternal.h"
 #import <objc/runtime.h>
 #import <wtf/RetainPtr.h>
+#import <wtf/Scope.h>
 #import <wtf/SetForScope.h>
 #import <wtf/text/CString.h>
@@ -62 +64 @@
 
     API::Dictionary* _currentDictionary;
+    RetainPtr<NSMutableSet> _objectsBeingEncoded; // Used to detect cycles.
 }
 
@@ -71 +74 @@
     _rootDictionary = API::Dictionary::create();
     _currentDictionary = _rootDictionary.get();
+    _objectsBeingEncoded = adoptNS([[NSMutableSet alloc] init]);
 
     return self;
@@ -305 +309 @@
     if (!objectClass)
         [NSException raise:NSInvalidArgumentException format:@"-classForCoder returned nil for %@", object];
+
+    if ([encoder->_objectsBeingEncoded containsObject:object]) {
+        RELEASE_LOG_FAULT(IPC, "WKRemoteObjectCode::encodeObject: Object of type '%{private}s' contains a cycle", class_getName(object_getClass(object)));
+        @try {
+            // Try to encode a newly initialized object instead.
+            id newObject = [[[[object class] alloc] init] autorelease];
+            object = newObject;
+        } @catch (NSException *e) {
+            [NSException raise:NSInvalidArgumentException format:@"Object of type '%s' contains a cycle", class_getName(object_getClass(object))];
+        }
+    }
+
+    [encoder->_objectsBeingEncoded addObject:object];
+    auto exitScope = makeScopeExit([encoder, object] {
+        [encoder->_objectsBeingEncoded removeObject:object];
+    });
 
     encoder->_currentDictionary->set(classNameKey, API::String::create(class_getName(objectClass)));

trunk/Tools/ChangeLog

@@ +1 @@
+2020-12-08  Chris Dumez  <cdumez@apple.com>
+
+        Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle
+        https://bugs.webkit.org/show_bug.cgi?id=219620
+
+        Reviewed by Geoffrey Garen.
+
+        Add API test coverage.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.h:
+        * TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm:
+        (TEST):
+        * TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistryPlugIn.mm:
+        (-[RemoteObjectRegistryPlugIn takeDictionary:completionHandler:]):
+
 2020-12-08  Jonathan Bedard  <jbedard@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.h

@@ -49 +49 @@
 - (void)takeUnsignedLong:(unsigned long)value completionHandler:(void (^)(unsigned long value))completionHandler;
 - (void)takeLong:(long)value completionHandler:(void (^)(long value))completionHandler;
+- (void)takeDictionary:(NSDictionary *)value completionHandler:(void (^)(NSDictionary *value))completionHandler;
 - (void)doNotCallCompletionHandler:(void (^)())completionHandler;
 - (void)sendRequest:(NSURLRequest *)request response:(NSURLResponse *)response challenge:(NSURLAuthenticationChallenge *)challenge error:(NSError *)error completionHandler:(void (^)(NSURLRequest *, NSURLResponse *, NSURLAuthenticationChallenge *, NSError *))completionHandler;

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistry.mm

@@ -127 +127 @@
         isDone = false;
 
-        class DoneWhenDestroyed : public RefCounted<DoneWhenDestroyed> {
-        public:
-            DoneWhenDestroyed(bool& isDone)
-                : isDone(isDone) { }
-            ~DoneWhenDestroyed() { isDone = true; }
-        private:
-            bool& isDone;
-        };
-
-        {
-            RefPtr<DoneWhenDestroyed> doneWhenDestroyed = adoptRef(*new DoneWhenDestroyed(isDone));
-            [object doNotCallCompletionHandler:[doneWhenDestroyed]() {
-            }];
-        }
-
-        TestWebKitAPI::Util::run(&isDone);
-
         NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"https://webkit.org/"]];
         NSHTTPURLResponse *response = [[[NSHTTPURLResponse alloc] initWithURL:[NSURL URLWithString:@"https://webkit.org/"] statusCode:200 HTTPVersion:@"HTTP/1.1" headerFields:@{ @"testFieldName" : @"testFieldValue" }] autorelease];
@@ -159 +142 @@
         }];
         TestWebKitAPI::Util::run(&isDone);
+
+        isDone = false;
+
+        bool exceptionThrown = false;
+        NSMutableDictionary *child = [NSMutableDictionary dictionaryWithObjectsAndKeys:@"foo", @"name", [NSNumber numberWithInt:1], @"value", nil];
+        NSMutableDictionary *dictionaryWithCycle = [NSMutableDictionary dictionaryWithObjectsAndKeys:@"root", @"name", child, @"child", nil];
+        [child setValue:dictionaryWithCycle forKey:@"parent"]; // Creates a cycle.
+        @try {
+            [object takeDictionary:dictionaryWithCycle completionHandler:^(NSDictionary* value) {
+                NSString *name = [value objectForKey:@"name"];
+                EXPECT_WK_STREQ(@"root", name);
+                NSDictionary *child = [value objectForKey:@"child"];
+                EXPECT_TRUE(!!child);
+                NSString* childName = [child objectForKey:@"name"];
+                EXPECT_WK_STREQ(@"foo", childName);
+                NSNumber *childValue = [child objectForKey:@"value"];
+                EXPECT_EQ(1, [childValue integerValue]);
+                // We should have encoded parent as an empty NSDictionary.
+                NSDictionary *childParent = [child objectForKey:@"parent"];
+                EXPECT_TRUE(!!childParent);
+                EXPECT_EQ(0U, [childParent count]);
+                isDone = true;
+            }];
+            TestWebKitAPI::Util::run(&isDone);
+            isDone = false;
+        } @catch (NSException *e) {
+            exceptionThrown = true;
+        }
+        EXPECT_FALSE(exceptionThrown);
+
+        class DoneWhenDestroyed : public RefCounted<DoneWhenDestroyed> {
+        public:
+            DoneWhenDestroyed(bool& isDone)
+                : isDone(isDone) { }
+            ~DoneWhenDestroyed() { isDone = true; }
+        private:
+            bool& isDone;
+        };
+
+        {
+            RefPtr<DoneWhenDestroyed> doneWhenDestroyed = adoptRef(*new DoneWhenDestroyed(isDone));
+            [object doNotCallCompletionHandler:[doneWhenDestroyed]() {
+            }];
+        }
+
+        TestWebKitAPI::Util::run(&isDone);
     }
 }

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/RemoteObjectRegistryPlugIn.mm

@@ -105 +105 @@
 }
 
+- (void)takeDictionary:(NSDictionary *)value completionHandler:(void (^)(NSDictionary *value))completionHandler
+{
+    completionHandler(value);
+}
+
 - (void)doNotCallCompletionHandler:(void (^)())completionHandler
 {