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

Improve CrashLogMessageExtractor memory safety #909

Merged
merged 5 commits into from
Aug 2, 2024
Merged

Improve CrashLogMessageExtractor memory safety #909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
458e08b
add CrashLogMessageExtractor swapCxaThrow: Bool arg. Add more memory …
mallexxx Jul 24, 2024
9f70f0e
only attach exception message to diagnostics payloads with non-empty …
mallexxx Jul 26, 2024
9783b0e
documentation
mallexxx Aug 1, 2024
bd3d913
Improve email sanitizing
mallexxx Aug 1, 2024
a440ead
improve file name detection, don‘t remove objc class/swift types look…
mallexxx Aug 1, 2024
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
22 changes: 16 additions & 6 deletions Sources/Common/Extensions/StringExtension.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ extension RegEx {
// from https://stackoverflow.com/a/25717506/73479
static let hostName = regex("^(((?!-)[A-Za-z0-9-]{1,63}(?<!-)\\.)*[A-Za-z0-9-]{2,63})$", .caseInsensitive)

static let email = regex(#"[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)*@[a-zA-Z0-9]+(?:\.[a-zA-Z0-9]+)+"#)
static let email = regex(#"[^\s]+@[^\s]+\.[^\s]+"#)
}

// Use this instead of NSLocalizedString for strings that are not supposed to be translated
Expand Down Expand Up @@ -99,7 +99,8 @@ public extension String {

// clean-up file paths and email addresses
func sanitized() -> String {
var message = self
// clean-up emails
var message = self.replacing(RegEx.email, with: "<removed>")

// find all the substring ranges looking like a file path
let pathRanges = message.rangesOfFilePaths()
Expand Down Expand Up @@ -132,15 +133,17 @@ public extension String {
}
}

// clean-up emails
message = message.replacing(RegEx.email, with: "<removed>")

return message
}

private enum FileRegex {
// "(matching url/file/path/at/in..-like prefix)(not an end of expr)(=|:) (open quote/brace)
static let varStart = regex(#"(?:url\b|\bfile\b|path\b|\bin\b|\bfrom\b|\bat)[^.,;?!"'`\])}>]\s*[:= ]?\s*["'“`\[({<]?"#, .caseInsensitive)
static let varStart = regex(#"(?:"# +
#"ur[il]\b|"# +
#"\b(?:config|input|output|temp|log|backup|resource)?file(?:ur[il]|name)?\b|"# +
#"\b(?:absolute|relative|network|temp|url|uri|config|input|output|log|backup|resource)?(?:file|directory|dir)?path(?:ur[il])?\b|"# +
#"\bin\b|\bfrom\b|\bat"# +
#")[^.,;?!"'`\])}>]\s*[:= ]?\s*["'“`\[({<]?"#, .caseInsensitive)
static let closingQuotes = [
"\"": regex(#""[,.;:]?(?:\s|$)|$"#),
"'": regex(#"'[,.;:]?(?:\s|$)|$"#),
Expand All @@ -163,6 +166,9 @@ public extension String {

static let lineNumber = regex(#":\d+$"#)
static let trailingSpecialCharacters = regex(#"[\s\.,;:\])}>"”'`]+$"#)

static let moduleTypeName = regex(#"^\.*[A-Za-z_]*(?:DuckDuckGo|DataBroker|NetworkProtection|VPNProxy)[A-Za-z_]*\.(?:(?:[A-Z_]+[a-z_]+)+)$"#)
static let swiftTypeName = regex(#"^\.*[A-Za-z_]+\.Type$"#)
}

// MARK: File Paths
Expand Down Expand Up @@ -291,6 +297,10 @@ public extension String {
dropLineNumberAndTrimSpaces(&resultRange)

guard let pathRange = Range(NSRange(resultRange, in: self)), pathRange.count > 2 else { continue }
// don‘t remove type names like _NSViewAnimator_DuckDuckGo_Privacy_Browser.MouseOverButton or Any.Type
let fileName = String(self[resultRange])
guard FileRegex.moduleTypeName.matches(in: fileName, range: fileName.fullRange).isEmpty,
FileRegex.swiftTypeName.matches(in: fileName, range: fileName.fullRange).isEmpty else { continue }
// collect the result
result.insert(integersIn: pathRange)
}
Expand Down
88 changes: 64 additions & 24 deletions Sources/Crashes/CrashCollection.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ public final class CrashCollection {
}, didFindCrashReports: didFindCrashReports)
}

/// Start `MXDiagnostic` (App Store) crash processing with callbacks called when crash data is found
/// - Parameters:
/// - process: callback preprocessing `MXDiagnosticPayload` Array and returning processed JSON Data Array to be uploaded
/// - didFindCrashReports: callback called after payload preprocessing is finished.
/// Provides processed JSON data to be presented to the user and Pixel parameters to fire a crash Pixel.
/// `uploadReports` callback is used when the user accepts uploading the crash report and starts crash upload to the server.
public func start(process: @escaping ([MXDiagnosticPayload]) -> [Data], didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) {
let first = isFirstCrash
isFirstCrash = false
Expand Down Expand Up @@ -89,6 +95,12 @@ public final class CrashCollection {
MXMetricManager.shared.add(crashHandler)
}

/// Start `MXDiagnostic` (App Store) crash processing with `didFindCrashReports` callback called when crash data is found and processed.
/// This method collects additional NSException/C++ exception data (message and stack trace) saved by `CrashLogMessageExtractor` and attaches it to payloads.
/// - Parameters:
/// - didFindCrashReports: callback called after payload preprocessing is finished.
/// Provides processed JSON data to be presented to the user and Pixel parameters to fire a crash Pixel.
/// `uploadReports` callback is used when the user accepts uploading the crash report and starts crash upload to the server.
public func startAttachingCrashLogMessages(didFindCrashReports: @escaping (_ pixelParameters: [[String: String]], _ payloads: [Data], _ uploadReports: @escaping () -> Void) -> Void) {
start(process: { payloads in
payloads.compactMap { payload in
Expand All @@ -98,34 +110,62 @@ public final class CrashCollection {
if #available(macOS 14.0, iOS 17.0, *) {
pid = payload.crashDiagnostics?.first?.metaData.pid
}
var crashDiagnostics = dict["crashDiagnostics"] as? [[AnyHashable: Any]] ?? []
var crashDiagnosticsDict = crashDiagnostics.first ?? [:]
var diagnosticMetaDataDict = crashDiagnosticsDict["diagnosticMetaData"] as? [AnyHashable: Any] ?? [:]
var objCexceptionReason = diagnosticMetaDataDict["objectiveCexceptionReason"] as? [AnyHashable: Any] ?? [:]

var exceptionMessage = (objCexceptionReason["composedMessage"] as? String)?.sanitized()
var stackTrace: [String]?

// append crash log message if loaded
if let diagnostic = try? CrashLogMessageExtractor().crashDiagnostic(for: payload.timeStampBegin, pid: pid)?.diagnosticData(), !diagnostic.isEmpty {
if let existingMessage = exceptionMessage, !existingMessage.isEmpty {
exceptionMessage = existingMessage + "\n\n---\n\n" + diagnostic.message
} else {
exceptionMessage = diagnostic.message
// The `MXDiagnostic` payload may not include `crashDiagnostics`, but may instead contain `cpuExceptionDiagnostics`,
// `diskWriteExceptionDiagnostics`, or `hangDiagnostics`.
// For now, we are ignoring these.
if var crashDiagnostics = dict["crashDiagnostics"] as? [[AnyHashable: Any]], !crashDiagnostics.isEmpty {
var crashDiagnosticsDict = crashDiagnostics[0]
var diagnosticMetaDataDict = crashDiagnosticsDict["diagnosticMetaData"] as? [AnyHashable: Any] ?? [:]
var objCexceptionReason = diagnosticMetaDataDict["objectiveCexceptionReason"] as? [AnyHashable: Any] ?? [:]

var exceptionMessage = (objCexceptionReason["composedMessage"] as? String)?.sanitized()
var stackTrace: [String]?

// Look up for NSException/C++ exception diagnostics data (name, reason, `userInfo` dictionary, stack trace)
// by searching for the related crash diagnostics file using the provided crash event timestamp and/or the crashed process PID.
// The stored data was sanitized to remove any potential filename or email occurrences.
if let diagnostic = try? CrashLogMessageExtractor().crashDiagnostic(for: payload.timeStampBegin, pid: pid)?.diagnosticData(), !diagnostic.isEmpty {
// append the loaded crash diagnostics message if the `MXDiagnostic` already contains one
if let existingMessage = exceptionMessage, !existingMessage.isEmpty {
exceptionMessage = existingMessage + "\n\n---\n\n" + diagnostic.message
} else {
// set the loaded diagnostics message in place of the original `MXDiagnostic` exceptionMessage if none exists
exceptionMessage = diagnostic.message
}
stackTrace = diagnostic.stackTrace
}
stackTrace = diagnostic.stackTrace
}

objCexceptionReason["composedMessage"] = exceptionMessage
objCexceptionReason["stackTrace"] = stackTrace
diagnosticMetaDataDict["objectiveCexceptionReason"] = objCexceptionReason
crashDiagnosticsDict["diagnosticMetaData"] = diagnosticMetaDataDict
if crashDiagnostics.isEmpty {
crashDiagnostics = [crashDiagnosticsDict]
} else {
/** Rebuild the original `MXDiagnostic` JSON with appended crash diagnostics:
```
{
"crashDiagnostics": [
{
"callStackTree": { ... },
"diagnosticMetaData": {
"appVersion": "1.95.0",
"objectiveCexceptionReason": {
"composedMessage": "NSTableViewException: Row index 9223372036854775807 out of row range (numberOfRows: 0)",
"stackTrace": [
"0 CoreFoundation 0x00000001930072ec __exceptionPreprocess + 176",
"1 libobjc.A.dylib 0x0000000192aee788 objc_exception_throw + 60",
"2 AppKit 0x00000001968dc20c -[NSTableRowData _availableRowViewWhileUpdatingAtRow:] + 0,",
...
]
},
...
}
}
],
"timeStampBegin": "2024-07-05 14:10:00",
}
``` */
objCexceptionReason["composedMessage"] = exceptionMessage
objCexceptionReason["stackTrace"] = stackTrace
diagnosticMetaDataDict["objectiveCexceptionReason"] = objCexceptionReason
crashDiagnosticsDict["diagnosticMetaData"] = diagnosticMetaDataDict
crashDiagnostics[0] = crashDiagnosticsDict
dict["crashDiagnostics"] = crashDiagnostics
}
dict["crashDiagnostics"] = crashDiagnostics

guard JSONSerialization.isValidJSONObject(dict) else {
assertionFailure("Invalid JSON object: \(dict)")
Expand Down
31 changes: 29 additions & 2 deletions Sources/Crashes/CrashLogMessageExtractor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import Foundation
/// `applicationSupportDir/Diagnostics/2024-05-20T12:11:33Z-%pid%.log`
public struct CrashLogMessageExtractor {

/// Structure representing an NSException/C++ diagnostic data file stored in the `Application Support/Diagnostics` directory.
public struct CrashDiagnostic {

// ""2024-05-22T08:17:23Z59070.log"
Expand All @@ -57,6 +58,8 @@ public struct CrashLogMessageExtractor {
public let timestamp: Date
public let pid: pid_t

/// Failable constructor parsing the provided file name to extract crash timestamp and pid from the provided NSException/C++ diagnostic data file URL.
/// - Returns: `nil` if the provided file name doesn’t match the `2024-05-20T12:11:33Z-%pid%.log` format.
init?(url: URL) {
let fileName = url.lastPathComponent

Expand All @@ -77,6 +80,7 @@ public struct CrashLogMessageExtractor {
self.pid = pid
}

/// JSON-codable NSException/C++ diagnostic data container.
public struct DiagnosticData: Codable {
public let message: String
public let stackTrace: [String]
Expand All @@ -91,6 +95,7 @@ public struct CrashLogMessageExtractor {
}
}

/// Try reading diagnostic data from the diagnostics file URL.
public func diagnosticData() throws -> DiagnosticData {
do {
let data = try Data(contentsOf: url)
Expand All @@ -107,7 +112,24 @@ public struct CrashLogMessageExtractor {
fileprivate static var nextCppTerminateHandler: (() -> Void)!
fileprivate static var diagnosticsDirectory: URL!

public static func setUp() {
/// Install uncaught NSException and C++ exception handlers.
/// - Parameters:
/// - swapCxaThrow: whether the `__cxa_throw` hook should be installed.
/// - Note:
/// `__cxa_throw` is a method called under the hood when `throw MyCppException();` is executed.
/// It unwinds the stack to look for a `catch` handler to handle the thrown exception. If the handler can’t
/// be found, `std::terminate` is called, which crashes the app.
///
/// We use the `__cxa_throw` hook (or call it _swizzle_) to record the stack trace of the original place
/// where the exception was thrown and write it to the current NSThread dictionary.
///
/// If the exception causes app termination, we check the NSThread dictionary for the recorded stack trace
/// in our custom `std::terminate` handler that we install using `std::set_terminate` and save
/// the exception message and the stack trace to the _Diagnostics_ directory.
///
/// After the custom `std::terminate` or `NSUncaughtExceptionHandler` is done, we call the
/// original exception handler, which causes the app termination.
public static func setUp(swapCxaThrow: Bool = true) {
prepareDiagnosticsDirectory()

// Set unhandled NSException handler
Expand All @@ -116,7 +138,9 @@ public struct CrashLogMessageExtractor {
// Set unhandled C++ exception handler
nextCppTerminateHandler = SetCxxExceptionTerminateHandler(handleTerminateOnCxxException)
// Swap C++ `throw` to collect stack trace when throw happens
CxaThrowSwapper.swapCxaThrow(with: captureStackTrace)
if swapCxaThrow {
CxaThrowSwapper.swapCxaThrow(with: captureStackTrace)
}
}

/// create App Support/Diagnostics folder
Expand Down Expand Up @@ -146,6 +170,9 @@ public struct CrashLogMessageExtractor {
self.diagnosticsDirectory = diagnosticsDirectory ?? Self.diagnosticsDirectory ?? self.fileManager.diagnosticsDirectory
}

/// Write exception diagnostics as JSON data, including the exception name, reason, and `userInfo` dictionary,
/// to `applicationSupportDir/Diagnostics/2024-05-20T12:11:33Z-%pid%.log`.
/// - Note: Data sanitization is applied to clean up any potential filename or email occurrences.
func writeDiagnostic(for exception: NSException) throws {
// collect exception diagnostics data
let message = (
Expand Down
Loading
Loading