Skip to content

[lldb] Defend against infinite recursion in GetClassDescriptor #145396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 24, 2025

Conversation

JDevlieghere
Copy link
Member

We defend against a direct cycle where a base class ValueObject is its own parent, but not against a longer base cycle. This cycle requires that some value's Type includes a base class, and that base class is in a class hierarchy that cycles back to the original base class.

I wrote a test case that creates a cycle in the class hierarchy by dynamically overwriting the superclass of an object, but I can't reproduce the crash. I can't think of any other way to make a real object that behaves that way. Maybe is a type system problem in making up the type for whatever type we're trying to ingest here.

While unsatisfying, without a reproducer this is the best we can do for now.

rdar://140293233

We defend against a direct cycle where a base class ValueObject is its
own parent, but not against a longer base cycle. This cycle requires
that some value's Type includes a base class, and that base class is in
a class hierarchy that cycles back to the original base class.

I wrote a test case that creates a cycle in the class hierarchy by
dynamically overwriting the superclass of an object, but I can't
reproduce the crash. I can't think of any other way to make a real
object that behaves that way.  Maybe is a type system problem in making
up the type for whatever type we're trying to ingest here.

While unsatisfying, without a reproducer this is the best we can do for
now.

rdar://140293233
@llvmbot
Copy link
Member

llvmbot commented Jun 23, 2025

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

Changes

We defend against a direct cycle where a base class ValueObject is its own parent, but not against a longer base cycle. This cycle requires that some value's Type includes a base class, and that base class is in a class hierarchy that cycles back to the original base class.

I wrote a test case that creates a cycle in the class hierarchy by dynamically overwriting the superclass of an object, but I can't reproduce the crash. I can't think of any other way to make a real object that behaves that way. Maybe is a type system problem in making up the type for whatever type we're trying to ingest here.

While unsatisfying, without a reproducer this is the best we can do for now.

rdar://140293233


Full diff: https://github.com/llvm/llvm-project/pull/145396.diff

2 Files Affected:

  • (modified) lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp (+15-6)
  • (modified) lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h (+5)
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
index e459cb281793a..4fcdebe5bac62 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
@@ -1505,15 +1505,24 @@ AppleObjCRuntimeV2::GetClassDescriptorFromISA(ObjCISA isa) {
 
 ObjCLanguageRuntime::ClassDescriptorSP
 AppleObjCRuntimeV2::GetClassDescriptor(ValueObject &valobj) {
+  ValueObjectSet seen;
+  return GetClassDescriptorImpl(valobj, seen);
+}
+
+ObjCLanguageRuntime::ClassDescriptorSP
+AppleObjCRuntimeV2::GetClassDescriptorImpl(ValueObject &valobj,
+                                           ValueObjectSet &seen) {
+  seen.insert(&valobj);
+
   ClassDescriptorSP objc_class_sp;
   if (valobj.IsBaseClass()) {
     ValueObject *parent = valobj.GetParent();
-    // if I am my own parent, bail out of here fast..
-    if (parent && parent != &valobj) {
-      ClassDescriptorSP parent_descriptor_sp = GetClassDescriptor(*parent);
-      if (parent_descriptor_sp)
-        return parent_descriptor_sp->GetSuperclass();
-    }
+    // Fail if there's a cycle in our parent chain.
+    if (!parent || seen.count(parent))
+      return nullptr;
+    if (ClassDescriptorSP parent_descriptor_sp =
+            GetClassDescriptorImpl(*parent, seen))
+      return parent_descriptor_sp->GetSuperclass();
     return nullptr;
   }
   // if we get an invalid VO (which might still happen when playing around with
diff --git a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
index 79840f9be79b3..e5dba5bddf936 100644
--- a/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
+++ b/lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.h
@@ -20,6 +20,7 @@
 #include "Plugins/LanguageRuntime/ObjC/ObjCLanguageRuntime.h"
 
 #include "llvm/ADT/BitVector.h"
+#include "llvm/ADT/SmallSet.h"
 
 class RemoteNXMapTable;
 
@@ -421,6 +422,10 @@ class AppleObjCRuntimeV2 : public AppleObjCRuntime {
 
   lldb::addr_t GetISAHashTablePointer();
 
+  using ValueObjectSet = llvm::SmallSet<ValueObject *, 8>;
+  ClassDescriptorSP GetClassDescriptorImpl(ValueObject &valobj,
+                                           ValueObjectSet &seen);
+
   /// Update the generation count of realized classes. This is not an exact
   /// count but rather a value that is incremented when new classes are realized
   /// or destroyed. Unlike the count in gdb_objc_realized_classes, it will

@JDevlieghere
Copy link
Member Author

CI checks failed due to #143739.

Copy link
Collaborator

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one comment

@JDevlieghere JDevlieghere merged commit b171ebb into llvm:main Jun 24, 2025
7 checks passed
@JDevlieghere JDevlieghere deleted the rdar140293233 branch June 24, 2025 19:28
anthonyhatran pushed a commit to anthonyhatran/llvm-project that referenced this pull request Jun 26, 2025
…145396)

We defend against a direct cycle where a base class ValueObject is its
own parent, but not against a longer base cycle. This cycle requires
that some value's Type includes a base class, and that base class is in
a class hierarchy that cycles back to the original base class.

I wrote a test case that creates a cycle in the class hierarchy by
dynamically overwriting the superclass of an object, but I can't
reproduce the crash. I can't think of any other way to make a real
object that behaves that way. Maybe is a type system problem in making
up the type for whatever type we're trying to ingest here.

While unsatisfying, without a reproducer this is the best we can do for
now.

rdar://140293233
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
…145396)

We defend against a direct cycle where a base class ValueObject is its
own parent, but not against a longer base cycle. This cycle requires
that some value's Type includes a base class, and that base class is in
a class hierarchy that cycles back to the original base class.

I wrote a test case that creates a cycle in the class hierarchy by
dynamically overwriting the superclass of an object, but I can't
reproduce the crash. I can't think of any other way to make a real
object that behaves that way. Maybe is a type system problem in making
up the type for whatever type we're trying to ingest here.

While unsatisfying, without a reproducer this is the best we can do for
now.

rdar://140293233
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants