From cbb54e2a7d3eca88675bc4086a02775281728707 Mon Sep 17 00:00:00 2001 From: Nikolay Volosatov Date: Fri, 11 Oct 2024 17:34:54 +0100 Subject: [PATCH] Make SIGTERM monitoring optional (#578) * Add flag for enabling SIGTERM monitoring * Add integration tests * Fix format * Disable signal tests on watchOS * Add flag to sample * Fix false-positive OOM reports --- .../InstallConfig.swift | 4 ++ .../IntegrationTestRunner.swift | 5 +++ .../SampleUI/Screens/InstallView.swift | 3 ++ Samples/Tests/Core/IntegrationTestBase.swift | 22 ++++++++++ Samples/Tests/Core/PartialCrashReport.swift | 9 +++++ Samples/Tests/IntegrationTests.swift | 40 +++++++++++++++++++ Sources/KSCrashRecording/KSCrashC.c | 1 + .../KSCrashRecording/KSCrashConfiguration.m | 3 ++ .../Monitors/KSCrashMonitor_Memory.h | 6 +++ .../Monitors/KSCrashMonitor_Memory.m | 2 + .../Monitors/KSCrashMonitor_Signal.c | 21 +++++++++- .../Monitors/KSCrashMonitor_Signal.h | 7 ++++ .../include/KSCrashCConfiguration.h | 12 ++++++ .../include/KSCrashConfiguration.h | 11 +++++ 14 files changed, 145 insertions(+), 1 deletion(-) diff --git a/Samples/Common/Sources/IntegrationTestsHelper/InstallConfig.swift b/Samples/Common/Sources/IntegrationTestsHelper/InstallConfig.swift index 164b4b11..90bd27e3 100644 --- a/Samples/Common/Sources/IntegrationTestsHelper/InstallConfig.swift +++ b/Samples/Common/Sources/IntegrationTestsHelper/InstallConfig.swift @@ -30,6 +30,7 @@ import KSCrashRecording public struct InstallConfig: Codable { public var installPath: String public var isCxaThrowEnabled: Bool? + public var isSigTermMonitoringEnabled: Bool? public init(installPath: String) { self.installPath = installPath @@ -43,6 +44,9 @@ extension InstallConfig { if let isCxaThrowEnabled { config.enableSwapCxaThrow = isCxaThrowEnabled } + if let isSigTermMonitoringEnabled { + config.enableSigTermMonitoring = isSigTermMonitoringEnabled + } try KSCrash.shared.install(with: config) } } diff --git a/Samples/Common/Sources/IntegrationTestsHelper/IntegrationTestRunner.swift b/Samples/Common/Sources/IntegrationTestsHelper/IntegrationTestRunner.swift index 27386a42..0dd2e253 100644 --- a/Samples/Common/Sources/IntegrationTestsHelper/IntegrationTestRunner.swift +++ b/Samples/Common/Sources/IntegrationTestsHelper/IntegrationTestRunner.swift @@ -79,4 +79,9 @@ public extension IntegrationTestRunner { let data = try JSONEncoder().encode(Script(install: install, report: report, delay: delay)) return data.base64EncodedString() } + + static func script(install: InstallConfig? = nil, delay: TimeInterval? = nil) throws -> String { + let data = try JSONEncoder().encode(Script(install: install, delay: delay)) + return data.base64EncodedString() + } } diff --git a/Samples/Common/Sources/SampleUI/Screens/InstallView.swift b/Samples/Common/Sources/SampleUI/Screens/InstallView.swift index 23827522..2f14a711 100644 --- a/Samples/Common/Sources/SampleUI/Screens/InstallView.swift +++ b/Samples/Common/Sources/SampleUI/Screens/InstallView.swift @@ -76,6 +76,9 @@ struct InstallView: View { Toggle(isOn: bridge.configBinding(for: \.enableSwapCxaThrow)) { Text("Swap __cxa_throw") } + Toggle(isOn: bridge.configBinding(for: \.enableSigTermMonitoring)) { + Text("SIGTERM monitoring") + } } Button("Only set up reports") { diff --git a/Samples/Tests/Core/IntegrationTestBase.swift b/Samples/Tests/Core/IntegrationTestBase.swift index 78ac4edf..a8c1efc7 100644 --- a/Samples/Tests/Core/IntegrationTestBase.swift +++ b/Samples/Tests/Core/IntegrationTestBase.swift @@ -158,12 +158,29 @@ class IntegrationTestBase: XCTestCase { return report } + func hasCrashReport() throws -> Bool { + let reportsDirUrl = installUrl.appendingPathComponent("Reports") + let files = try? FileManager.default.contentsOfDirectory(atPath: reportsDirUrl.path) + return (files ?? []).isEmpty == false + } + func readAppleReport() throws -> String { let url = try waitForFile(in: appleReportsUrl, timeout: reportTimeout) let appleReport = try String(contentsOf: url) return appleReport } + func launchAndInstall(installOverride: ((inout InstallConfig) throws -> Void)? = nil) throws { + var installConfig = InstallConfig(installPath: installUrl.path) + try installOverride?(&installConfig) + app.launchEnvironment[IntegrationTestRunner.envKey] = try IntegrationTestRunner.script( + install: installConfig, + delay: actionDelay + ) + + launchAppAndRunScript() + } + func launchAndCrash(_ crashId: CrashTriggerId, installOverride: ((inout InstallConfig) throws -> Void)? = nil) throws { var installConfig = InstallConfig(installPath: installUrl.path) try installOverride?(&installConfig) @@ -188,4 +205,9 @@ class IntegrationTestBase: XCTestCase { let report = try readAppleReport() return report } + + func terminate() throws { + app.terminate() + _ = app.wait(for: .notRunning, timeout: self.appTerminateTimeout) + } } diff --git a/Samples/Tests/Core/PartialCrashReport.swift b/Samples/Tests/Core/PartialCrashReport.swift index fa9677f1..697346a8 100644 --- a/Samples/Tests/Core/PartialCrashReport.swift +++ b/Samples/Tests/Core/PartialCrashReport.swift @@ -29,8 +29,17 @@ import Foundation struct PartialCrashReport: Decodable { struct Crash: Decodable { struct Error: Decodable { + struct Signal: Decodable { + var signal: Int? + var name: String? + var code: Int? + var code_name: String? + } + var reason: String? var type: String? + + var signal: Signal? } struct Thread: Decodable { diff --git a/Samples/Tests/IntegrationTests.swift b/Samples/Tests/IntegrationTests.swift index 7c0e5ba4..e3e9d04c 100644 --- a/Samples/Tests/IntegrationTests.swift +++ b/Samples/Tests/IntegrationTests.swift @@ -70,6 +70,46 @@ final class CppTests: IntegrationTestBase { } } +#if !os(watchOS) + +final class SignalTests: IntegrationTestBase { + func testAbort() throws { + try launchAndCrash(.signal_abort) + + let rawReport = try readPartialCrashReport() + try rawReport.validate() + XCTAssertEqual(rawReport.crash?.error?.type, "signal") + XCTAssertEqual(rawReport.crash?.error?.signal?.name, "SIGABRT") + + let appleReport = try launchAndReportCrash() + XCTAssertTrue(appleReport.contains("SIGABRT")) + } + + func testTermination() throws { + // Default (termination monitoring disabled) + try launchAndInstall() + try terminate() + + XCTAssertFalse(try hasCrashReport()) + + // With termination monitoring enabled + try launchAndInstall { config in + config.isSigTermMonitoringEnabled = true + } + try terminate() + + let rawReport = try readPartialCrashReport() + try rawReport.validate() + XCTAssertEqual(rawReport.crash?.error?.signal?.name, "SIGTERM") + + let appleReport = try launchAndReportCrash() + print(appleReport) + XCTAssertTrue(appleReport.contains("SIGTERM")) + } +} + +#endif + extension PartialCrashReport { func validate() throws { let crashedThread = self.crash?.threads?.first(where: { $0.crashed }) diff --git a/Sources/KSCrashRecording/KSCrashC.c b/Sources/KSCrashRecording/KSCrashC.c index 5b2d18fb..8386e842 100644 --- a/Sources/KSCrashRecording/KSCrashC.c +++ b/Sources/KSCrashRecording/KSCrashC.c @@ -194,6 +194,7 @@ void handleConfiguration(KSCrashCConfiguration *configuration) #endif ksccd_setSearchQueueNames(configuration->enableQueueNameSearch); kscrashreport_setIntrospectMemory(configuration->enableMemoryIntrospection); + kscm_signal_sigterm_setMonitoringEnabled(configuration->enableSigTermMonitoring); if (configuration->doNotIntrospectClasses.strings != NULL) { kscrashreport_setDoNotIntrospectClasses(configuration->doNotIntrospectClasses.strings, diff --git a/Sources/KSCrashRecording/KSCrashConfiguration.m b/Sources/KSCrashRecording/KSCrashConfiguration.m index 0a985b49..f067db1d 100644 --- a/Sources/KSCrashRecording/KSCrashConfiguration.m +++ b/Sources/KSCrashRecording/KSCrashConfiguration.m @@ -60,6 +60,7 @@ - (instancetype)init _addConsoleLogToReport = cConfig.addConsoleLogToReport ? YES : NO; _printPreviousLogOnStartup = cConfig.printPreviousLogOnStartup ? YES : NO; _enableSwapCxaThrow = cConfig.enableSwapCxaThrow ? YES : NO; + _enableSigTermMonitoring = cConfig.enableSigTermMonitoring ? YES : NO; _reportStoreConfiguration = [KSCrashReportStoreConfiguration new]; _reportStoreConfiguration.appName = nil; @@ -102,6 +103,7 @@ - (KSCrashCConfiguration)toCConfiguration config.addConsoleLogToReport = self.addConsoleLogToReport; config.printPreviousLogOnStartup = self.printPreviousLogOnStartup; config.enableSwapCxaThrow = self.enableSwapCxaThrow; + config.enableSigTermMonitoring = self.enableSigTermMonitoring; return config; } @@ -154,6 +156,7 @@ - (nonnull id)copyWithZone:(nullable NSZone *)zone copy.addConsoleLogToReport = self.addConsoleLogToReport; copy.printPreviousLogOnStartup = self.printPreviousLogOnStartup; copy.enableSwapCxaThrow = self.enableSwapCxaThrow; + copy.enableSigTermMonitoring = self.enableSigTermMonitoring; return copy; } diff --git a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.h b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.h index 97b769df..792ee9c3 100644 --- a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.h +++ b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.h @@ -121,6 +121,12 @@ void ksmemory_set_fatal_reports_enabled(bool enabled); */ bool ksmemory_get_fatal_reports_enabled(void); +/** Notifies memory monitoring logic that there was an unhandled fatal signal. + * E.g. SIGTERM is not considered as a crash by-default but ignoring this signal + * causes false-positive OOM reports. + */ +void ksmemory_notifyUnhandledFatalSignal(void); + #ifdef __cplusplus } #endif diff --git a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.m b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.m index 6420cc92..e0144a3e 100644 --- a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.m +++ b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Memory.m @@ -569,3 +569,5 @@ bool ksmemory_previous_session_was_terminated_due_to_memory(bool *userPerceptibl void ksmemory_set_fatal_reports_enabled(bool enabled) { g_FatalReportsEnabled = enabled; } bool ksmemory_get_fatal_reports_enabled(void) { return g_FatalReportsEnabled; } + +void ksmemory_notifyUnhandledFatalSignal(void) { g_memory->fatal = true; } diff --git a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.c b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.c index cd893d71..fdca2046 100644 --- a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.c +++ b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.c @@ -30,6 +30,7 @@ #include "KSCrashMonitorContextHelper.h" #include "KSCrashMonitorHelper.h" #include "KSCrashMonitor_MachException.h" +#include "KSCrashMonitor_Memory.h" #include "KSID.h" #include "KSMachineContext.h" #include "KSSignalInfo.h" @@ -52,6 +53,7 @@ // ============================================================================ static volatile bool g_isEnabled = false; +static bool g_sigterm_monitoringEnabled = false; static KSCrash_MonitorContext g_monitorContext; static KSStackCursor g_stackCursor; @@ -66,6 +68,13 @@ static struct sigaction *g_previousSignalHandlers = NULL; static char g_eventID[37]; +// ============================================================================ +#pragma mark - Private - +// ============================================================================ + +static void uninstallSignalHandler(void); +static bool shouldHandleSignal(int sigNum) { return !(sigNum == SIGTERM && !g_sigterm_monitoringEnabled); } + // ============================================================================ #pragma mark - Callbacks - // ============================================================================ @@ -85,7 +94,7 @@ static char g_eventID[37]; static void handleSignal(int sigNum, siginfo_t *signalInfo, void *userContext) { KSLOG_DEBUG("Trapped signal %d", sigNum); - if (g_isEnabled) { + if (g_isEnabled && shouldHandleSignal(sigNum)) { thread_act_array_t threads = NULL; mach_msg_type_number_t numThreads = 0; ksmc_suspendEnvironment(&threads, &numThreads); @@ -110,6 +119,9 @@ static void handleSignal(int sigNum, siginfo_t *signalInfo, void *userContext) kscm_handleException(crashContext); ksmc_resumeEnvironment(threads, numThreads); + } else { + uninstallSignalHandler(); + ksmemory_notifyUnhandledFatalSignal(); } KSLOG_DEBUG("Re-raising signal for regular handlers to catch."); @@ -231,6 +243,13 @@ static void addContextualInfoToEvent(struct KSCrash_MonitorContext *eventContext #endif /* KSCRASH_HAS_SIGNAL */ +void kscm_signal_sigterm_setMonitoringEnabled(bool enabled) +{ +#if KSCRASH_HAS_SIGNAL + g_sigterm_monitoringEnabled = enabled; +#endif +} + KSCrashMonitorAPI *kscm_signal_getAPI(void) { #if KSCRASH_HAS_SIGNAL diff --git a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.h b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.h index c9cc296f..d110d0ae 100644 --- a/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.h +++ b/Sources/KSCrashRecording/Monitors/KSCrashMonitor_Signal.h @@ -40,6 +40,13 @@ extern "C" { */ KSCrashMonitorAPI *kscm_signal_getAPI(void); +/** Enables or disables SIGTERM monitoring. + * Default to false. + * + * @param enabled if true, SIGTERM signals will be monitored and reported. + */ +void kscm_signal_sigterm_setMonitoringEnabled(bool enabled); + #ifdef __cplusplus } #endif diff --git a/Sources/KSCrashRecording/include/KSCrashCConfiguration.h b/Sources/KSCrashRecording/include/KSCrashCConfiguration.h index 26f903e3..3df76c73 100644 --- a/Sources/KSCrashRecording/include/KSCrashCConfiguration.h +++ b/Sources/KSCrashRecording/include/KSCrashCConfiguration.h @@ -209,6 +209,17 @@ typedef struct { * **Default**: true */ bool enableSwapCxaThrow; + + /** If true, enables monitoring for SIGTERM signals. + * + * A SIGTERM is usually sent to the application by the OS during a graceful shutdown, + * but it can also happen on some Watchdog events. + * Enabling this can provide more insights into the cause of the SIGTERM, but + * it can also generate many false-positive crash reports. + * + * **Default**: false + */ + bool enableSigTermMonitoring; } KSCrashCConfiguration; static inline KSCrashCConfiguration KSCrashCConfiguration_Default(void) @@ -226,6 +237,7 @@ static inline KSCrashCConfiguration KSCrashCConfiguration_Default(void) .addConsoleLogToReport = false, .printPreviousLogOnStartup = false, .enableSwapCxaThrow = true, + .enableSigTermMonitoring = false, }; } diff --git a/Sources/KSCrashRecording/include/KSCrashConfiguration.h b/Sources/KSCrashRecording/include/KSCrashConfiguration.h index 4bca1c48..11bb595c 100644 --- a/Sources/KSCrashRecording/include/KSCrashConfiguration.h +++ b/Sources/KSCrashRecording/include/KSCrashConfiguration.h @@ -152,6 +152,17 @@ NS_ASSUME_NONNULL_BEGIN */ @property(nonatomic, assign) BOOL enableSwapCxaThrow; +/** If true, enables monitoring for SIGTERM signals. + * + * A SIGTERM is usually sent to the application by the OS during a graceful shutdown, + * but it can also happen on some Watchdog events. + * Enabling this can provide more insights into the cause of the SIGTERM, but + * it can also generate many false-positive crash reports. + * + * **Default**: false + */ +@property(nonatomic, assign) BOOL enableSigTermMonitoring; + @end NS_SWIFT_NAME(CrashReportStoreConfiguration)