Use EXCEPTION_STATE_IDENTITY_PROTECTED on iOS. [crashpad/crashpad : main]
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
This is need for using Blink on iOS. I'm not sure why PROTECTED is required by Apple for Blink on iOS, or even if the in-process design for iOS makes sense for Blink on iOS.
If it does, this appears to work, but my big open questions are:
- (1) when to use EXCEPTION_STATE_IDENTITY_PROTECTED vs EXCEPTION_STATE_IDENTITY [perhaps as a flag passed to Crashpad, or maybe it's fine to use on anyone > iOS18]
- (2) Why the PROTECTED API doesn't pass the task and thread like the non-PROTECTED version, and if it's OK to simply recover it to match the old API, like I do in this CL (util/mach/exc_server_variants.cc::469-513)
- (3) Perhaps more minor, but the difference between ExcTraits and MachExcTraits each having their own kMachMessageIDExceptionRaiseStateIdentityProtected const (with the ExcTraits version unused)
Consider this is a first draft in what perhaps should be a conversation. Let me know if doing this over CL comments is OK, or if you'd prefer to chat.
Thanks!
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 11 comments![]( "Open in Gerrit")
- 1 The `in_request` uses a `task_id_token_t` instead of providing a
send right to the excepting task's port. The handler uses
`task_identity_token_get_task_port()` to retrieve the task port from
this token.
- 2 Instead of a `in_request->thread.name` (thread port), the handler
receives a `in_request->thread_id`. This ID is used to find the
corresponding thread port by iterating through the task's threads.This list doesn’t need to be numbered.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
if (__builtin_available(iOS 18, *)) {
// It seems like EXCEPTION_STATE_IDENTITY_PROTECTED works everywhere, but I
// wonder if this would be safer as an optional flag passed to Crashpad?
state_identity = EXCEPTION_STATE_IDENTITY_PROTECTED;
}
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0For iOS (including simulator), I think it’s probably safe to use this unconditionally.
That only applies to this file, though. I’ll talk about the other files when I get there.
#include <mach/task.h>Not necessary, because `"util/mach/exc_server/variants.h"` already brings in all of `<mach/mach.h>`.
// UnusedUnused?
I think there are lots of things in this file that are potentially unused. Why call this one out?
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
using ExceptionRaiseStateIdentityProtectedRequest =
__Request__mach_exception_raise_state_identity_protected_t;
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Typedefs are safe to do unconditionally, as long as we’re sure that the types exist. We always `mig` our own recent copy of `<mach/mach_exc.defs>`, so these types should always exist. These sorts of things can be unconditional.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
static kern_return_t MIGCheckRequestExceptionRaiseStateIdentityProtected(
const ExceptionRaiseStateIdentityProtectedRequest* in_request,
const ExceptionRaiseStateIdentityProtectedRequest** in_request_1) {
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(
const_cast<ExceptionRaiseStateIdentityProtectedRequest*>(in_request),
const_cast<ExceptionRaiseStateIdentityProtectedRequest**>(
in_request_1));
}
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Same for functions like this that operate on types that you know you have access to, even if the function may not be used.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
//! \brief Handles exceptions raised by
//! `mach_exception_raise_state_identity_protected()`.
//!
//! This behaves equivalently to a
//! `catch_mach_exception_raise_state_identity_protected()` function used
//! with `mach_exc_server()`.
//!
//! \param[in] trailer The trailer received with the request message.
//! \param[out] destroy_request `true` if the request message is to be
//! destroyed even when this method returns success. See
//! MachMessageServer::Interface.
virtual kern_return_t CatchExceptionRaiseStateIdentityProtected(
exception_handler_t exception_port,
thread_t thread,
task_t task,
exception_type_t exception,
const typename Traits::ExceptionCode* code,
mach_msg_type_number_t code_count,
thread_state_flavor_t* flavor,
ConstThreadState old_state,
mach_msg_type_number_t old_state_count,
thread_state_t new_state,
mach_msg_type_number_t* new_state_count,
const mach_msg_trailer_t* trailer,
bool* destroy_request) = 0;
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Here’s where things get trickier. We may not want to define a new pure virtual interface and force other downstream things to have to implement it yet.
But: your enabling condition is more complex than it needs to be. Don’t we always build with an iOS SDK ≥ 18?
This should wind up matching whatever condition applies to the other (_ios) file, so if you drop the `#if` from that file altogether and it winds up applying to all _ios builds (device and simulator) without regard to SDK version, then this should match.
Exception to all of this: if everything gets funneled through the “Simplified” interface which you’ve taken care of implementing already, then there won’t be anything for anyone else to need to implement, and you can make this unconditional. I would prefer that, but only if it doesn’t mean any extra (unused, untested) code.
Traits::kMachMessageIDExceptionRaiseStateIdentityProtected,You would want the same condition to wrap this.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0And this.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0And this.
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0And then this file would just need to see its conditions tweaked to match what’s done in the others.
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 added 7 comments![]( "Open in Gerrit")
I think if we are only supporting Xcode 16+ everywhere then this doesn't need a conditional anywhere. WDYT? I intentionally left many of the comments unresolved since they are all the same thing -- in case this isn't what you preferred.
PTAL
- 1 The `in_request` uses a `task_id_token_t` instead of providing a
send right to the excepting task's port. The handler uses
`task_identity_token_get_task_port()` to retrieve the task port from
this token.
- 2 Instead of a `in_request->thread.name` (thread port), the handler
receives a `in_request->thread_id`. This ID is used to find the
corresponding thread port by iterating through the task's threads.This list doesn’t need to be numbered.
Done
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
if (__builtin_available(iOS 18, *)) {
// It seems like EXCEPTION_STATE_IDENTITY_PROTECTED works everywhere, but I
// wonder if this would be safer as an optional flag passed to Crashpad?
state_identity = EXCEPTION_STATE_IDENTITY_PROTECTED;
}
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0For iOS (including simulator), I think it’s probably safe to use this unconditionally.
That only applies to this file, though. I’ll talk about the other files when I get there.
Done
Not necessary, because `"util/mach/exc_server/variants.h"` already brings in all of `<mach/mach.h>`.
Done
Unused?
I think there are lots of things in this file that are potentially unused. Why call this one out?
removed
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
using ExceptionRaiseStateIdentityProtectedRequest =
__Request__mach_exception_raise_state_identity_protected_t;
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Typedefs are safe to do unconditionally, as long as we’re sure that the types exist. We always `mig` our own recent copy of `<mach/mach_exc.defs>`, so these types should always exist. These sorts of things can be unconditional.
Done
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0
static kern_return_t MIGCheckRequestExceptionRaiseStateIdentityProtected(
const ExceptionRaiseStateIdentityProtectedRequest* in_request,
const ExceptionRaiseStateIdentityProtectedRequest** in_request_1) {
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(
const_cast<ExceptionRaiseStateIdentityProtectedRequest*>(in_request),
const_cast<ExceptionRaiseStateIdentityProtectedRequest**>(
in_request_1));
}
#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Same for functions like this that operate on types that you know you have access to, even if the function may not be used.
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(Run `git cl format`.
if (kr != MACH_MSG_SUCCESS) {
// If this fails, pass TASK_NULL to
// CatchExceptionRaiseStateIdentityProtected
}This function doesn’t really log anything, but this does seem log-worthy.
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 added 7 comments![]( "Open in Gerrit")
I think if we are only supporting Xcode 16+ everywhere then this doesn't need a conditional anywhere. WDYT? I intentionally left many of the comments unresolved since they are all the same thing -- in case this isn't what you preferred.
PTAL
Done
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(Run `git cl format`.
Neither Crashpad nor Chromium `git cl format` suggests any changes here?
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0#endif // defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >=
// __IPHONE_18_0Here’s where things get trickier. We may not want to define a new pure virtual interface and force other downstream things to have to implement it yet.
But: your enabling condition is more complex than it needs to be. Don’t we always build with an iOS SDK ≥ 18?
This should wind up matching whatever condition applies to the other (_ios) file, so if you drop the `#if` from that file altogether and it winds up applying to all _ios builds (device and simulator) without regard to SDK version, then this should match.
Exception to all of this: if everything gets funneled through the “Simplified” interface which you’ve taken care of implementing already, then there won’t be anything for anyone else to need to implement, and you can make this unconditional. I would prefer that, but only if it doesn’t mean any extra (unused, untested) code.
Done
Traits::kMachMessageIDExceptionRaiseStateIdentityProtected,You would want the same condition to wrap this.
Done
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0Justin CohenAnd this.
Done
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0Justin CohenAnd this.
Done
#if defined(__IPHONE_18_0) && __IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_18_0And then this file would just need to see its conditions tweaked to match what’s done in the others.
- Mark Mentovai
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 added 2 comments![]( "Open in Gerrit")
if (kr != MACH_MSG_SUCCESS) {
// If this fails, pass TASK_NULL to
// CatchExceptionRaiseStateIdentityProtected
}This function doesn’t really log anything, but this does seem log-worthy.
Added MACH_LOG(WARNING and CRASHPAD_RAW_LOG_ERROR for in-process-safe iOS
Related details
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(Justin CohenRun `git cl format`.
Neither Crashpad nor Chromium `git cl format` suggests any changes here?
Neither Crashpad nor Chromium `git cl format` suggests any changes here?
If you manually format as, say:
```cpp
return
__MIG_check__Request__mach_exception_raise_state_identity_protected_t(
const_cast<ExceptionRaiseStateIdentityProtectedRequest*>(
in_request),
const_cast<ExceptionRaiseStateIdentityProtectedRequest**>(
in_request_1));
```
does `clang-format` leave that alone, or does it insist on rewrapping to this thing that goes beyond 80 columns?
MACH_LOG(WARNING, kr) << "task_identity_token_get_task_port";Missing an `#include` for me.
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. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
#include "util/ios/raw_logging.h"You should only `#include` this when targeting iOS.
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 added 3 comments![]( "Open in Gerrit")
You should only `#include` this when targeting iOS.
Done
return __MIG_check__Request__mach_exception_raise_state_identity_protected_t(Justin CohenRun `git cl format`.
Mark MentovaiNeither Crashpad nor Chromium `git cl format` suggests any changes here?
Neither Crashpad nor Chromium `git cl format` suggests any changes here?
If you manually format as, say:
```cpp
return
__MIG_check__Request__mach_exception_raise_state_identity_protected_t(
const_cast<ExceptionRaiseStateIdentityProtectedRequest*>(
in_request),
const_cast<ExceptionRaiseStateIdentityProtectedRequest**>(
in_request_1));
```does `clang-format` leave that alone, or does it insist on rewrapping to this thing that goes beyond 80 columns?
I'll file a clang-format bug. I will force it with the manual format.
MACH_LOG(WARNING, kr) << "task_identity_token_get_task_port";Missing an `#include` for me.
- 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 Code-Review+1![]( "Open in Gerrit")
- 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 LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Use EXCEPTION_STATE_IDENTITY_PROTECTED on iOS.
This CL adds support for `EXCEPTION_STATE_IDENTITY_PROTECTED` Mach
exceptions and enables it for the iOS crash handler.
This exception behavior is a variant of `EXCEPTION_STATE_IDENTITY`. This
change is required when using certain iOS entitlements
(com.apple.developer.web-browser-engine.host and
com.apple.security.hardened-process.platform-restrictions).
The primary differences from `EXCEPTION_STATE_IDENTITY` are:
- The `in_request` uses a `task_id_token_t` instead of providing a
send right to the excepting task's port. The handler uses
`task_identity_token_get_task_port()` to retrieve the task port from
this token.
- Instead of a `in_request->thread.name` (thread port), the handler
receives a `in_request->thread_id`. This ID is used to find the
corresponding thread port by iterating through the task's threads.
- Implemented the server-side logic for the new exception type in
`exc_server_variants.cc`.
- Updated `crash_handler_ios.cc` to install the protected handler.
- Added unit tests for the new server logic.
- M client/crash_handler_ios.cc
- M util/BUILD.gn
- M util/mach/exc_server_variants.cc
- M util/mach/exc_server_variants_test.cc
Code-Review: +1 by Mark Mentovai
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |