Skip to content

[lldb] Tighten ABI assert in StopInfoMachException::DeterminePtrauthFailure (NFC) #95015

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

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

medismailben
Copy link
Member

This patch tightens the assert check for the ABISP object in StopInfoMachException::DeterminePtrauthFailure.

This causes some failure when debugging on a system that doesn't have pointer authentification support, like on Intel for instance.

rdar://129401926

…ailure (NFC)

This patch tightens the assert check for the ABISP object in
`StopInfoMachException::DeterminePtrauthFailure`.

This causes some failure when debugging on a system that doesn't have
pointer authentification support, like on Intel for instance.

rdar://129401926

Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
@llvmbot
Copy link
Member

llvmbot commented Jun 10, 2024

@llvm/pr-subscribers-lldb

Author: Med Ismail Bennani (medismailben)

Changes

This patch tightens the assert check for the ABISP object in StopInfoMachException::DeterminePtrauthFailure.

This causes some failure when debugging on a system that doesn't have pointer authentification support, like on Intel for instance.

rdar://129401926


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

1 Files Affected:

  • (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+3-2)
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 75504323b4fdf..25cee369d7ee3 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -92,9 +92,7 @@ bool StopInfoMachException::DeterminePtrauthFailure(ExecutionContext &exe_ctx) {
 
   Target &target = *exe_ctx.GetTargetPtr();
   Process &process = *exe_ctx.GetProcessPtr();
-  ABISP abi_sp = process.GetABI();
   const ArchSpec &arch = target.GetArchitecture();
-  assert(abi_sp && "Missing ABI info");
 
   // Check for a ptrauth-enabled target.
   const bool ptrauth_enabled_target =
@@ -110,6 +108,9 @@ bool StopInfoMachException::DeterminePtrauthFailure(ExecutionContext &exe_ctx) {
     strm.Printf("Note: Possible pointer authentication failure detected.\n");
   };
 
+  ABISP abi_sp = process.GetABI();
+  assert(abi_sp && "Missing ABI info");
+
   // Check if we have a "brk 0xc47x" trap, where the value that failed to
   // authenticate is in x16.
   Address current_address = current_frame->GetFrameCodeAddress();

@medismailben medismailben merged commit 93b91dd into llvm:main Jun 10, 2024
6 of 7 checks passed
medismailben added a commit to medismailben/llvm-project that referenced this pull request Jun 10, 2024
…Failure` (NFC) (llvm#95015)

This patch tightens the assert check for the ABISP object in
`StopInfoMachException::DeterminePtrauthFailure`.

This causes some failure when debugging on a system that doesn't have
pointer authentification support, like on Intel for instance.

rdar://129401926

Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
(cherry picked from commit 93b91dd)
@jasonmolenda
Copy link
Collaborator

this is fine but fwiw there are Process::FixAnyAddress, Process::FixDataAddress, Process::FixCodeAddress methods that are the preferred way of clearing pointerauth bits and they fall back to returning the value unmodified if there is no ABI.

@@ -110,6 +108,9 @@ bool StopInfoMachException::DeterminePtrauthFailure(ExecutionContext &exe_ctx) {
strm.Printf("Note: Possible pointer authentication failure detected.\n");
};

ABISP abi_sp = process.GetABI();
assert(abi_sp && "Missing ABI info");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to crash the debugger if there is no abi plug-in? I would like us to not crash by reporting an error to the debug console and being able to let the debugger live and just check abi_sp before we use it.

Copy link
Member Author

@medismailben medismailben Jun 11, 2024

Choose a reason for hiding this comment

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

@clayborg Would you be fine with that ?

Suggested change
assert(abi_sp && "Missing ABI info");
if(!abi_sp) {
// log missing ABI info
return false;
}

@jasonmolenda would we still need to call FixAnyAddress here ?

Copy link
Collaborator

@jasonmolenda jasonmolenda Jun 11, 2024

Choose a reason for hiding this comment

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

tbh I'm not entirely sure why we're here at all for an x86_64 target? this method is only be called when the target cpu is llvm::Triple::aarch64. It fetches the value of the x16 register just before this, and returns if no such register is found I think? But yes, I would change these to not get the ABI/assert. Instead, call Process::FixCodeAddress at the point we're clearing the non-addressable bits. Process::FixCodeAddress will silently return the same value if there is no ABI (and if it gets an ABI without a FixCodeAddress override method, it will also return the same value).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, FWIW, this is happening with crashlog command when loading an arm64 target on an intel machine. That could explain why we're seeing this behavior.

Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Jun 12, 2024
…Failure` (NFC) (llvm#95015)

This patch tightens the assert check for the ABISP object in
`StopInfoMachException::DeterminePtrauthFailure`.

This causes some failure when debugging on a system that doesn't have
pointer authentification support, like on Intel for instance.

rdar://129401926

Signed-off-by: Med Ismail Bennani <ismail@bennani.ma>
@HerrCai0907 HerrCai0907 mentioned this pull request Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants