Workaround for a crash caused by integer overflow in address arithmetic.#364
Conversation
fabianmuecke
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR implements a workaround for crashes caused by integer overflow during address arithmetic by filtering out problematic crash reports rather than processing them. The changes include updating the binary images parsing to return a Result type, modifying arithmetic operators on Address to return optionals on overflow, and adapting the crash formatting and diagnostic flows accordingly.
- Updated tests to verify that crash formatting no longer crashes.
- Modified Report and BinaryImage types to handle invalid binary image data using Result.
- Adjusted arithmetic operations and crash filter flows to safely handle integer overflow.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| RollbarReportTests/RollbarReportTests.swift | Added a test to ensure crash report formatting does not crash. |
| RollbarReport/Report/Report.swift | Changed binaryImages property to return a Result and updated mapping logic accordingly. |
| RollbarReport/Report/BinaryImage.swift | Modified initializer to return nil when address arithmetic fails to prevent invalid image data. |
| RollbarReport/Report/Address.swift | Updated operator overloads for + and - to safely return nil on overflow using reporting APIs. |
| RollbarReport/Formatter/RollbarCrashFormattingFilter.swift | Adjusted formatting functions to work with the new Result-based types and improved error handling. |
| RollbarReport/Formatter/FormatBuilder.swift | Updated Formatted structure and builder extensions for handling Result types. |
| RollbarReport/Error.swift | Added new NSError factories for invalid binary images and addresses. |
| RollbarReport/Diagnostic/RollbarCrashDiagnosticFilter.swift | Updated binaryImages handling in diagnostic logic to unwrap the Result type. |
matux
left a comment
matux
left a comment
There was a problem hiding this comment.
This is great @fabianmuecke! I really appreciate your contribution. I left you a few comments, I don't mean to take much of your time, but I'd like to see if I understand what you're going for, and what an ideal solution would look like.
We'll probably merge as is and then work on a way to handle this more gracefully.
Great job and again, much appreciated!!!
| var binaryImages: Result<[BinaryImage], NSError> { | ||
| let raw: [Map] = self[any: "binary_images", default: []] | ||
| let images = raw.map(BinaryImage.init) | ||
| let result = images.compactMap { $0 } | ||
| return result.count == images.count | ||
| ? .success(result) | ||
| : .failure(.invalidBinaryImages(self, names: raw.compactMap { $0[any: "name"] })) |
There was a problem hiding this comment.
So, ideally, we could have a concept of InvalidBinaryImage, so we don't invalidate the entire array. What do you think?
(Not asking you to do it, just floating the idea!)
There was a problem hiding this comment.
I was thinking about simply compactMapping over the results and removing invalid binary images. But I don't know enough about what would happen, if a binary is missing and the report is submitted anyway, so I played it safe. If the backend systems will be able to handle a report with a missing binary image, it'd of course be desirable to still receive the report, even if it's missing a binary image. 👍
| extension Formatted { | ||
| /// Concatenates two formatted optional strings. | ||
| @inlinable | ||
| static func *? <Success: StringProtocol, Failure: Error>(lhs: Self, rhs: Self) -> Formatted<Result<String, Failure>> | ||
| where T == Result<Success, Failure> { | ||
| switch (lhs.string, rhs.string) { | ||
| case let (.failure(failure), _), | ||
| let (_, .failure(failure)): | ||
| Formatted<Result<String, Failure>>(string: .failure(failure)) | ||
| case (.none, _), | ||
| (_, .none): | ||
| Formatted<Result<String, Failure>>(string: .none) | ||
| case let (.success(lhs), .success(rhs)): | ||
| Formatted<Result<String, Failure>>(string: .success(lhs.appending(rhs))) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
So, here's where the magic happens, right?
If I understand correctly, you're adding the idea that a T can be a Result in order to return a Failure in case of an overflow, is that correct?
Ideally, what we'd want is to return some error string, maybe to avoid all the workaround dance. (Not asking you to do it, or implying what you did is wrong, just thinking out loud to see what you think, too!)
There was a problem hiding this comment.
This was again because I didn't know what would happen, if I just returned something like "invalid address" instead of failing and then sent that now invalid report to the backend systems. If the backend doesn't care about the actual content of these reports, I expect a stack trace containing an invalid address might still be valuable in a lot of cases – at least much more valuable than not knowing about the crash at all.
Hold on. Looks like that last change broke a test 🤔 Edit: Of course it did. 🤦♂️ Fixed. |
Head branch was pushed to by a user without write access
|
@matux Could you rerun the workflow? 🙏 |
That's why we have them, no worries. |
Description of the change
This PR prevents Rollbar from crashing the app when
Rollbar.initWithConfiguration(_:)is called and a crash report contains addresses that would cause integer overflows during address arithmetic.This change does not address the underlying cause of the address overflows, but ensures that reports with such issues are filtered out and not uploaded to Rollbar.
@matux Just a quick note—I updated the implementation this morning after noticing that failing reports were being filtered out, but filterReports was still reporting success. Today’s commit addresses this issue.
Type of change
Related issues
Checklists
Development
Code review