ios: Validate exception code buffer size before read. [crashpad/crashpad : main]

Justin Cohen (Gerrit)

unread,

Feb 15, 2023, 12:12:47 PM2/15/23

to Mark Cogan, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Cogan.

View Change

3 comments:

Patchset:
- Patch Set #2:
  ptal, thanks!
File snapshot/ios/exception_snapshot_ios_intermediate_dump.cc:
- Patch Set #2, Line 182: // TODO: rationalize with the macOS implementation.
  I'm not sure what's left to change here. Mac's `ExceptionSnapshotMac::Initialize` sets `exception_` and `codes_` the same way. Mac has some additional logic for EXC_CRASH, EXC_RESOURCE and EXC_GUARD crashes, but we can't catch those on iOS.
  What do you think?
- Patch Set #2, Line 189: code_count
  This check probably isn't necessary since, but the second one is. Any objections?

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Mark Mentovai (Gerrit)

unread,

Feb 15, 2023, 12:30:20 PM2/15/23

to Justin Cohen, Mark Cogan, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Justin Cohen, Mark Cogan.

View Change

3 comments:

File snapshot/ios/exception_snapshot_ios_intermediate_dump.cc:

- Patch Set #2, Line 182: // TODO: rationalize with the macOS implementation.
  I'm not sure what's left to change here. Mac's `ExceptionSnapshotMac::Initialize` sets `exception_` and `codes_` the same way. Mac has some additional logic for EXC_CRASH, EXC_RESOURCE and EXC_GUARD crashes, but we can't catch those on iOS.
  What do you think?

- Sounds fine.

- Patch Set #2, Line 189: code_count
  This check probably isn't necessary since, but the second one is. Any objections?

- I agree. Although you left out the explanation after “since”, I do see what I think you mean.
  The best way to express this is to turn it into a `DCHECK_GE`.
- Patch Set #2, Line 191:
```
      if (code_count >= 2)
        exception_address_ = code[1];
```
  There’s a strong push to always use braces now in Chrome, and it’s even stronger in Crashpad.

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Justin Cohen (Gerrit)

unread,

Feb 15, 2023, 1:03:02 PM2/15/23

to Mark Mentovai, Mark Cogan, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Mark Cogan, Mark Mentovai.

Patch set 3:Commit-Queue +1

View Change

4 comments:

Patchset:
- Patch Set #3:
  Thanks, ptal!
File snapshot/ios/exception_snapshot_ios_intermediate_dump.cc:

- Patch Set #2, Line 182: // TODO: rationalize with the macOS implementation.

- > I'm not sure what's left to change here. […]
  removed TODO.

- Patch Set #2, Line 189: code_count

- > This check probably isn't necessary since, but the second one is. Any objections? […]
  Done

- Patch Set #2, Line 191:
```
      if (code_count >= 2)
        exception_address_ = code[1];
```
  There’s a strong push to always use braces now in Chrome, and it’s even stronger in Crashpad.

- Done

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Mark Mentovai (Gerrit)

unread,

Feb 15, 2023, 1:11:14 PM2/15/23

to Justin Cohen, Mark Cogan, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Justin Cohen, Mark Cogan.

Patch set 3:Code-Review +1

View Change

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Justin Cohen (Gerrit)

unread,

Feb 15, 2023, 1:11:39 PM2/15/23

to Mark Mentovai, Mark Cogan, Crashpad LUCI CQ, crashp...@chromium.org

Attention is currently required from: Justin Cohen, Mark Cogan.

Patch set 3:Commit-Queue +2

View Change

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Crashpad LUCI CQ (Gerrit)

unread,

Feb 15, 2023, 1:13:50 PM2/15/23

to Justin Cohen, Mark Mentovai, Mark Cogan, crashp...@chromium.org

Crashpad LUCI CQ submitted this change.

View Change

Approvals:
  Justin Cohen: Commit
  Mark Mentovai: Looks good to me

ios: Validate exception code buffer size before read.

Bug: 1415371
Change-Id: I9e1bd902494a664d4f07829e686803712fa8e7a8
Reviewed-on: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/4255568
Reviewed-by: Mark Mentovai <ma...@chromium.org>
Commit-Queue: Justin Cohen <justi...@chromium.org>
---
M snapshot/BUILD.gn
M snapshot/ios/exception_snapshot_ios_intermediate_dump.cc
M snapshot/ios/process_snapshot_ios_intermediate_dump_test.cc
A snapshot/ios/testdata/crash-c44acfcbccd8c7a8
4 files changed, 25 insertions(+), 3 deletions(-)

To view, visit change 4255568. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward