| Summary: | Potential crash under [WKRemoteObjectEncoder encodeObject:forKey:] when the object graph contains a cycle | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | achristensen, beidson, ddkilzer, ggaren, thorton, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 222172 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2020-12-07 16:01:29 PST
Created attachment 415598 [details]
Patch
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.
(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? (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. Maybe encode the container as an empty container, or just skip that specific entry in the container? (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? Created attachment 415669 [details]
Patch
(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 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 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. Created attachment 415677 [details]
Patch
Comment on attachment 415677 [details]
Patch
r=me
Committed r270559: <https://trac.webkit.org/changeset/270559> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415677 [details]. This patch did not suffice and I now have a reproduction case. I am following up with Bug 222172. |