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.

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

Merged
merged 1 commit into from
Jun 10, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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 •
edited
Loading

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 •
edited
Loading

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.


// Check if we have a "brk 0xc47x" trap, where the value that failed to
// authenticate is in x16.
Address current_address = current_frame->GetFrameCodeAddress();
Expand Down
Loading