Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
9 changes: 5 additions & 4 deletions clang/include/clang/AST/ASTContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,11 @@ class ASTContext : public RefCountedBase<ASTContext> {
return findPointerAuthContent(T) != PointerAuthContent::None;
}

// A simple helper function to short circuit pointer auth checks.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

bool isPointerAuthenticationAvailable() const {
return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
}

private:
llvm::DenseMap<const CXXRecordDecl *, CXXRecordDeclRelocationInfo>
RelocatableClasses;
Expand All @@ -670,10 +675,6 @@ class ASTContext : public RefCountedBase<ASTContext> {
AddressDiscriminatedData
};

// A simple helper function to short circuit pointer auth checks.
bool isPointerAuthenticationAvailable() const {
return LangOpts.PointerAuthCalls || LangOpts.PointerAuthIntrinsics;
}
PointerAuthContent findPointerAuthContent(QualType T) const;
mutable llvm::DenseMap<const RecordDecl *, PointerAuthContent>
RecordContainsAddressDiscriminatedPointerAuth;
Expand Down
5 changes: 5 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,11 @@ def err_ptrauth_address_discrimination_invalid : Error<
def err_ptrauth_extra_discriminator_invalid : Error<
"invalid extra discriminator flag '%0'; '__ptrauth' requires a value between "
"'0' and '%1'">;
def warn_ptrauth_weak_schema
Copy link
Contributor

Choose a reason for hiding this comment

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

@efriedma-quic would it be reasonable to have a note suggesting the use of the __ptrauth qualifier?

: Warning<"%0 has internal linkage with a %select{|default }1"
"pointer authentication schema that should be overridden by "
"%select{a|an explicit}1 schema with unique diversifiers">,
InGroup<DiagGroup<"ptrauth-weak-schema">>;

/// main()
// static main() is not an error in C, just in C++.
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/Sema/SemaDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8357,6 +8357,16 @@ NamedDecl *Sema::ActOnVariableDeclarator(
D.isFunctionDefinition());
}

// Warn about the use of a weak pointer authentication schema on a variable
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be pulled into a separate function - we'll want to use this for other declaration types over time, and it will likely become more complex as additional rules occur. Something like

bool Sema::DiagnoseWeakPointerAuthenticationSchema(NamedDecl *ND) { ... }

It's possible we'll need other information over time, but I think for now that's sufficient.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, but I have a question. My understanding is that you are thinking of moving both the condition we check and the Diag call into a separate function. Considering that no action such as invalidating the declaration would be taken on the caller side -at least for now-, shouldn't the return type of the separate function be void (similar to void Sema::DiagnoseUniqueObjectDuplication(const VarDecl *) or void Sema::DiagnoseAutoDeductionFailure(const VarDecl *, const Expr *)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah - I think that's an easier interface to use, otherwise all the code ends up having to do:

if (ShouldDiagnoseBlah(...))
   DiagnoseBlah(...); // If you construct the diagnostic inline then as the checks become
                         // richer every call site has to work out the full diagnostic message

And yes, the result should be void - I was thinking in terms of Sema checks that can fail the diagnostics due to the error breaking further tests which is not the case here: warnings by definition don't cause this problem.

Copy link
Author

Choose a reason for hiding this comment

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

One more thing related to the separate function. The class NamedDecl does not contain the DeclType member which we use for the check. I will propose for the parameter type to be ValueDecl but shouldn't we go straight with VarDecl instead? I could not find a case in which the function will receive an EnumConstantDecl (the other subclass of ValueDecl ), and the only caller passes an argument of (static) VarDecl type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's start with vardecl for now, we'll probably need to change to nameddecl in future and add many casts which will be sad - but we can cross that bridge when we get there.

// with internal linkage.
if (Context.isPointerAuthenticationAvailable() &&
NewVD->isFunctionPointerType() && !NewVD->isExternallyVisible()) {
PointerAuthQualifier Q = NewVD->getType().getQualifiers().getPointerAuth();
if (!Q || (!Q.isAddressDiscriminated() && Q.getExtraDiscriminator() == 0)) {
Diag(NewVD->getLocation(), diag::warn_ptrauth_weak_schema) << NewVD << !Q;
}
}

if (NewTemplate) {
if (NewVD->isInvalidDecl())
NewTemplate->setInvalidDecl();
Expand Down
60 changes: 60 additions & 0 deletions clang/test/Sema/ptrauth-weak-schema.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// RUN: %clang_cc1 -triple arm64e-apple-ios -fptrauth-calls -fptrauth-intrinsics -fsyntax-only -Wno-unused-variable -verify %s
// RUN: %clang_cc1 -triple arm64e-apple-ios -DNO_PTRAUTH -fsyntax-only -Wno-unused-variable -verify=noptrauth %s

// noptrauth-no-diagnostics

#include <ptrauth.h>

#if defined(__PTRAUTH__) == defined(NO_PTRAUTH)
#error expected pointer authentication state does not match actual
#endif

#if defined(NO_PTRAUTH)
#define FN_PTR_AUTH(address_diversity, constant_discriminator)
#else
#define FN_PTR_AUTH(address_diversity, constant_discriminator) \
__ptrauth(ptrauth_key_function_pointer, address_diversity, constant_discriminator)
#endif

// Global variables with external linkage and weak pointer authentication should
// not raise any warning.
extern void(* g1_external_weak)(void);
void(* FN_PTR_AUTH(0, 0) g2_external_weak)(void);

// Global variables with internal linkage and strong pointer authentication
// should not raise any warning.
static void(* FN_PTR_AUTH(1, 65535) g1_internal_strong)(void);
static void(* FN_PTR_AUTH(0, 65535) g2_internal_strong)(void);
static void(* FN_PTR_AUTH(1, 0) g3_internal_strong)(void);

#if !defined(NO_PTRAUTH)
// Global variables with internal linkage and weak pointer authentication should
// raise a warning.
static void(* g1_internal_weak)(void);
// expected-warning@-1 {{'g1_internal_weak' has internal linkage with a default pointer authentication schema that should be overridden by an explicit schema with unique diversifiers}}
static void(* FN_PTR_AUTH(0, 0) g2_internal_weak)(void);
// expected-warning@-1 {{'g2_internal_weak' has internal linkage with a pointer authentication schema that should be overridden by a schema with unique diversifiers}}

// Assert that -Wptrauth-weak-schema silences warnings.
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wptrauth-weak-schema"
static void(* g3_internal_weak)(void);
#pragma clang diagnostic pop
#endif

void test_local_variables(void) {
#if !defined(NO_PTRAUTH)
Copy link
Contributor

Choose a reason for hiding this comment

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

you should remove this ifdef, instead you can make each test use a different prefix. Due to the wildly different amount of tests, I'd just override for the non-ptrauth case, which you can do with -verify=noptrauth and then replace // expected-no-diagnostics with // noptrauth-no-diagnostics

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good

// Local variables (internal linkage) with weak pointer authentication
// should raise a warning.
static void(* l1_internal_weak)(void);
// expected-warning@-1 {{'l1_internal_weak' has internal linkage with a default pointer authentication schema that should be overridden by an explicit schema with unique diversifiers}}
static void(* FN_PTR_AUTH(0, 0) l2_internal_weak)(void);
// expected-warning@-1 {{'l2_internal_weak' has internal linkage with a pointer authentication schema that should be overridden by a schema with unique diversifiers}}
#endif

// Local variables (internal linkage) with strong pointer authentication
// should not raise any warning.
void(* FN_PTR_AUTH(1, 65535) l1_internal_strong)(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

l{1|2}_internal_weak has static storage duration, while l{1|2|3}_internal_strong has scoped storage duration. Is this intended?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @kovdan01 for having a look at this PR. That was an unfortunate coincidence as it should not be relevant. I added the missing combinations to the test to make sure that they work as expected.

void(* FN_PTR_AUTH(0, 65535) l2_internal_strong)(void);
void(* FN_PTR_AUTH(1, 0) l3_internal_strong)(void);
}
Loading