[ios] Support guarding concurrent reads and writes to Annotations [crashpad/crashpad : main]
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the otherShouldn’t this be handled by the happens-before store/load memory order specified by the spin guard?
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the otherShouldn’t this be handled by the happens-before store/load memory order specified by the spin guard?
Good question! I honestly don't know why we need to `vm_read()` in the first place when we're in-process, but maybe Justin and Mark can weigh in there.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Justin Cohen (Gerrit)
Attention is currently required from: Ben Hamilton, Mark Mentovai, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
Good question! I honestly don't know why we need to `vm_read()` in the first place when we're in-pro […]
I think the general rule is to do vm_read everywhere for safety. I don't know if we need to vm_write here -- we haven't had to anywhere else. mark@ do you have any concerns not using vm_write here?
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
I think the general rule is to do vm_read everywhere for safety. […]
If you don't have any assurance that a pointer is good, it's not safe to dereference it directly from handler code. You need to `vm_read`, because it fails gracefully instead of crashing. In general, Crashpad handler code has a higher bar for defensiveness and not crashing. Crashpad also has to assume that the process may already be sick and suffering from memory corruption.
On iOS, the handler and client code mix in the same process. It's OK to do normal but less-safe things like dereferencing memory or entering the allocator from client-only code (perhaps maintaining annotations, for example), but not safe to do this in handler code (reading annotations after a crash to stick in a report, in the same example).
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
If you don't have any assurance that a pointer is good, it's not safe to dereference it directly fro […]
OK, so if I understand correctly, the `vm_read()` calls for the iOS in-process handler are not to actually read memory, but rather to handle the case where the pointers to the `Annotation`s themselves could have been clobbered.
In that case, the `vm_read()` will fail, and we'll skip the annotation.
That means this code does not need to `vm_write()`, since the handler has already verified that the memory region is good (for both reads and writes), including the atomic boolean for the spinguard.
I've updated the comments. Thanks!
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
OK, so if I understand correctly, the `vm_read()` calls for the iOS in-process handler are not to actually read memory, but rather to handle the case where the pointers to the `Annotation`s themselves could have been clobbered.
In that case, the `vm_read()` will fail, and we'll skip the annotation.
That means this code does not need to `vm_write()`, since the handler has already verified that the memory region is good (for both reads and writes), including the atomic boolean for the spinguard.
That assumes that the rest of the program is frozen and corruption can’t happen and the map can’t change after the first `vm_read` verified that the memory is mapped and readable. It also assumes that readable memory is also writeable.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
> OK, so if I understand correctly, the `vm_read()` calls for the iOS in-process handler are not to […]
Definitely true.
But as far as I can tell, that's already the case even before this CL — it's already the case that the `Annotation`'s contents can be completely garbled and the program can corrupt it/make the pages unreadable/etc. after the `vm_read()` succeeded.
Thankfully, the logic here is extremely simple (atomic test-and-set on a boolean), so I don't think the risk is much worse than it already is.
LMK if there's any way to improve this!
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
Definitely true. […]
OK, Justin explained offline. I misunderstood how this is using `vm_read()` — the kernel is indeed allocating memory on behalf of the in-process handler, and copying a snapshot of the `Annotation` into a new buffer.
That means we can use `ScopedSpinGuard` to at least skip reading the `Annotation` if it's mid-write, but we can't use it to hold the spin guard state and ensure existing writes quiesce/new ones don't start. I'll refine the comments some more and make sure everything works end-to-end.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #38, Line 1194:
// TODO: Do we have to vm_write() when we're in-process so the other
OK, Justin explained offline. […]
I realized `vm_read()` is not going to work for `ScopedSpinGuard`, since it could start reading the `Annotation` before a writing thread obtains the spin guard, and then continue reading data while the write is in progress.
I sent out https://crrev.com/c/4114550/ to introduce a new `ScopedVMMap` which we can use for `Annotation`s which support `ScopedSpinGuard`. It just uses `vm_remap()` instead of `vm_read()`.
We could change the existing code to use `vm_remap()` in the general case for all `Annotation`s (this would save a copy), but to be safe, this CL now only uses `ScopedVMMap` for `Annotation` which support `ScopedSpinGuard`.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Robert Sesek.
5 comments:
Commit Message:
For all changes in this series: is there a Bug: filed at https://crashpad.chromium.org/bug that we can reference to tie things together?
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #46, Line 1167:
// Use vm_read() to ensure that the linked-list AnnotationList head (which isRefer to the discussion about placement of blank lines before comments elsewhere in this patch series. Throughout this file (and change and series).
// TODO: Pass down a `params` object into this method to optionally enable
// a timeout here.Do you want to do this?
Patch Set #46, Line 1219:
/*timeout_nanos=*/This style, also per discussion elsewhere in this patch series.
File test/ios/crash_type_xctest.mm:
Patch Set #46, Line 17:
#include <vector>Blank line between C and C++ system headers.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Mark Mentovai, Robert Sesek.
5 comments:
Commit Message:
For all changes in this series: is there a Bug: filed at https://crashpad.chromium. […]
Thanks, filed https://bugs.chromium.org/p/crashpad/issues/detail?id=437 and added `Bug: crashpad:437` to each CL in this stack.
File client/ios_handler/in_process_intermediate_dump_handler.cc:
Patch Set #46, Line 1167:
// Use vm_read() to ensure that the linked-list AnnotationList head (which is
Refer to the discussion about placement of blank lines before comments elsewhere in this patch serie […]
Done
// TODO: Pass down a `params` object into this method to optionally enable
// a timeout here.Do you want to do this?
I think it would make sense to try a very small timeout (a few microseconds). Do you think it's worth experimenting? Happy to remove this comment if it doesn't seem worth pursuing (I was figuring we could thread a config object down and let the clients experiment).
Patch Set #46, Line 1219:
/*timeout_nanos=*/This style, also per discussion elsewhere in this patch series.
Changed to a constexpr.
File test/ios/crash_type_xctest.mm:
Patch Set #46, Line 17:
#include <vector>Blank line between C and C++ system headers.
Done
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen, Robert Sesek.
Patch set 56:Code-Review +1
2 comments:
Patchset:
For Justin’s and possibly Robert’s review as well.
File client/ios_handler/in_process_intermediate_dump_handler.cc:
// TODO: Pass down a `params` object into this method to optionally enable
// a timeout here.
I think it would make sense to try a very small timeout (a few microseconds). Do you think it's worth experimenting? Happy to remove this comment if it doesn't seem worth pursuing (I was figuring we could thread a config object down and let the clients experiment).
I think it’s worth experimenting, but if you know you don’t care or you know you’ll never get to it, I’d remove the comment.
It’s fine for the experimentation to be conducted in a follow-up.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen, Robert Sesek.
1 comment:
File client/ios_handler/in_process_intermediate_dump_handler.cc:
// TODO: Pass down a `params` object into this method to optionally enable
// a timeout here.
> I think it would make sense to try a very small timeout (a few microseconds). […]
OK, filed https://crbug.com/crashpad/438 to follow up and updated the TODO comment.
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Robert Sesek (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen.
Patch set 63:Code-Review +1
1 comment:
File test/ios/crash_type_xctest.mm:
nit: 1u
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Justin Cohen.
1 comment:
File test/ios/crash_type_xctest.mm:
nit: 1u
Done
To view, visit change 4031730. To unsubscribe, or for help writing mail filters, visit settings.
Ben Hamilton (Gerrit)
Attention is currently required from: Ben Hamilton, Justin Cohen.
Patch set 66:Commit-Queue +2
Crashpad LUCI CQ (Gerrit)
Crashpad LUCI CQ submitted this change.
63 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: test/ios/crash_type_xctest.mm
Insertions: 1, Deletions: 1.
@@ -326,7 +326,7 @@
XCTAssertTrue([[dict[@"objects"][2] valueForKeyPath:@"#TEST# one"]
isEqualToString:@"moocow"]);
// Ensure `ring_buffer` is present but not `busy_ring_buffer`.
- XCTAssertEqual(1ULL, [dict[@"ringbuffers"] count]);
+ XCTAssertEqual(1u, [dict[@"ringbuffers"] count]);
NSData* ringBufferNSData =
[dict[@"ringbuffers"][0] valueForKeyPath:@"#TEST# ring_buffer"];
crashpad::RingBufferData ringBufferData;
```
```
The name of the file: client/ios_handler/in_process_intermediate_dump_handler.cc
Insertions: 1, Deletions: 1.
@@ -1208,7 +1208,7 @@
// which lives for the duration of the read.
ScopedVMMap<Annotation> mapped_node;
std::optional<ScopedSpinGuard> annotation_guard;
- if (node->GetConcurrentAccessGuardMode() ==
+ if (node->concurrent_access_guard_mode() ==
Annotation::ConcurrentAccessGuardMode::kScopedSpinGuard) {
constexpr vm_prot_t kDesiredProtection = VM_PROT_WRITE | VM_PROT_READ;
if (!mapped_node.Map(node.get()) ||
```
[ios] Support guarding concurrent reads and writes to Annotations
Since iOS reads Annotations in-process, this CL updates the iOS
intermediate dump handler to check each Annotation to see if it supports
guarding concurrent reads and writes using ScopedSpinGuard.
For any such Annotation, the in-process dump handler now tries (without
spinning) to obtain the ScopedSpinGuard for the Annotation before
reading its memory.
If the ScopedSpinGuard cannot immediately be obtained, the in-process
dump handler just skips writing the memory of the Annotation to the
intermediate dump. (I'd like to follow up and thread down a Params
object so we can experiment with adding an optional timeout to make
this more reliable.)
Change-Id: Ie6c9849fac94ab89b36364b07aea62326cabe552
Bug: crashpad:437
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4031730
Commit-Queue: Ben Hamilton <benha...@google.com>
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Reviewed-by: Robert Sesek <rse...@chromium.org>
---
M client/ios_handler/in_process_intermediate_dump_handler.cc
M test/ios/BUILD.gn
M test/ios/crash_type_xctest.mm
M test/ios/host/cptest_application_delegate.mm
4 files changed, 130 insertions(+), 7 deletions(-)