Fix alignment issues when running under UBSan [crashpad/crashpad : main]
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
CQ is kinda red, but judging by https://ci.chromium.org/p/crashpad/g/main/console I think that's pre-existing?
Related details
- Mark Mentovai
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 8 comments![]( "Open in Gerrit")
// `old_state` is not sufficiently aligned, so make a copy first.Why is this only needed for the test?
using ExceptionCodes = decltype(std::declval<Request>().code);Not used beyond this class, should be private (although it means you’ll be private-public-private, but I think that’s OK).
static constexpr size_t kMaxCodes = std::extent_v<ExceptionCodes>;Should be private too.
AlignedExceptionCodes(const Request& request) : size_(request.codeCnt) {The constructor should be `explicit`.
AlignedExceptionCodes(const Request& request) : size_(request.codeCnt) {Since all callers hold a `Request*`, why not accept a `const Request*` here, and avoid the need for callers to use the awkward `*` dereference?
ExceptionCode codes_[kMaxCodes] = {};
mach_msg_type_number_t size_ = 0;You don’t need initializers for either of these.
codes.data(),
codes.size(),Alternatively, we could redefine these interfaces to pass a `const std::vector<mach_exception_data_type_t>&`.
Related details
- David Benjamin
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Benjamin
| Commit-Queue | +1 |
2 comments
David BenjaminBah, sorry, it seems something was out of sync between my two repos and this doesn't build. Removing you from the attention set while I figure out what's going on.
Okay, hopefully that's OK now.
codes.size()),I believe this is what the old code was doing, but I'm pretty sure it's wrong. Thoughts?
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. |
David Benjamin (Gerrit)
David Benjamin added 1 comment![]( "Open in Gerrit")
Bah, sorry, it seems something was out of sync between my two repos and this doesn't build. Removing you from the attention set while I figure out what's going on.
- David Benjamin
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Benjamin (Gerrit)
David Benjamin voted and added 11 comments![]( "Open in Gerrit")
Votes added by David Benjamin
| Commit-Queue | +1 |
11 comments
Whoops, I lost track of this CL and only got back to it now. You probably want to review it with fresh eyes.
// `old_state` is not sufficiently aligned, so make a copy first.Why is this only needed for the test?
Apparently it isn't, but only gwp_asan tests flagged this. Fixed in both.
using ExceptionCodes = decltype(std::declval<Request>().code);Not used beyond this class, should be private (although it means you’ll be private-public-private, but I think that’s OK).
Done
static constexpr size_t kMaxCodes = std::extent_v<ExceptionCodes>;David BenjaminShould be private too.
Done
AlignedExceptionCodes(const Request& request) : size_(request.codeCnt) {The constructor should be `explicit`.
Done
AlignedExceptionCodes(const Request& request) : size_(request.codeCnt) {Since all callers hold a `Request*`, why not accept a `const Request*` here, and avoid the need for callers to use the awkward `*` dereference?
Done
CHECK(size_ <= kMaxCodes);David Benjamin`CHECK_LE`
Done
ExceptionCode codes_[kMaxCodes] = {};
mach_msg_type_number_t size_ = 0;You don’t need initializers for either of these.
Done
Alternatively, we could redefine these interfaces to pass a `const std::vector<mach_exception_data_type_t>&`.
Done, but with spans. Also made the funny class just be a helper function that returns a `std::vector`.
codes.size()),I believe this is what the old code was doing, but I'm pretty sure it's wrong. Thoughts?
// span.Happy to do this in this CL if you'd like, but that would spread into many more files.
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 9 comments![]( "Open in Gerrit")
Header other_header;
memcpy(&other_header, buffer, sizeof(other_header));
if (other_header.magic != kMagic || other_header.version != kVersion) {
return false;
}
header.data_range = other_header.data_range;Do we need `other_header` or can we just do this all in-place with a `memcpy` into the `header` that we already have?
If it’s important for `header` to be untouched if `magic` or `verson` are wrong, then I guess `other_header` is necessary.
Still, I think I’d have written this with `bit_cast`s and the single `header` instead of holding a `memcpy` directly.
auto state =
CopyUnalignedState<x86_thread_state>(old_state, old_state_count);
switch (state.tsh.flavor) {1. No good reason to use `auto` here. Please put the proper type back.
2. Can these be `bit_cast`s too? That’s the standard way we have to handle this concern. `CopyUnalignedState` just seems to reinvent a specialized version of that.
namespace {
// The thread state passed to these functions is not necessarily sufficiently
// aligned for the destination struct. This helper copies it out before
// accessing the fields.
template <typename State>
State CopyUnalignedState(ConstThreadState state,
mach_msg_type_number_t state_count) {
CHECK_LE(sizeof(State), state_count * sizeof(*state));
State ret;
memcpy(&ret, state, sizeof(ret));
return ret;
}
} // namespaceIf you find that you do need to introduce a helper, don’t do it once in each file that you need. Do it once in a more central location (util/mac?) and access that from where both client and snapshot.
const auto x86_thread_state =
CopyUnalignedState<x86_thread_state_t>(state, state_count);
if (x86_thread_state.tsh.flavor != x86_THREAD_STATE32) {Same.
auto CopyUnalignedExceptionCodes(const Request* request) {Too much `auto`. These types aren’t so complex that you need to fall back on that. I don’t think you should have any `auto` in this change at all.
std::vector<ExceptionCode> ret;If we’re in full control of all of the code in here, this is the only place that what underlies the `span`s are ever made, and (per this line) we only ever make `vector`s, then why wouldn’t we just pass `vector&`s around instead of the `span`s?
Crashpad code doesn’t really use `span`. I see one use of it total right now, and it can probably be removed, which might cut down on Crashpad’s size.
// When using `MachExcTraits`, `ExceptionCode` is a
// `mach_exception_data_type_t`, which is `int64_t`. Many of the request structs
// use `pack(4)`, so the `code` array may not be sufficiently aligned. This
// trips UBSan when taking a pointer to array. This utility function makes an
// aligned copy first.
template <typename Request>
auto CopyUnalignedExceptionCodes(const Request* request) {
using ExceptionCode =
std::remove_extent_t<decltype(std::declval<Request>().code)>;
std::vector<ExceptionCode> ret;
ret.reserve(request->codeCnt);
for (mach_msg_type_number_t i = 0; i < request->codeCnt; i++) {
// `request->code[i]` cannot be passed directly to `push_back`. `push_back`
// takes its input as a reference, which is enough to construct an unaligned
// pointer.
ExceptionCode code = request->code[i];
ret.push_back(code);
}
return ret;
}Same: is there a way to use `bit_cast` here? (Maybe the array makes that trickier.)
base::span<const typename Traits::ExceptionCode> codes,I think that the reason that I didn’t do this initially was to allow for the possibility that `code` might be `nullptr`, and to not take a stance on that but to allow the implementer of this interface to decide how to handle it.
That shouldn’t happen in practice, but we’re not in total control of caller behavior. How do you handle that here? (Looks like you crash in `CopyUnalignedExceptionCodes`.)
codes.size()),I believe this is what the old code was doing, but I'm pretty sure it's wrong. Thoughts?
I believe this is what the old code was doing, but I'm pretty sure it's wrong. Thoughts?
- David Benjamin
- 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")
codes.data(),
codes.size(),David BenjaminAlternatively, we could redefine these interfaces to pass a `const std::vector<mach_exception_data_type_t>&`.
Done, but with spans. Also made the funny class just be a helper function that returns a `std::vector`.
Done, but with spans. Also made the funny class just be a helper function that returns a `std::vector`.
(look, 20+-month-ago me agreed!)