Bug 219620 – Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

<rdar://71551776>

[Chris Dumez]

Created attachment 415598 [details]
Patch

[Geoffrey Garen]

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.

[Chris Dumez]

(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?

[Chris Dumez]

(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.

[Geoffrey Garen]

Maybe encode the container as an empty container, or just skip that specific entry in the container?

[Chris Dumez]

(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?

[Chris Dumez]

Created attachment 415669 [details]
Patch

[Chris Dumez]

(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.

[Geoffrey Garen]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 415677 [details]
Patch

[Geoffrey Garen]

Comment on attachment 415677 [details]
Patch

r=me

[EWS]

Committed r270559: <https://trac.webkit.org/changeset/270559>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415677 [details].

[Chris Dumez]

This patch did not suffice and I now have a reproduction case. I am following up with Bug 222172.