[PAC][clang] Correct handling of ptrauth queries of incomplete types

[ojhunt]

In normal circumstances we can never get to this point as earlier Sema checks will have already have prevented us from making these queries. However in some cases, for example a sufficiently large number of errors, clang can start allowing incomplete types in records.

This means a number of the internal interfaces can end up perform type trait queries that require querying the pointer authentication properties of types that contain incomplete types. While the trait queries attempt to guard against incomplete types, those tests fail in this case as the incomplete types are actually nested in the seemingly complete parent type.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)

Changes

In normal circumstances we can never get to this point as earlier Sema checks will have already have prevented us from making these queries. However in some cases, for example a sufficiently large number of errors, clang can start allowing incomplete types in records.

This means a number of the internal interfaces can end up perform type trait queries that require querying the pointer authentication properties of types that contain incomplete types. While the trait queries attempt to guard against incomplete types, those tests fail in this case as the incomplete types are actually nested in the seemingly complete parent type.

---

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

2 Files Affected:

• (modified) clang/lib/AST/ASTContext.cpp (+23-2)
• (added) clang/test/SemaCXX/ptrauth-nested-incomplete-types.cpp (+39)

diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp
index 32c8f6209a693..485082590f079 100644
--- a/clang/lib/AST/ASTContext.cpp
+++ b/clang/lib/AST/ASTContext.cpp
@@ -1618,8 +1618,26 @@ void ASTContext::setRelocationInfoForCXXRecord(
   RelocatableClasses.insert({D, Info});
 }
 
+// In future we may want to distinguish the presence or absence of address
+// discrimination, from the inability to determine the presence. For now we rely
+// on all source facing interfaces (type trait queries, etc) diagnosing and
+// reporting an error before reaching these paths.
+static bool canDeterminePointerAuthContent(QualType Type) {
+  if (Type->isIncompleteType() || Type->isDependentType())
+    return false;
+  const TagDecl *Decl = Type->getAsTagDecl();
+  return !Decl || !Decl->getDefinition()->isInvalidDecl();
+}
+static bool canDeterminePointerAuthContent(const ASTContext& Ctx,
+                                           const TagDecl *Decl) {
+  CanQualType DeclType = Ctx.getCanonicalTagType(Decl);
+  return canDeterminePointerAuthContent(DeclType);
+}
+
 static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
     const ASTContext &Context, const CXXRecordDecl *Class) {
+  if (!canDeterminePointerAuthContent(Context, Class))
+    return false;
   if (!Class->isPolymorphic())
     return false;
   const CXXRecordDecl *BaseType = Context.baseForVTableAuthentication(Class);
@@ -1639,7 +1657,7 @@ ASTContext::findPointerAuthContent(QualType T) const {
   assert(isPointerAuthenticationAvailable());
 
   T = T.getCanonicalType();
-  if (T->isDependentType())
+  if (!canDeterminePointerAuthContent(T))
     return PointerAuthContent::None;
 
   if (T.hasAddressDiscriminatedPointerAuth())
@@ -3230,6 +3248,7 @@ QualType ASTContext::removeAddrSpaceQualType(QualType T) const {
 
 uint16_t
 ASTContext::getPointerAuthVTablePointerDiscriminator(const CXXRecordDecl *RD) {
+  assert(canDeterminePointerAuthContent(*this, RD));
   assert(RD->isPolymorphic() &&
          "Attempted to get vtable pointer discriminator on a monomorphic type");
   std::unique_ptr<MangleContext> MC(createMangleContext());
@@ -3517,7 +3536,9 @@ static void encodeTypeForFunctionPointerAuth(const ASTContext &Ctx,
 uint16_t ASTContext::getPointerAuthTypeDiscriminator(QualType T) {
   assert(!T->isDependentType() &&
          "cannot compute type discriminator of a dependent type");
-
+  assert(canDeterminePointerAuthContent(T) && 
+         "cannot compute type discriminator of an incomplete or otherwise " \
+         "invalid type");
   SmallString<256> Str;
   llvm::raw_svector_ostream Out(Str);
 
diff --git a/clang/test/SemaCXX/ptrauth-nested-incomplete-types.cpp b/clang/test/SemaCXX/ptrauth-nested-incomplete-types.cpp
new file mode 100644
index 0000000000000..233d365288d8c
--- /dev/null
+++ b/clang/test/SemaCXX/ptrauth-nested-incomplete-types.cpp
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -fptrauth-intrinsics -fsyntax-only -ferror-limit 1 -verify -std=c++26 %s
+// RUN: %clang_cc1 -fptrauth-intrinsics -fsyntax-only -ferror-limit 1 -verify -std=c++03 %s
+// RUN: %clang_cc1                      -fsyntax-only -ferror-limit 1 -verify -std=c++03 %s
+
+/// Force two errors so we hit the error limit leading to skip of template instantiation
+# "" // expected-error {{invalid preprocessing directive}}
+# ""
+// expected-error@* {{too many errors emitted}}
+
+template <typename>
+struct a {};
+struct test_polymorphic {
+  virtual ~test_polymorphic();
+  a<int> field;
+};
+static_assert(__is_trivially_relocatable(test_polymorphic));
+
+struct test_struct {
+  test_struct(int) {}
+  void test_instantiate() {
+    test_struct d(0);
+  }
+  void test_type_trait_query() {
+    __is_trivially_relocatable(test_struct);
+  }
+  a<int> e;
+};
+
+struct test_subclass : test_struct {
+   test_subclass() : test_struct(0) {
+   }
+
+   void test_subclass_instantiation() {
+    test_subclass subclass{};
+   }
+  void test_subclass_type_trait_query() {
+    __is_trivially_relocatable(test_subclass);
+  }
+};

[ojhunt]

I believe this is a more robust fix for #164505 and I've added a few additional tests for other paths that fail.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ojhunt]

My innate ability to forget to run clang-format strikes again

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
View the diff from clang-format here.

[ojhunt]

@cor3ntin is there a better way to do this? It seems like there should be

[mizvekov]

Is it guaranteed at this point to have already required this type to be complete?

Otherwise, this will not make sense for template specializations, since they can be lazily instantiated.

[ojhunt]

@mizvekov do you mean in a future "the answer is {yes, no, unknown}" world?

You can only hit this in the case where we have reached an error state that has meant we have a type that claims to be complete but contains an incomplete type (by value, not a pointer/reference).

In the case of template types, we already ignore template decls and don't produce true results until the point of instantiation.

I spent a bunch of time trying to find a state where I could create a "complete" type, containing an incomplete member, but it seems to only be achievable via the original report: if you exceed the max errors limit we stop instantiation of templated types inside classes.

What this code is dealing with is

// many many many errors

template <class> struct SomeCorrectTemplatedType {};

struct SomeOtherStruct {
   SomeCorrectTemplatedType<int> field;
};

// perform some type trait query
__is_trivially_relocatable(SomeOtherStruct);

Clang constructs SomeOtherStruct, and includes the CorrectTemplatedType<int> field, but does not actually synthesize CorrectTemplatedType<int>, leaving it as an incomplete type, despite being a value typed field of SomeOtherStruct. The various type trait queries generally get culled early on by either excluding incomplete types - which SomeOtherStruct is not - or querying bits from the type definition, which is present (that's why we believe the type is complete). When we try to compute the pointer auth properties we can't rely on precomputed bits in the DefinitionData and similar, so we have to duplicate the isCompleteDefinition check that is performed when adding a field to the type originally in our own code paths.

If I could find a way to get down this code path without requiring the "too many errors" mechanism for prevent template instantiation.

[cor3ntin]

maybe there is a bug in Sema::RequireCompleteType

 static bool primaryBaseHaseAddressDiscriminatedVTableAuthentication(
     const ASTContext &Context, const CXXRecordDecl *Class) {
-  if (!Class->isPolymorphic())
+  if (Class->isInvalidDecl() || !Class->isPolymorphic())
     return false;
   const CXXRecordDecl *BaseType = Context.baseForVTableAuthentication(Class);
   using AuthAttr = VTablePointerAuthenticationAttr;
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 280b3c92cce1..9c9a071abf18 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9231,7 +9231,7 @@ bool Sema::RequireCompleteType(SourceLocation Loc, QualType T,
     TD->setCompleteDefinitionRequired();
     Consumer.HandleTagDeclRequiredDefinition(TD);
   }
-  return false;
+  return T->isIncompleteType();
 }
 
 bool Sema::hasStructuralCompatLayout(Decl *D, Decl *Suggested) {

Try to see if that change helps.

[ojhunt]

Ah ha, that does sound like it might be a better option :D

Assuming this works I will be much happier - I was certain that there must be a centralized location to test this.

I'm also convinced that there must be some other path that ends up assuming a complete type cannot include an incomplete one but I haven't been able to find one - my only reason for wanting such a path is to verify that if it does exist, this would fix it.

[cor3ntin]

Nah, this does not work at all!
But I think there is probably a bug in RequireCompleteTypeImpl or something nearby

[ojhunt]

Does it break existing tests, or simply not fix this issue?

[ojhunt]

@AaronBallman do you have any idea how I might be able to get a field with an invalid or incomplete type without causing the containing record to be marked incomplete or invalid?

[mizvekov]

Did you try modifying isIncompleteType so it doesn't consider a Tag with invalid definition as complete?

That would give us more data to answer if this a good approach.

[ojhunt]

I was worried about making more widespread changes, but I'll give that a shot - certainly it seems reasonable to consider invalid as being incomplete.

I had a comment for Corentin asking if there's an existing function that basically answers the "is this a complete, valid, and fully instantiated type" (rather than these local helpers), but I'm not sure if such a query has any value beyond this specific case. Certainly if we were to use some bits in the type/decl and evaluate this logic eagerly in future this logic disappears and the query would become unnecessary so if there's no other need for such a general there would be no reason to add it globally.

Anyway, that last paragraph is independent of isIncomplete also testing for isInvalid. I have a suspicion that we may get some issues with diagnostics - at least the diagnostic messages themselves changing, I can't imagine it impacting actual correctness, but lets find out :D

[mizvekov]

If the isIncompleteType change has larger consequences and ends up being a positive, we can tease it out into another PR.

That would certainly be less arbitrary than if we did that just for this specific case.

[ojhunt]

Agreed, it will just take time to run all the tests (and I have a few other builds running atm), will hopefully get to testing that in the next hour or so

[ojhunt]

@mizvekov Changing the behavior of isIncompleteType, or changing RequireCompleteType as Corentin tried break all kinds of things everywhere. A lot of it seems to be diagnostics (significant regressions), but there are so many failures I can't be sure there are also actual correctness errors.

[ojhunt]

Ok, so after some investigation, when we construct the invalid record, we do mark it as being invalid, so we can just test for isInvalidDecl() when we encounter a record, and that test is transitive over the nested types in the record.