Skip to content
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

[compiler-rt][ubsan][nfc-ish] Fix a type conversion bug #100665

Merged
merged 4 commits into from
Jul 26, 2024

Conversation

alanzhao1
Copy link
Contributor

@alanzhao1 alanzhao1 commented Jul 25, 2024

If the inline asm version of ptrauth_strip is used instead of the builtin, the inline asm implementation will return an unsigned long, causing an incompatible pointer conversion issue.

With llvm#100483, if the inline asm
version of `ptrauth_strip` is used instead of the builtin, the inline
asm implementation will return an unsigned long, causing an incompatible
pointer conversion issue.
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 25, 2024

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: Alan Zhao (alanzhao1)

Changes

With #100483, if the inline asm version of ptrauth_strip is used instead of the builtin, the inline asm implementation will return an unsigned long, causing an incompatible pointer conversion issue.


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

1 Files Affected:

  • (modified) compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp (+2-1)
diff --git a/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp b/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp
index 15788574dd995..7cc57268d40da 100644
--- a/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp
+++ b/compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp
@@ -207,7 +207,8 @@ struct VtablePrefix {
   std::type_info *TypeInfo;
 };
 VtablePrefix *getVtablePrefix(void *Vtable) {
-  Vtable = ptrauth_strip(Vtable, ptrauth_key_cxx_vtable_pointer);
+  Vtable = reinterpret_cast<void *>(
+      ptrauth_strip(Vtable, ptrauth_key_cxx_vtable_pointer));
   VtablePrefix *Vptr = reinterpret_cast<VtablePrefix*>(Vtable);
   VtablePrefix *Prefix = Vptr - 1;
   if (!IsAccessibleMemoryRange((uptr)Prefix, sizeof(VtablePrefix)))

@asl
Copy link
Collaborator

asl commented Jul 25, 2024

With #100483, if the inline asm version of ptrauth_strip is used instead of the builtin, the inline asm implementation will return an unsigned long, causing an incompatible pointer conversion issue.

To be more precise, it was inline asm version before that PR. It was just an implicit cast due to function call result.

Copy link
Collaborator

@asl asl left a comment

Choose a reason for hiding this comment

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

Provided that downstream builds pass. AFAIK there are no public builtbots that checks pac-ret configuration.

Thanks!

@alanzhao1
Copy link
Contributor Author

With #100483, if the inline asm version of ptrauth_strip is used instead of the builtin, the inline asm implementation will return an unsigned long, causing an incompatible pointer conversion issue.

To be more precise, it was inline asm version before that PR. It was just an implicit cast due to function call result.

Updated the PR description.

@asl
Copy link
Collaborator

asl commented Jul 25, 2024

@alanzhao1 Actually, the spec for ptrauth_sign is:

   The result will have the same type as the original value.

So, I think we'd also fix the inline asm version while here. How about the following:

      __typeof(__value) ret;                \

?

@alanzhao1
Copy link
Contributor Author

@alanzhao1 Actually, the spec for ptrauth_sign is:

   The result will have the same type as the original value.

So, I think we'd also fix the inline asm version while here. How about the following:

      __typeof(__value) ret;                \

?

Done

@alanzhao1 alanzhao1 merged commit 25f9415 into llvm:main Jul 26, 2024
6 checks passed
@alanzhao1 alanzhao1 deleted the bugfix/compiler-rt-cast branch July 26, 2024 00:39
@asl
Copy link
Collaborator

asl commented Jul 26, 2024

/cherry-pick 25f9415

@asl asl added this to the LLVM 19.X Release milestone Jul 26, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 26, 2024

Failed to cherry-pick: 25f9415

https://github.com/llvm/llvm-project/actions/runs/10103888499

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

@asl
Copy link
Collaborator

asl commented Jul 26, 2024

@tru This will apply cleanly after #100634

@asl asl removed this from the LLVM 19.X Release milestone Jul 26, 2024
@asl
Copy link
Collaborator

asl commented Jul 26, 2024

/cherry-pick 25f9415

@asl asl added this to the LLVM 19.X Release milestone Jul 26, 2024
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 26, 2024
If the inline asm version of `ptrauth_strip` is used instead of the
builtin, the inline asm implementation currently returns an unsigned
long, causing an incompatible pointer conversion issue. The spec for
`ptrauth_sign` is that the result has the same type as the original
value, so we add a cast to the result of the inline asm.

(cherry picked from commit 25f9415)
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 26, 2024

/pull-request #100713

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Jul 26, 2024
If the inline asm version of `ptrauth_strip` is used instead of the
builtin, the inline asm implementation currently returns an unsigned
long, causing an incompatible pointer conversion issue. The spec for
`ptrauth_sign` is that the result has the same type as the original
value, so we add a cast to the result of the inline asm.

(cherry picked from commit 25f9415)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants