Bug 219620 - Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle
Summary: Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 222172
Blocks:
  Show dependency treegraph
 
Reported: 2020-12-07 16:01 PST by Chris Dumez
Modified: 2021-02-19 08:14 PST (History)
6 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2020-12-07 16:06 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2020-12-08 13:16 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (12.08 KB, patch)
2020-12-08 14:22 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2020-12-07 16:01:29 PST
Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle:
Thread 0 Crashed:
0   libsystem_malloc.dylib        	0x00007fff20348dfa _malloc_zone_malloc + 114
1   com.apple.CoreFoundation      	0x00007fff20592f14 __CFStrAllocateMutableContents + 61
2   com.apple.CoreFoundation      	0x00007fff20592678 __CFStringChangeSizeMultiple + 677
3   com.apple.CoreFoundation      	0x00007fff205b248b __CFStringAppendBytes + 584
4   com.apple.CoreFoundation      	0x00007fff205afca9 __CFStringAppendFormatCore + 8684
5   com.apple.CoreFoundation      	0x00007fff206de734 _CFStringCreateWithFormatAndArgumentsReturningMetadata + 159
6   com.apple.CoreFoundation      	0x00007fff205adaaf _CFStringCreateWithFormatAndArgumentsAux2 + 20
7   com.apple.Foundation          	0x00007fff21346eef +[NSString stringWithFormat:] + 153
8   com.apple.Foundation          	0x00007fff213787f5 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1029
9   com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
10  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
11  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
12  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
13  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
14  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
15  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
16  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
17  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
18  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
19  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
20  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
21  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
22  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
23  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
24  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
25  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
26  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
27  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
28  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
29  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
30  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
31  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
32  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
33  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
34  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
35  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
36  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
37  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
38  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
39  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
40  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
41  com.apple.WebKit              	0x0000000114994d06 encodeObject(WKRemoteObjectEncoder*, objc_object*) + 822 (WKRemoteObjectCoder.mm:321)
42  com.apple.WebKit              	0x000000011498f5c1 createEncodedObject(WKRemoteObjectEncoder*, objc_object*) + 161 (WKRemoteObjectCoder.mm:332)
43  com.apple.WebKit              	0x000000011498f3f4 -[WKRemoteObjectEncoder encodeObject:forKey:] + 84 (WKRemoteObjectCoder.mm:430)
44  com.apple.Foundation          	0x00007fff21378860 -[NSDictionary(NSDictionary) encodeWithCoder:] + 1136
Comment 1 Chris Dumez 2020-12-07 16:01:48 PST
<rdar://71551776>
Comment 2 Chris Dumez 2020-12-07 16:06:17 PST
Created attachment 415598 [details]
Patch
Comment 3 Geoffrey Garen 2020-12-07 20:20:28 PST
Comment on attachment 415598 [details]
Patch

r=me

Seems OK, but aren't we just going to keep crashing? (NSException is fatal.)

Maybe a better behavior would be to log an error and skip the recursion.
Comment 4 Chris Dumez 2020-12-08 08:39:19 PST
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 415598 [details]
> Patch
> 
> r=me
> 
> Seems OK, but aren't we just going to keep crashing? (NSException is fatal.)

It depends if the call sites are dealing with exceptions or not. Exceptions could already be raised by encoder so in theory they should. However, I agree that in practice, this may keep crashing. Hopefully the crash would be more useful though.

> 
> Maybe a better behavior would be to log an error and skip the recursion.


Yes, this is a viable alternative although I am not 100% clear what "skip the recursion" means in practice. We are being asked to encode an object. I guess I could ignore the passed in object and encode NSNull instead?
Comment 5 Chris Dumez 2020-12-08 11:02:52 PST
(In reply to Chris Dumez from comment #4)
> (In reply to Geoffrey Garen from comment #3)
> > Comment on attachment 415598 [details]
> > Patch
> > 
> > r=me
> > 
> > Seems OK, but aren't we just going to keep crashing? (NSException is fatal.)
> 
> It depends if the call sites are dealing with exceptions or not. Exceptions
> could already be raised by encoder so in theory they should. However, I
> agree that in practice, this may keep crashing. Hopefully the crash would be
> more useful though.
> 
> > 
> > Maybe a better behavior would be to log an error and skip the recursion.
> 
> 
> Yes, this is a viable alternative although I am not 100% clear what "skip
> the recursion" means in practice. We are being asked to encode an object. I
> guess I could ignore the passed in object and encode NSNull instead?

I tried encoding NSNull and this caused an exception to be raised later on when decoding instead. NSNull is not part of the allowed class types. Even if it was part of the allowed class types though, there is no guarantee decoding would succeed in general if what we encoded does not match exactly what the decoder expects.
Comment 6 Geoffrey Garen 2020-12-08 11:21:29 PST
Maybe encode the container as an empty container, or just skip that specific entry in the container?
Comment 7 Chris Dumez 2020-12-08 12:33:38 PST
(In reply to Geoffrey Garen from comment #6)
> Maybe encode the container as an empty container, or just skip that specific
> entry in the container?

We are encoding objects of virtually any type. I happened to use an NSDictionary in my API test for convenience but I don't think a solution specific to container types is suitable?
Comment 8 Chris Dumez 2020-12-08 13:16:24 PST
Created attachment 415669 [details]
Patch
Comment 9 Chris Dumez 2020-12-08 13:17:50 PST
(In reply to Chris Dumez from comment #8)
> Created attachment 415669 [details]
> Patch

Here is another iteration that tries to avoid raising an exception when possible. When a cycle is detected, we encode a default-initialized version of the object instead of the object itself. If we fail to default-initialize an object of the same type, we still raise an exception though.
Comment 10 Geoffrey Garen 2020-12-08 14:11:00 PST
Comment on attachment 415669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415669&action=review

> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:316
> +            id newObject = [[[object class] alloc] init];

Should we use objectClass here instead?

I think we need an autorelease after the init to avoid a leak.
Comment 11 Chris Dumez 2020-12-08 14:13:57 PST
Comment on attachment 415669 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=415669&action=review

>> Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:316
>> +            id newObject = [[[object class] alloc] init];
> 
> Should we use objectClass here instead?
> 
> I think we need an autorelease after the init to avoid a leak.

objectClass is [object classForCoder], not [object class]. I am trying to create a new object of same class as |object| so I think my code is correct. Using [object classForCoder] means I would potentially create an object of a different type.

Good point about the leaking. I will fix.
Comment 12 Chris Dumez 2020-12-08 14:22:31 PST
Created attachment 415677 [details]
Patch
Comment 13 Geoffrey Garen 2020-12-08 14:26:57 PST
Comment on attachment 415677 [details]
Patch

r=me
Comment 14 EWS 2020-12-08 15:45:48 PST
Committed r270559: <https://trac.webkit.org/changeset/270559>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415677 [details].
Comment 15 Chris Dumez 2021-02-19 08:14:21 PST
This patch did not suffice and I now have a reproduction case. I am following up with Bug 222172.