Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a4f020
[Clang] Fix null pointer dereference in enum debug info generation
smanna12 Jun 28, 2024
0cf295b
Address review comments- add test
smanna12 Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/lib/CodeGen/CGDebugInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3518,6 +3518,10 @@ llvm::DIType *CGDebugInfo::CreateTypeDefinition(const EnumType *Ty) {

SmallVector<llvm::Metadata *, 16> Enumerators;
ED = ED->getDefinition();

if (!ED)
return nullptr;

Copy link
Member

Choose a reason for hiding this comment

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

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

for (const auto *Enum : ED->enumerators()) {
Enumerators.push_back(
DBuilder.createEnumerator(Enum->getName(), Enum->getInitVal()));
Expand Down
3 changes: 3 additions & 0 deletions clang/test/CodeGen/debug-info-enum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,6 @@ enum E8 { A8 = -128, B8 = 127 } x8;
// CHECK-NOT: DIFlagEnumClass
// CHECK: !DIEnumerator(name: "A8", value: -128)

// Forward declaration of an enum class.
enum class Color : int;
// CHECK-NOT: !DICompositeType(tag: DW_TAG_enumeration_type, name: "Color"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;")

Comment on lines +101 to +103
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Loading