Improve CrashLogMessageExtractor memory safety #909

mallexxx · 2024-07-24T10:39:50Z

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/414709148257752/1207878606354585/f
iOS PR: duckduckgo/iOS#3133
macOS PR: duckduckgo/macos-browser#3011
What kind of version bump will this require?: Minor

Description:

Move memory protection modifier withTemporaryUnprotectedMemory earlier, matching https://github.com/kstenerud/KSCrash/blob/master/Sources/KSCrashRecordingCore/KSCxaThrowSwapper.c
Add symbol tables/string table regions boundary checks for safer memory access
Add swapCxaThrow parameter to CrashLogMessageExtractor.setUp to allow disabling __cxa_throw swapping

Steps to test this PR:

pass swapCxaThrow: false to CrashLogMessageExtractor.setUp
detach debugger
simulate c++ crash
run from debugger
validate uploaded reports is generated (without the exception stack trace)
Simulate NSException and validate the stack trace is there

OS Testing:

Internal references:

Software Engineering Expectations
Technical Design Template

bwaresiak

@mallexxx I went through these changes, these looks safer programmatically but it's getting more and more difficult to process what's going on here. :)

So two questions:

How we can make this more accessible to regular iOS/macOS dev that typically sees dyld only during WWDC highlights :)
Can we test this handler logic?

bwaresiak · 2024-07-25T18:12:19Z

Sources/Crashes/Mach-O/ImageMap.swift

+        guard symtab.indices.contains(symtabIndex) else { return nil }
+        let strtabOffset = Int(symtab[symtabIndex].n_un.n_strx)
+        guard strtab.indices.contains(strtabOffset),
+              strtab.indices.contains(strtabOffset + 1) else { return nil }


I see all of this guarantees the checks on indexes 0 and 1 won't crash - could be a good idea to add some docs to the function to describe how safe to use is returned pointer.

there were crashes in swift‘s String(cString:) initializer for some C string pointers, I‘ve changed it to match the original KSCrash code using strcmp (bytewise compare) that doesn‘t copy the memory and stop at the first non-matching byte, so the check is there from KSCrash, probably strcmp would just work…

bwaresiak · 2024-07-25T19:52:09Z

Sources/Crashes/CxaThrowSwapper.swift

+                          strcmp(symbolName.advanced(by: 1), "__cxa_throw") == 0 else { continue }
+
+                    let sectionInfo = try Dl_info(section)
+                    os_log(.debug, log: CxaThrowSwapper.log, "found %s: %s", String(cString: symbolName), indirectSymbolBindings[i].debugDescription)

                    cxxOriginalThrowFunctions[UnsafeRawPointer(sectionInfo.dli_fbase)] = indirectSymbolBindings[i]


One note: as this will be processed during the app life cycle, I suspect there is one out of billion chance of this being modified as some other thread is handling exception (and re-throwing it to original handler) - but I don't think we really need to worry about it much tbh.

in general this should catch all the throws, write the last exception stack trace into a Thread dictionary and pass control to the designated handler (not inevitably leading to a crash), worths adding an extra C++ test but should just work.

bwaresiak · 2024-07-25T20:16:59Z

Sources/Crashes/CxaThrowSwapper.swift

+                    guard !indicesToSkip.contains(symtabIndex),
+                          let symbolName = imageMap.symbolName(at: Int(symtabIndex)),
+                          symbolName[0] != 0, symbolName[1] != 0,
+                          strcmp(symbolName.advanced(by: 1), "__cxa_throw") == 0 else { continue }


TBH I'm quite curious why we advance by 1 here :)

me too :)
it‘s skipping the first _, that‘s the original approach in KSCrash

If you check symbol table for a binary (e.g. nm /Applications/DuckDuckGo.app/Contents/MacOS/DuckDuckGo | less) then you'll notice that all symbols have a _ prefix. Swift mangled function names are prefixed with an underscore as well, and __cxa_throw is also prefixed with _ :)

bwaresiak · 2024-07-25T20:18:22Z

Sources/Crashes/CxaThrowSwapper.swift

@@ -112,7 +112,7 @@ private func processMachHeader(_ header: UnsafePointer<mach_header>?, slide: Int
 }

 private var cxxOriginalThrowFunctions = [UnsafeRawPointer: UnsafeRawPointer]()
-private let indicesToSkip = [UInt32(bitPattern: INDIRECT_SYMBOL_ABS), INDIRECT_SYMBOL_LOCAL, UInt32(Int64(INDIRECT_SYMBOL_LOCAL) | Int64(INDIRECT_SYMBOL_ABS))]
+private let indicesToSkip = [UInt32(INDIRECT_SYMBOL_ABS), INDIRECT_SYMBOL_LOCAL, INDIRECT_SYMBOL_LOCAL | UInt32(INDIRECT_SYMBOL_ABS)]


I wonder if we can have some tests that find these, feed them int the logic and validate we correctly skip these.

