Limit the recursion depth when trying to get the mangling for a context descriptor #35314
Conversation
…xt descriptor This should help guard against corrupted data in the target app when debugging.
| @@ -1131,7 +1131,7 @@ class MetadataReader { | |||
| Demangle::NodePointer | |||
| buildContextMangling(ContextDescriptorRef descriptor, | |||
| Demangler &dem) { | |||
| auto demangling = buildContextDescriptorMangling(descriptor, dem); | |||
| auto demangling = buildContextDescriptorMangling(descriptor, dem, 50); | |||
There was a problem hiding this comment.
@jckarter Does 50 seem like a sufficient limit here?
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test Windows platform |
|
@swift-ci Please test macOS |
|
Restarted testing; looks like there are random test timeouts keeping this from finishing. |
| if (descriptor.isResolved()) { | ||
| return buildContextDescriptorMangling(descriptor.getResolved(), dem); | ||
| return buildContextDescriptorMangling(descriptor.getResolved(), dem, recursion_limit - 1); |
There was a problem hiding this comment.
You're cutting the actual recursion limit in half by decrementing here too, since every time we try to resolve a parent we'll end up decrementing recursion_limit twice.
|
A type could legitimately be scoped 25 levels deep; maybe that's unlikely, but it'd be unfortunate for debugging to mysteriously stop working if it does happen. Maybe we need a more robust way of detecting valid pointers into the remote object. We know that context descriptors can only ever be in the immutable |
|
@jckarter I agree that more checks are needed here, but I don't think we can avoid some kind of recursion limit since malformed data could potentially have cycles, and simply checking for valid pointers can't guard against that. I've fixed the double-counting, so we're now supporting 50 levels of nesting. Based on your comment, I'm guessing that should be sufficient? |
|
If we're primarily concerned about cycles, then could we track a set of already-visited pointers, and error out if we see a real cycle? If we're going to put an arbitrary limit here, then IMO we should also impose the limit consistently in the compiler and other tools. |
|
(Or, you don't actually need to track a set; you can do the old "find cycles in a linked list" trick by keeping another pointer that you update at double speed through the list and check if the two pointers ever line up) |
|
@swift-ci Please test |
|
Even if we restrict this to known immutable memory, we could get a bad pointer into something that happens to be immutable memory, and end up chasing things down in what would essentially be garbage data. I really like the idea of detecting cycles explicitly, but I'm also afraid that we'll encounter garbage that just happens to present as a non-cyclic structure deep enough to overflow the stack. |
|
@swift-ci Please test |
|
@swift-ci Please test Windows platform |
1 similar comment
|
@swift-ci Please test Windows platform |
|
Build failed |
|
@swift-ci Please test |
1 similar comment
|
@swift-ci Please test |
|
Build failed |
|
@swift-ci Please test macOS Platform |
1 similar comment
|
@swift-ci Please test macOS Platform |
|
Build failed |
|
@swift-ci Please test Linux platform |
This should help guard against corrupted data in the target app when debugging.
Resolves rdar://66442021