snapshot: prepare MachOImageAnnotationsReader for macOS/iOS 26 [crashpad/crashpad : main]
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Commit-Queue | +1 |
3 comments
struct crashreporter_annotations_t {I didn’t touch this because I’m kind of afraid to. Technically it’s already wrong, because version 4 `crashreporter_annotations_t` won’t have `abort_cause`, but the `Read` on line 1153 doesn’t know that. It will usually succeed because util/ios/scoped_vm_read.cc reads full pages, and it’s not likely that `abort_cause` will spill off of the page and into unmapped memory. In any event, this code never uses `abort_cause`, so it doesn’t matter what it reads. But it’s not right, either. Since you won’t see version 4 structures in the wild anymore (they were last used on OS X 10.10 (current in 2014–2015), so probably iOS 8), this wasn’t a real bug worth worrying about. But now that this code will see both version 5 and 7 structures, and the version 7 structures are much larger than the version 5 ones, and I’m noticing the latent bug, I’m not going to make matters worse.
If you wanted to extend this structure, you could read the `version` field and then decide how much to read based on that, like I do for macOS in snapshot/mac/process_types/custom.cc `crashpad::process_types::internal::crashreporter_annotations_t<>::ExpectedSizeForVersion` and its caller snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. But I’m not equipped to test that more invasive change.
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);Another thing I noticed: I have to deal with `__DATA_DIRTY,__crash_info` on macOS in addition to this `__DATA,__crash_info`. For example, macOS 15.5 24F74’s /usr/lib/dyld uses the dirty segment, although macOS 26.0db4 25A5316i’s does not. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. You might need to deal with this on iOS too.
crash_info->version != 7)) {Also, I didn’t test this, and it doesn’t look like there’s any test that covers it.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 4 comments![]( "Open in Gerrit")
struct crashreporter_annotations_t {I didn’t touch this because I’m kind of afraid to. Technically it’s already wrong, because version 4 `crashreporter_annotations_t` won’t have `abort_cause`, but the `Read` on line 1153 doesn’t know that. It will usually succeed because util/ios/scoped_vm_read.cc reads full pages, and it’s not likely that `abort_cause` will spill off of the page and into unmapped memory. In any event, this code never uses `abort_cause`, so it doesn’t matter what it reads. But it’s not right, either. Since you won’t see version 4 structures in the wild anymore (they were last used on OS X 10.10 (current in 2014–2015), so probably iOS 8), this wasn’t a real bug worth worrying about. But now that this code will see both version 5 and 7 structures, and the version 7 structures are much larger than the version 5 ones, and I’m noticing the latent bug, I’m not going to make matters worse.
If you wanted to extend this structure, you could read the `version` field and then decide how much to read based on that, like I do for macOS in snapshot/mac/process_types/custom.cc `crashpad::process_types::internal::crashreporter_annotations_t<>::ExpectedSizeForVersion` and its caller snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. But I’m not equipped to test that more invasive change.
I don't think we ever supported anything below iOS 14/15.
Is there any value in fixing this now until we know what's beyond abort_cause?
Either way, I'd like to add a comment around line 63 noting that
any attempt to extend crashreporter_annotations_t would need, as you wrote, logic that reads the version field and then decides how much to read... for posterity.
While we are here -- do we know if anything but message is ever used?
constexpr size_t kMaxMessageSize = 1024;Should this be bumped to 4096 as well?
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);Another thing I noticed: I have to deal with `__DATA_DIRTY,__crash_info` on macOS in addition to this `__DATA,__crash_info`. For example, macOS 15.5 24F74’s /usr/lib/dyld uses the dirty segment, although macOS 26.0db4 25A5316i’s does not. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. You might need to deal with this on iOS too.
I wonder if I can just check both all the time by adding something like:
```
#define SEG_DIRTY_DATA "__DATA_DIRTY"
```
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DIRTY_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);
}
...
```
Is there any harm in always checking both?
crash_info->version != 7)) {Also, I didn’t test this, and it doesn’t look like there’s any test that covers it.
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai voted and added 7 comments![]( "Open in Gerrit")
Votes added by Mark Mentovai
| Commit-Queue | +1 |
7 comments
struct crashreporter_annotations_t {Justin CohenI didn’t touch this because I’m kind of afraid to. Technically it’s already wrong, because version 4 `crashreporter_annotations_t` won’t have `abort_cause`, but the `Read` on line 1153 doesn’t know that. It will usually succeed because util/ios/scoped_vm_read.cc reads full pages, and it’s not likely that `abort_cause` will spill off of the page and into unmapped memory. In any event, this code never uses `abort_cause`, so it doesn’t matter what it reads. But it’s not right, either. Since you won’t see version 4 structures in the wild anymore (they were last used on OS X 10.10 (current in 2014–2015), so probably iOS 8), this wasn’t a real bug worth worrying about. But now that this code will see both version 5 and 7 structures, and the version 7 structures are much larger than the version 5 ones, and I’m noticing the latent bug, I’m not going to make matters worse.
If you wanted to extend this structure, you could read the `version` field and then decide how much to read based on that, like I do for macOS in snapshot/mac/process_types/custom.cc `crashpad::process_types::internal::crashreporter_annotations_t<>::ExpectedSizeForVersion` and its caller snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. But I’m not equipped to test that more invasive change.
I don't think we ever supported anything below iOS 14/15.
Is there any value in fixing this now until we know what's beyond abort_cause?
Either way, I'd like to add a comment around line 63 noting that
any attempt to extend crashreporter_annotations_t would need, as you wrote, logic that reads the version field and then decides how much to read... for posterity.While we are here -- do we know if anything but message is ever used?
I don't think we ever supported anything below iOS 14/15.
Is there any value in fixing this now until we know what's beyond abort_cause?
Not really.
Either way, I'd like to add a comment around line 63 noting that
any attempt to extend crashreporter_annotations_t would need, as you wrote, logic that reads the version field and then decides how much to read... for posterity.
OK, I added that.
While we are here -- do we know if anything but message is ever used?
dyld uses both `message` and `message2`. I haven’t spotted anything else populating `message2` or any of the other fields, but I’m sure there are other users.
Should this be bumped to 4096 as well?
Should this be bumped to 4096 as well?
Yeah, you’re right, we should keep it in sync.
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);Justin CohenAnother thing I noticed: I have to deal with `__DATA_DIRTY,__crash_info` on macOS in addition to this `__DATA,__crash_info`. For example, macOS 15.5 24F74’s /usr/lib/dyld uses the dirty segment, although macOS 26.0db4 25A5316i’s does not. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. You might need to deal with this on iOS too.
I wonder if I can just check both all the time by adding something like:
```
#define SEG_DIRTY_DATA "__DATA_DIRTY"```
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DIRTY_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);
}
...
```Is there any harm in always checking both?
I wonder if I can just check both all the time by adding something like:
```
#define SEG_DIRTY_DATA "__DATA_DIRTY"
No need for the `#define`. We use `SEG_DATA` because the SDK provides it, but where it’s not provided to us, we’d just write `"__DATA_DIRTY"` in-line as the segment name argument. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`.
```
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DIRTY_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);
}
...
```Is there any harm in always checking both?
If the intermediate dump somehow winds up with with two `kAnnotationsCrashInfo` maps in a single module, the reader will (util/ios/ios_intermediate_dump_reader.cc `crashpad::internal::IOSIntermediateDumpReader::Parse` will log an error (`"Inserting duplicate key"`) and discard the second. (The message is wrong, it’s not inserting a duplicate, it’s _ignoring_ it, and it’d be helpful if it identified which key.) Maybe the error is tolerable? The net of this, behavior-wise, is that the minidump will contain the information from whichever segment appeared first in the Mach-O image, `__DATA` or `__DATA_DIRTY`, which is maybe a little nondeterministic?
Since the environment is so limited when writing the intermediate dump, if we wanted things to be more deterministic, I think we’d have to define a different key to use for `__DATA_DIRTY,__crash_info`, and then resolve it in snapshot/ios/module_snapshot_ios_intermediate_dump.cc `crashpad::internal:ModuleSnapshotIOSIntermediateDump::Initialize`.
continue;This was bug waiting to happen! If iOS 26 uses crashreporter_annotations_t::version 7 as macOS 26 does, this `continue` means that the loop will advance without moving `section_vm_read_ptr` to the next `section_64`. Or: once the loop lands on a `__DATA,__crash_info` that’s a version that it doesn’t understand, it’ll camp on that one and won’t ever check any sections that follow.
The loop will still terminate because it’s predicated on `sect_index <= segment_vm_read_ptr->nsects`, and `sect_index` progresses normally.
Practical impact is near zero because we don’t expect any modules to have both `__DATA,crashpad_info` and `__DATA,__crash_info`. But it was wrong enough that I wanted to fix it.
if (section_vm_read_ptr->size >= sizeof(CrashpadInfo)) {I added this size check and the one on line 1177 to make sure that we’re not reading a section beyond its bounds.
crashpad_info->size() <= section_vm_read_ptr->size &&
crashpad_info->size() >= sizeof(CrashpadInfo) &&I also changed the existing exact-size check to this, which is more in line with the macOS implementation: appropriately permissive (runtime structures might grow, and it’s no big deal because we’ll only look at the part we understand) while also respecting size fields for bounds checking.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen voted and added 3 comments![]( "Open in Gerrit")
Votes added by Justin Cohen
| Code-Review | +1 |
3 comments
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);Justin CohenAnother thing I noticed: I have to deal with `__DATA_DIRTY,__crash_info` on macOS in addition to this `__DATA,__crash_info`. For example, macOS 15.5 24F74’s /usr/lib/dyld uses the dirty segment, although macOS 26.0db4 25A5316i’s does not. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`. You might need to deal with this on iOS too.
Mark MentovaiI wonder if I can just check both all the time by adding something like:
```
#define SEG_DIRTY_DATA "__DATA_DIRTY"```
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DIRTY_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);
}
...
```Is there any harm in always checking both?
I wonder if I can just check both all the time by adding something like:
```
#define SEG_DIRTY_DATA "__DATA_DIRTY"No need for the `#define`. We use `SEG_DATA` because the SDK provides it, but where it’s not provided to us, we’d just write `"__DATA_DIRTY"` in-line as the segment name argument. See snapshot/mac/mach_o_image_annotations_reader.cc `crashpad::MachOImageAnnotationsReader::ReadCrashReporterClientAnnotations`.
```
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DIRTY_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);
}
...
```Is there any harm in always checking both?
If the intermediate dump somehow winds up with with two `kAnnotationsCrashInfo` maps in a single module, the reader will (util/ios/ios_intermediate_dump_reader.cc `crashpad::internal::IOSIntermediateDumpReader::Parse` will log an error (`"Inserting duplicate key"`) and discard the second. (The message is wrong, it’s not inserting a duplicate, it’s _ignoring_ it, and it’d be helpful if it identified which key.) Maybe the error is tolerable? The net of this, behavior-wise, is that the minidump will contain the information from whichever segment appeared first in the Mach-O image, `__DATA` or `__DATA_DIRTY`, which is maybe a little nondeterministic?
Since the environment is so limited when writing the intermediate dump, if we wanted things to be more deterministic, I think we’d have to define a different key to use for `__DATA_DIRTY,__crash_info`, and then resolve it in snapshot/ios/module_snapshot_ios_intermediate_dump.cc `crashpad::internal:ModuleSnapshotIOSIntermediateDump::Initialize`.
ack.
If the intermediate dump somehow winds up with with two kAnnotationsCrashInfo maps in a single module, the reader will
Did you encounter this on the macOS side, two different __crash_info sectnames?
I'm trying to decide if this is worth implementing at all, with the naive approach of assuming just one and dropping the other, or trying to get both.
if (section_vm_read_ptr->size >= sizeof(CrashpadInfo)) {I added this size check and the one on line 1177 to make sure that we’re not reading a section beyond its bounds.
Thanks!
crashpad_info->size() <= section_vm_read_ptr->size &&
crashpad_info->size() >= sizeof(CrashpadInfo) &&I also changed the existing exact-size check to this, which is more in line with the macOS implementation: appropriately permissive (runtime structures might grow, and it’s no big deal because we’ll only look at the part we understand) while also respecting size fields for bounds checking.
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
ack.
If the intermediate dump somehow winds up with with two kAnnotationsCrashInfo maps in a single module, the reader will
Did you encounter this on the macOS side, two different __crash_info sectnames?
No. There’s both `__DATA` and `__DATA_DIRTY` simultaneously in a single module, but I’ve only seen at most one with `__crash_info`.
I'm trying to decide if this is worth implementing at all, with the naive approach of assuming just one and dropping the other, or trying to get both.
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
} else if (strcmp(segment_vm_read_ptr->segname, SEG_DATA) == 0) {
WriteDataSegmentAnnotations(writer, segment_vm_read_ptr, slide);- Mark Mentovai
Code-Owners
Code-Review
| 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
| Commit-Queue | +2 |
1 comment
Justin and I can keep talking about `__DATA_DIRTY` on iOS and deal with it if needed. It’s out of scope for this change.
Related details
Code-Owners
Code-Review
| 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
snapshot: prepare MachOImageAnnotationsReader for macOS/iOS 26
macOS/iOS 26 introduce a new version of the crashreporter_annotations_t
structure used in __DATA,__crash_info or __DATA_DIRTY,__crash_info:
version 7. This is expected to be the value of
CRASHREPORTER_ANNOTATIONS_VERSION in the private
<CrashReporterClient.h>. Version 7 crashreporter_annotations_t
structures are empirically observed to be 328 bytes long, which is 264
bytes (or 33 uint64_ts) longer than the previously known version 5
structure. Version 5 was used from OS X 10.11 through macOS 15, and
there does not appear to be a version 6. The precise layout and meaning
of the new fields in the version 7 extension is not yet known.
- M client/ios_handler/in_process_intermediate_dump_handler.cc
- M snapshot/mac/mach_o_image_annotations_reader.cc
- M snapshot/mac/mach_o_image_annotations_reader.h
- M snapshot/mac/process_types/crashreporterclient.proctype
- M snapshot/mac/process_types/custom.cc
Code-Review: +1 by Justin Cohen
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |