[Clang] Fix null pointer dereference in enum debug info generation

[smanna12]

This patch prevents dereferencing a null enum definition by returning nullptr if getDefinition() fails in CreateTypeDefinition().

[llvmbot]

@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: None (smanna12)

Changes

This patch prevents dereferencing a null enum definition by returning nullptr if getDefinition() fails in CreateTypeDefinition().

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGDebugInfo.cpp (+4)

diff --git a/clang/lib/CodeGen/CGDebugInfo.cpp b/clang/lib/CodeGen/CGDebugInfo.cpp
index 3d8a715b692de..c236e2657fe98 100644
--- a/clang/lib/CodeGen/CGDebugInfo.cpp
+++ b/clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3518,6 +3518,10 @@ llvm::DIType *CGDebugInfo::CreateTypeDefinition(const EnumType *Ty) {
 
   SmallVector<llvm::Metadata *, 16> Enumerators;
   ED = ED->getDefinition();
+
+  if (!ED)
+    return nullptr;
+
   for (const auto *Enum : ED->enumerators()) {
     Enumerators.push_back(
         DBuilder.createEnumerator(Enum->getName(), Enum->getInitVal()));

[Michael137]

Do you have an example/reproducer where this would get exercised?

[tahonermann]

Thanks @smanna12. I think this looks ok; returning null here does appear to be consistent with other overloads of CreateTypeDefinition for RecordType and ObjCInterfaceType. I agree with @Michael137 that it would be nice to have an example that fails the added condition. The code already checks for an incomplete type so is presumably intended to handle such types. Perhaps we are missing a test though? Presumably one that uses a forward declareable enum type?

[dwblaikie]

Thanks @smanna12. I think this looks ok; returning null here does appear to be consistent with other overloads of CreateTypeDefinition for RecordType and ObjCInterfaceType. I agree with @Michael137 that it would be nice to have an example that fails the added condition. The code already checks for an incomplete type so is presumably intended to handle such types. Perhaps we are missing a test though? Presumably one that uses a forward declareable enum type?

Not a hard line - but I'd discourage approving a patch like this until the test issue has been resolved (either a test is added, or a good explanation for it being omitted has been documented).

[smanna12]

Thanks @smanna12. I think this looks ok; returning null here does appear to be consistent with other overloads of CreateTypeDefinition for RecordType and ObjCInterfaceType. I agree with @Michael137 that it would be nice to have an example that fails the added condition. The code already checks for an incomplete type so is presumably intended to handle such types. Perhaps we are missing a test though? Presumably one that uses a forward declareable enum type?

Thanks @tahonermann, @Michael137, @dwblaikie for reviews. I have added test. Does this address your concerns?

[dwblaikie]

Have you run this test? Because an unused enum like this usually isn't/shouldn't be emitted into the output... - note all the other enums have a variable declared of their type (the trailing "} xN;")

[tahonermann]

Please follow the naming patterns already used in the test and rename the new enumeration to E9.

I added this change to my local build, but the test still passes without the associated change to clang/lib/CodeGen/CGDebugInfo.cpp. That suggests this test doesn't actually exercise the code in question.

EnumDecl::getDefinition() (see here) defers to TagDecl::getDefinition() (see here). The only way for the latter to return null is if isCompleteDefinition() (see here) is false for every declaration of the enumeration. For isCompleteDefinition() to return true, there has to have been an earlier call to setCompleteDefinition(). There are few calls to that function, the only relevant one looks to be from TagDecl::completeDefinition() (see here) by way of EnumDecl::completeDefinition() (see here). Auditing calls to completeDefinition() is a bit challenging, so I didn't check them all. The only obvious cases I see related to enumerations are calls related to when an enumeration definition is present. Based on that, I would expect the test to suffice to trigger a null dereference; unless CGDebugInfo::CreateTypeDefinition() is only called when a complete definition is available (in which case, the existing check for isIncompleteType() seems unnecessary and can be removed).

Can you do a bit more debugging to see if calls to CGDebugInfo::CreateTypeDefinition(const EnumType *) are in fact gated on a complete type definition? If they are, then the checks for isIncompleteType() can be removed and a non-null result from ED->getDefinition() can be asserted.

[tahonermann]

I spent some time experimenting and debugging today but have not been able to find a way for CGDebugInfo::CreateTypeDefinition(const EnumType *) to be invoked without an available definition. I tried various uses of non-standard enumeration forward declarations, but non-suppressible errors were issued for every case I tried that I thought might lead to generation of a debug info type.

The most interesting scenario I played with involved use of the enumeration type within its own definition. In that context, the type is incomplete, but a definition is (being made) available. I wouldn't expect generation of debug info to ever be sensitive to such a context though. I know little about how debug information is generated, but seeing as the function under discussion is part of code generation, I wouldn't expect a not-yet-completed definition to ever be observed.

I think adding an assert as proposed in #105556 suffices to address the static analysis concern.

[smanna12]

I am unable to add new change in this PR. Follow-up is here: #105556.