yes, let‘s plan a follow-up task to add some tests for the __cxa_throw hook machinery and for now release it disabled both for iOS and macOS

ayoy

This looks good to me, great job @mallexxx! I appreciate the recent changes to sanitizing the crash log 👏

For more context to others reading it (cc @bwaresiak):

I've spent some time playing with this last night, going through the code and trying to make sense of it and it worked quite well with the help of ChatGPT. I've synced with Alex on MM this morning and we talked about adding documentation (even though the code is a rewrite of KSCrash, it's now our code, so better be clear about what happens there and why). The docs are added now so it's good to go 💪
I agree it's a good idea to temporarily disable __cxa_throw swapping and re-enable it after adding tests. I've also spent some time last night trying to come up with a test, with a moderate success, then talked to Alex and he confirmed my rough plan, but I'll anyway leave it with Alex to implement tests.

…access checks.

…crashDiagnostics

…ing like a file name

Task/Issue URL: https://app.asana.com/0/414709148257752/1207878606354585/f BSK PR: duckduckgo/BrowserServicesKit#909 iOS PR: duckduckgo/iOS#3133

…3133) Task/Issue URL: https://app.asana.com/0/414709148257752/1207878606354585/f BSK PR: duckduckgo/BrowserServicesKit#909 macOS PR: duckduckgo/macos-browser#3011

* main: [DuckPlayer] - Create DuckPlayer target and move initially shared code (#921) Improve CrashLogMessageExtractor memory safety (#909)

* main: Tweaks to DB API, improved abstractions for better testability. (#913) Add RMF `messageShown` attribute (#923) [DuckPlayer] - Create DuckPlayer target and move initially shared code (#921) Improve CrashLogMessageExtractor memory safety (#909)

mallexxx requested a review from bwaresiak July 24, 2024 10:39

This was referenced Jul 24, 2024

restore CrashLogMessageExtractor with disabled __cxa_throw swapper duckduckgo/iOS#3133

Merged

restore CrashLogMessageExtractor duckduckgo/macos-browser#3011

Merged

bwaresiak reviewed Jul 25, 2024

View reviewed changes

ayoy approved these changes Aug 1, 2024

View reviewed changes

ayoy assigned mallexxx Aug 1, 2024

mallexxx added 5 commits August 2, 2024 14:08

add CrashLogMessageExtractor swapCxaThrow: Bool arg. Add more memory …

458e08b

…access checks.

only attach exception message to diagnostics payloads with non-empty …

9f70f0e

…crashDiagnostics

documentation

9783b0e

Improve email sanitizing

bd3d913

improve file name detection, don‘t remove objc class/swift types look…

a440ead

…ing like a file name

mallexxx force-pushed the alex/process-mach-header branch from 2a522cf to a440ead Compare August 2, 2024 08:10

mallexxx changed the base branch from main to hotfix/176.1.1-1 August 2, 2024 08:10

mallexxx merged commit 7325384 into hotfix/176.1.1-1 Aug 2, 2024
7 checks passed

mallexxx deleted the alex/process-mach-header branch August 2, 2024 08:16

mallexxx added a commit to duckduckgo/macos-browser that referenced this pull request Aug 2, 2024

restore CrashLogMessageExtractor (#3011)

9d3cc61

Task/Issue URL: https://app.asana.com/0/414709148257752/1207878606354585/f BSK PR: duckduckgo/BrowserServicesKit#909 iOS PR: duckduckgo/iOS#3133

samsymons added a commit that referenced this pull request Aug 3, 2024

Merge branch 'main' into bartek/data-debugging

fe39d1d

* main: [DuckPlayer] - Create DuckPlayer target and move initially shared code (#921) Improve CrashLogMessageExtractor memory safety (#909)

samsymons added a commit that referenced this pull request Aug 4, 2024

Merge branch 'main' into sam/add-rmf-message-shown-attribute

44ef02c

* main: [DuckPlayer] - Create DuckPlayer target and move initially shared code (#921) Improve CrashLogMessageExtractor memory safety (#909)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve CrashLogMessageExtractor memory safety #909

Improve CrashLogMessageExtractor memory safety #909

mallexxx commented Jul 24, 2024 •

edited

Loading

bwaresiak left a comment

bwaresiak Jul 25, 2024

mallexxx Jul 26, 2024

bwaresiak Jul 25, 2024

mallexxx Jul 26, 2024

bwaresiak Jul 25, 2024

mallexxx Jul 26, 2024

ayoy Jul 31, 2024

bwaresiak Jul 25, 2024 •

edited

Loading

mallexxx Jul 26, 2024

ayoy left a comment

Improve CrashLogMessageExtractor memory safety #909

Improve CrashLogMessageExtractor memory safety #909

Conversation

mallexxx commented Jul 24, 2024 • edited Loading

Internal references:

bwaresiak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwaresiak Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayoy left a comment

Choose a reason for hiding this comment

mallexxx commented Jul 24, 2024 •

edited

Loading

bwaresiak Jul 25, 2024 •

edited

Loading