Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-1883 Report binary image with no UUID #724

Merged
merged 1 commit into from
Jan 19, 2022
Merged

RUMM-1883 Report binary image with no UUID #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 14 additions & 17 deletions Sources/DatadogCrashReporting/PLCrashReporterIntegration/CrashReport.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ internal struct BinaryImageInfo {
}

/// The UUID of this image.
var uuid: String
var uuid: String?
/// The name of this image (referenced by "library name" in the stack frame).
var imageName: String
/// If its a system library image.
Expand Down Expand Up @@ -232,14 +232,12 @@ extension ThreadInfo {

extension BinaryImageInfo {
init?(from imageInfo: PLCrashReportBinaryImageInfo) {
guard let imagePath = imageInfo.imageName,
let imageUUID = imageInfo.hasImageUUID ? imageInfo.imageUUID : nil else {
// We can drop this image as it won't be useful for symbolication - both
// "image name" and "uuid" are necessary.
guard let imagePath = imageInfo.imageName else {
// We can drop this image as it won't be useful for symbolication
return nil
}

self.uuid = imageUUID
self.uuid = imageInfo.imageUUID
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's safe, but wanted to double check if this won't crash if imageUUID is nil? In PLCR it is defined as implicitly unwrapped String:

/**
 * 128-bit object UUID (matches Mach-O DWARF dSYM files). May be nil if unavailable.
 */
@property(nonatomic, readonly, strong) NSString *imageUUID;

Hence, previous code was checking imageInfo.hasImageUUID before evaluating it.

/**
 * YES if this image has an associated UUID.
 */
@property(nonatomic, readonly) BOOL hasImageUUID;

The problem with implicit unwrapping is common to many PLCR types/fields and is solved by our CrashReport (swift). This test should already cover it:

dd-sdk-ios/Tests/DatadogCrashReportingTests/PLCrashReporterIntegration/CrashReportTests.swift

Line 65 in 3b43f91

func testGivenPLCrashReportWithSomeInconsistentValues_whenInitializing_itReturnsValue() throws {

but for the sake of sanity, let's double check if it does.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without NS_ASSUME_NONNULL_* region or _Nonnull objc annotation, the property is implicitly unwrapped optional, so this won't crash as uuid is now optional. The previous check was not necessary since the guard let was already evaluating nullability.
But I can checks in test tho 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually already covered in test :)

dd-sdk-ios/Tests/DatadogCrashReportingTests/Mocks.swift

Line 144 in 3b43f91

var mockImageUUID: String! = nil

self.imageName = URL(fileURLWithPath: imagePath).lastPathComponent

#if targetEnvironment(simulator)
Expand Down Expand Up @@ -303,18 +301,17 @@ extension BinaryImageInfo.CodeType {

extension StackFrame {
init(from stackFrame: PLCrashReportStackFrameInfo, number: Int, in crashReport: PLCrashReport) {
if let image = crashReport.image(forAddress: stackFrame.instructionPointer),
let imageInfo = BinaryImageInfo(from: image) {
self.libraryName = imageInfo.imageName
self.libraryBaseAddress = imageInfo.imageBaseAddress
} else {
// Without "library name" and its "base address" symbolication will not be possible,
// but the presence of this frame in the stack will be still relevant.
self.libraryName = nil
self.libraryBaseAddress = nil
}

self.number = number
self.instructionPointer = stackFrame.instructionPointer

// Without "library name" and its "base address" symbolication will not be possible,
// but the presence of this frame in the stack will be still relevant.
let image = crashReport.image(forAddress: stackFrame.instructionPointer)

self.libraryBaseAddress = image?.imageBaseAddress

if let imagePath = image?.imageName {
self.libraryName = URL(fileURLWithPath: imagePath).lastPathComponent
}
}
}
2 changes: 1 addition & 1 deletion Sources/DatadogCrashReporting/PLCrashReporterIntegration/DDCrashReportExporter.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ internal struct DDCrashReportExporter {

return DDCrashReport.BinaryImage(
libraryName: image.imageName,
uuid: image.uuid,
uuid: image.uuid ?? unavailable,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still relevant to report binary images with no UUID, like we do for missing architecture. Without uuid nor arch, symbolication won't be possible, but this will give more info to the crash report and help us identify missing data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checking - this will be possible to differentiate one ??? from another ??? by exploring the content of binary_images[], right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We shouldn't even get ??? in stack frames for known library from PLCR as it already checks for name nullability (as long as the name is UTF8).
And we will now be able to identify binary_images with no uuid, they were filtered out before.

architecture: image.codeType?.architectureName ?? unavailable,
isSystemLibrary: image.isSystemImage,
loadAddress: loadAddressHex,
Expand Down
2 changes: 1 addition & 1 deletion Tests/DatadogCrashReportingTests/Mocks.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ extension ThreadInfo {

extension BinaryImageInfo {
static func mockWith(
uuid: String = .mockAny(),
uuid: String? = .mockAny(),
imageName: String = .mockAny(),
isSystemImage: Bool = .random(),
architectureName: String? = .mockAny(),
Expand Down
11 changes: 11 additions & 0 deletions Tests/DatadogCrashReportingTests/PLCrashReporterIntegration/DDCrashReportExporterTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,17 @@ class DDCrashReportExporterTests: XCTestCase {
}
}

func testExportingBinaryImageWhenUUIDIsUnavailable() {
// Given
crashReport.binaryImages = [.mockWith(uuid: nil)]

// When
let exportedImages = exporter.export(crashReport).binaryImages

// Then
XCTAssertEqual(exportedImages.first?.uuid, "???")
}

func testExportingBinaryImageAddressRange() throws {
let randomImageLoadAddress: UInt64 = .mockRandom()
let randomImageSize: UInt64 = .mockRandom()
Expand Down