mach: Support raising EXCEPTION_*_IDENTITY_PROTECTED [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 3 comments![]( "Open in Gerrit")
Justin CohenThere’s also EXCEPTION_IDENTITY_PROTECTED.
Crashpad already handles things like EXCEPTION_IDENTITY. Should it handle EXCEPTION_IDENTITY_PROTECTED too?
Mark MentovaiIs that useful for Crashpad? EXCEPTION_IDENTITY_PROTECTED does not carry thread state.
```
routine mach_exception_raise_identity_protected(
exception_port : mach_port_t;
thread_id : uint64_t;
task_id_token_t : mach_port_t;
exception : exception_type_t;
code : mach_exception_data_t
);
```
Justin CohenIs that useful for Crashpad? EXCEPTION_IDENTITY_PROTECTED does not carry thread state.
Same situation as for `EXCEPTION_IDENTITY`, which Crashpad does implement.
If receiving an identity-only behavior, Crashpad will make an extra call to look up the thread state.
It’s also valid in case Crashpad needs to forward to an exception handler with this behavior.
Acknowledged
Justin CohenI'm not sure if this CL is necessary, but it was pointed out to me that I missed updating util/mach/symbolic_constants_mach.cc with the new exception types, which led to updating the tests, and then the exc_client.
I don't think we use any of this on the iOS side of things (unless iOS changes CrashHandler::CatchMachException to Forward exceptions to original_handlers_ -- there's still a TODO there)
WDYT?
Acknowledged
return KERN_NOT_SUPPORTED;Justin CohenAdd a comment saying that `EXCEPTION_IDENTITY_PROTECTED | kMachExceptionCodes` could be viable, but isn’t implemented for (insert reason). This is in contrast to the other two, which aren‘t viable combinations.
Added support
Related details
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 7 comments![]( "Open in Gerrit")
case EXCEPTION_IDENTITY_PROTECTED:
case EXCEPTION_STATE_IDENTITY_PROTECTED:
return KERN_NOT_SUPPORTED;These would be a caller error. Let’s actually just `NOTREACHED` these explicitly (but reordered to be with the `exc` stuff, not in the middle of the `mach_exc` stuff), along with a comment explaining. Or just let them use the `default:` label without an explicit callout.
static const mach_msg_id_t
kMachMessageIDExceptionRaiseStateIdentityProtected = 2411;
static const mach_msg_id_t kMachMessageIDExceptionRaiseIdentityProtected =
2412;I’m curious where these numbers came from. These seem to correspond to the non-`MACH_EXCEPTION_CODES` protected behaviors, which don’t actually exist. They’re just made up, right? Do we need them at all?
If not, get rid of them.
If we need them as placeholders, let’s at least not have them overlap a viable namespace. They should also come with a comment explaining the situation.
using ExceptionRaiseStateIdentityProtectedRequest =
__Request__mach_exception_raise_state_identity_protected_t;
using ExceptionRaiseIdentityProtectedRequest =
__Request__mach_exception_raise_identity_protected_t;Keep the order in line with `<mach/mach_exc.defs>`, which puts identity before state_identity. Same on lines 121–124, and again with function definitions like 152–167, and in many other places in this file.
virtual kern_return_t CatchExceptionRaiseIdentityProtected(Public interface wants documentation.
2409,2411 too, since that’s one off from the last valid one now.
2508,Complete this list too, through 2511, inclusive.
basic_behavior == EXCEPTION_STATE_IDENTITY_PROTECTED;Since this isn’t defined for non-`MACH_EXCEPTION_CODES`, you should check for the correct `behavior` value, not `basic_behavior`.
Same on lines 30–31.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 7 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Commit-Queue | +1 |
7 comments
case EXCEPTION_IDENTITY_PROTECTED:
case EXCEPTION_STATE_IDENTITY_PROTECTED:
return KERN_NOT_SUPPORTED;These would be a caller error. Let’s actually just `NOTREACHED` these explicitly (but reordered to be with the `exc` stuff, not in the middle of the `mach_exc` stuff), along with a comment explaining. Or just let them use the `default:` label without an explicit callout.
Done
static const mach_msg_id_t
kMachMessageIDExceptionRaiseStateIdentityProtected = 2411;
static const mach_msg_id_t kMachMessageIDExceptionRaiseIdentityProtected =
2412;I’m curious where these numbers came from. These seem to correspond to the non-`MACH_EXCEPTION_CODES` protected behaviors, which don’t actually exist. They’re just made up, right? Do we need them at all?
If not, get rid of them.
If we need them as placeholders, let’s at least not have them overlap a viable namespace. They should also come with a comment explaining the situation.
Yup, made up, protected exceptions do not exist for the 32-bit exc subsystem.
To remove them I'd need to change the references in MachMessageServerRequestIDs. How about adding a static RequestIDs helper in both traints (in this patchset)
using ExceptionRaiseStateIdentityProtectedRequest =
__Request__mach_exception_raise_state_identity_protected_t;
using ExceptionRaiseIdentityProtectedRequest =
__Request__mach_exception_raise_identity_protected_t;Keep the order in line with `<mach/mach_exc.defs>`, which puts identity before state_identity. Same on lines 121–124, and again with function definitions like 152–167, and in many other places in this file.
Done
virtual kern_return_t CatchExceptionRaiseIdentityProtected(Justin CohenPublic interface wants documentation.
Done
2411 too, since that’s one off from the last valid one now.
Done
Complete this list too, through 2511, inclusive.
Done
Since this isn’t defined for non-`MACH_EXCEPTION_CODES`, you should check for the correct `behavior` value, not `basic_behavior`.
Same on lines 30–31.
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Code-Review | +1 |
1 comment
static const mach_msg_id_t
kMachMessageIDExceptionRaiseStateIdentityProtected = 2411;
static const mach_msg_id_t kMachMessageIDExceptionRaiseIdentityProtected =
2412;Justin CohenI’m curious where these numbers came from. These seem to correspond to the non-`MACH_EXCEPTION_CODES` protected behaviors, which don’t actually exist. They’re just made up, right? Do we need them at all?
If not, get rid of them.
If we need them as placeholders, let’s at least not have them overlap a viable namespace. They should also come with a comment explaining the situation.
Yup, made up, protected exceptions do not exist for the 32-bit exc subsystem.
To remove them I'd need to change the references in MachMessageServerRequestIDs. How about adding a static RequestIDs helper in both traints (in this patchset)
Yup, made up, protected exceptions do not exist for the 32-bit exc subsystem.
To remove them I'd need to change the references in MachMessageServerRequestIDs. How about adding a static RequestIDs helper in both traints (in this patchset)
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
crashpad-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
crashpa...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
mach: Support raising EXCEPTION_*_IDENTITY_PROTECTED
This is a follow-up to 226847ab2f143ffeb51822ed0cc39d358614c727, which
added server-side support for EXCEPTION_STATE_IDENTITY_PROTECTED.
This change implements client-side and server-side support for raising,
receiving, and forwarding both EXCEPTION_STATE_IDENTITY_PROTECTED and
EXCEPTION_IDENTITY_PROTECTED exceptions via UniversalExceptionRaise()
and UniversalMachExcServer.
This is required to allow unit testing of the protected exception paths
in exc_client_variants_test, and to support future forwarding of Mach
exceptions to original handlers in the iOS crash handler on iOS 18+.
- M util/mach/exc_client_variants.cc
- M util/mach/exc_client_variants_test.cc
- M util/mach/exc_server_variants.cc
- M util/mach/exc_server_variants_test.cc
- M util/mach/exception_behaviors.cc
- M util/mach/symbolic_constants_mach.cc
- M util/mach/symbolic_constants_mach_test.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |