Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 7 additions & 2 deletions MenuBarItemService/Listener.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,13 @@ final class Listener: @unchecked Sendable {
diagLog.debug("Listener received start request")
return .start
case let .configureLogging(filePath):
DiagnosticLogger.shared.attachToFile(at: URL(fileURLWithPath: filePath))
diagLog.debug("Listener attached diagnostic logging to \(filePath)")
if let filePath {
DiagnosticLogger.shared.attachToFile(at: URL(fileURLWithPath: filePath))
diagLog.debug("Listener attached diagnostic logging to \(filePath)")
} else {
DiagnosticLogger.shared.isEnabled = false
diagLog.debug("Listener disabled diagnostic logging")
}
return .configureLogging
case let .sourcePID(window):
diagLog.debug("Listener: sourcePID request for windowID=\(window.windowID) title=\(window.title ?? "nil")")
Expand Down
5 changes: 4 additions & 1 deletion Shared/Services/MenuBarItemService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@ enum MenuBarItemService {
extension MenuBarItemService {
enum Request: Codable {
case start
case configureLogging(filePath: String)
/// Points the service's diagnostic logger at `filePath`, or disables it
/// when `nil`. Sent at startup, on each rotation, and when the user
/// toggles diagnostic logging.
case configureLogging(filePath: String?)
case sourcePID(WindowInfo)
case sourcePIDs([WindowInfo])
}
Expand Down
441 changes: 366 additions & 75 deletions Shared/Utilities/DiagnosticLogger.swift

Large diffs are not rendered by default.

7 changes: 7 additions & 0 deletions Thaw/Main/AppState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ final class AppState: ObservableObject {
let diagLog = DiagLog(category: "AppState")

private lazy var setupTask = Task { @MainActor in
// When the main app rotates to a new log file, hand its path to the XPC
// service so both processes keep appending to the same segment. Set
// before logging is enabled so the first rotation is handled.
DiagnosticLogger.shared.onRotate = { url in
Task { await MenuBarItemService.Connection.shared.configureLogging(to: url.path) }
}

#if DEBUG
// Debug builds always have diagnostic logging on so logs are
// captured during development without depending on the toggle.
Expand Down
85 changes: 69 additions & 16 deletions Thaw/MenuBar/MenuBarItems/MenuBarItemServiceConnection.swift
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,19 @@ extension MenuBarItemService {
/// The connection's diagnostic logger.
private let diagLog: DiagLog

/// Tracks the diagnostic log path last pushed to the service so a
/// failed push can be retried on the next request.
private struct LoggingState {
var hasDesired = false
var desiredPath: String?
var dirty = false
/// Bumped on every new desired state so a stale, out-of-order
/// response cannot clear a newer pending retry.
var generation = 0
}

private let loggingState = OSAllocatedUnfairLock(initialState: LoggingState())

/// Creates a new connection.
private init() {
let queue = DispatchQueue.targetingGlobal(
Expand All @@ -44,22 +57,16 @@ extension MenuBarItemService {
func start() async {
diagLog.debug("Starting MenuBarItemService connection")

// If the main app already has a diagnostic log file open,
// hand its path to the XPC service so both processes append
// to the same file. Sent before the start request so the
// XPC service is logging to disk by the time it handles any
// subsequent traffic. When file logging is off in the main
// app currentLogFile is nil and the XPC service simply logs
// to OSLog only, matching the prior release-build behaviour.
if let logFile = DiagnosticLogger.shared.currentLogFile {
let response = await session.sendAsync(request: .configureLogging(filePath: logFile.path))
if response == nil {
diagLog.error("configureLogging request returned nil")
} else if case .configureLogging = response {
// success
} else {
diagLog.error("configureLogging request returned invalid response \(String(describing: response))")
}
// Hand the service the diagnostic log path so both processes append
// to the same file. Sent before the start request so the XPC service
// is logging to disk by the time it handles any subsequent traffic.
// After the first push, `loggingState` holds the desired state
// (path, or nil to disable) and is replayed on reconnect.
let pending = loggingState.withLock { ($0.hasDesired, $0.desiredPath, $0.generation) }
if pending.0 {
await sendConfigureLogging(pending.1, generation: pending.2)
} else if let logFile = DiagnosticLogger.shared.currentLogFile {
await configureLogging(to: logFile.path)
}

let response = await session.sendAsync(request: .start)
Expand All @@ -74,6 +81,50 @@ extension MenuBarItemService {
}
}

/// Points the service's diagnostic logger at `filePath`, or disables it
/// when `nil`. Called at startup, on each log rotation, and when the
/// user toggles diagnostic logging.
func configureLogging(to filePath: String?) async {
let generation = loggingState.withLock { state -> Int in
state.hasDesired = true
state.desiredPath = filePath
state.generation += 1
return state.generation
}
await sendConfigureLogging(filePath, generation: generation)
}

/// Sends a single configureLogging request and records whether it needs
/// to be retried. Only the latest send (matching `generation`) may
/// update the dirty flag, so a stale response cannot clear a newer retry.
private func sendConfigureLogging(_ filePath: String?, generation: Int) async {
let response = await session.sendAsync(request: .configureLogging(filePath: filePath))
loggingState.withLock { state in
guard state.generation == generation else { return }
if case .configureLogging = response {
state.dirty = false
} else {
state.dirty = true
}
}
if response == nil {
diagLog.error("configureLogging request returned nil")
} else if case .configureLogging = response {
// success
} else {
diagLog.error("configureLogging request returned invalid response \(String(describing: response))")
}
}

/// Resends the desired logging configuration if a previous push failed.
private func resendLoggingIfDirty() async {
let pending = loggingState.withLock { state -> (Bool, String?, Int) in
(state.dirty, state.desiredPath, state.generation)
}
guard pending.0 else { return }
await sendConfigureLogging(pending.1, generation: pending.2)
}

/// Returns the source process identifier for the given window.
func sourcePID(for window: WindowInfo) async -> pid_t? {
let response = await session.sendAsync(request: .sourcePID(window))
Expand All @@ -93,6 +144,8 @@ extension MenuBarItemService {
/// single batch XPC request, avoiding concurrent thread explosion
/// in the XPC service.
func sourcePIDs(for windows: [WindowInfo]) async -> [pid_t?] {
// Opportunistically retry a failed logging push on this frequent path.
await resendLoggingIfDirty()
let response = await session.sendAsync(request: .sourcePIDs(windows))
guard let response else {
diagLog.error("Source PIDs batch request returned nil")
Expand Down
77 changes: 77 additions & 0 deletions Thaw/Settings/Models/AdvancedSettings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,38 @@
import Combine
import SwiftUI

// MARK: - LogRotationInterval

/// How often diagnostic logs are rotated on a time basis, in addition to the
/// size limit.
enum LogRotationInterval: String, CaseIterable, Identifiable {
case off
case hourly
case daily

var id: String {
rawValue
}

/// The interval in seconds, or `0` when time-based rotation is off.
var seconds: TimeInterval {
switch self {
case .off: 0
case .hourly: 3600
case .daily: 86400
}
}

/// A human-readable title for the picker.
var title: String {
switch self {
case .off: String(localized: "Off")
case .hourly: String(localized: "Hourly")
case .daily: String(localized: "Daily")
}
}
}

// MARK: - AdvancedSettings

/// Model for the app's Advanced settings.
Expand Down Expand Up @@ -55,6 +87,16 @@ final class AdvancedSettings: ObservableObject {
/// A Boolean value that indicates whether diagnostic logging to file is enabled.
@Published var enableDiagnosticLogging = Defaults.DefaultValue.enableDiagnosticLogging

/// The maximum size, in megabytes, a diagnostic log file may reach before
/// it is rotated.
@Published var diagnosticLogMaxSizeMB = Defaults.DefaultValue.diagnosticLogMaxSizeMB

/// The number of days to keep diagnostic log files before deleting them.
@Published var diagnosticLogRetentionDays = Defaults.DefaultValue.diagnosticLogRetentionDays

/// How often diagnostic logs are rotated on a time basis.
@Published var diagnosticLogRotationInterval = Defaults.DefaultValue.diagnosticLogRotationInterval

/// A Boolean value that indicates whether to use LCS sorting instead of
/// full sorting on notched displays.
@Published var useLCSSortingOnNotchedDisplays = Defaults.DefaultValue.useLCSSortingOnNotchedDisplays
Expand Down Expand Up @@ -107,6 +149,13 @@ final class AdvancedSettings: ObservableObject {
Defaults.ifPresent(key: .showMenuBarTooltips, assign: &showMenuBarTooltips)
Defaults.ifPresent(key: .iconRefreshInterval, assign: &iconRefreshInterval)
Defaults.ifPresent(key: .enableDiagnosticLogging, assign: &enableDiagnosticLogging)
Defaults.ifPresent(key: .diagnosticLogMaxSizeMB, assign: &diagnosticLogMaxSizeMB)
Defaults.ifPresent(key: .diagnosticLogRetentionDays, assign: &diagnosticLogRetentionDays)
Defaults.ifPresent(key: .diagnosticLogRotationInterval) { (rawValue: String) in
if let interval = LogRotationInterval(rawValue: rawValue) {
diagnosticLogRotationInterval = interval
}
}
Defaults.ifPresent(key: .useLCSSortingOnNotchedDisplays, assign: &useLCSSortingOnNotchedDisplays)
Defaults.ifPresent(key: .enableMenuBarItemOverflow, assign: &enableMenuBarItemOverflow)
Defaults.ifPresent(key: .searchIncludeVisible, assign: &searchIncludeVisible)
Expand Down Expand Up @@ -170,9 +219,37 @@ final class AdvancedSettings: ObservableObject {
#else
DiagnosticLogger.shared.isEnabled = enabled
#endif
// Push the new state to the XPC service so toggling after launch
// takes effect there too (nil disables its file logging).
let path = DiagnosticLogger.shared.currentLogFile?.path
Task { await MenuBarItemService.Connection.shared.configureLogging(to: path) }
},
in: &c
)
$diagnosticLogMaxSizeMB.persistToDefaults(key: .diagnosticLogMaxSizeMB, in: &c)
$diagnosticLogRetentionDays.persistToDefaults(key: .diagnosticLogRetentionDays, in: &c)
$diagnosticLogRotationInterval.persistToDefaults(
key: .diagnosticLogRotationInterval,
transform: \.rawValue,
in: &c
)
// Apply the rotation policy to the logger whenever any of its inputs
// change. CombineLatest3 also emits once on subscription, seeding the
// initial policy.
Publishers.CombineLatest3(
$diagnosticLogMaxSizeMB,
$diagnosticLogRetentionDays,
$diagnosticLogRotationInterval
)
.receive(on: DispatchQueue.main)
.sink { maxSizeMB, retentionDays, interval in
var policy = DiagnosticLogger.RotationPolicy()
policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024
policy.retentionDays = max(1, retentionDays)
Comment on lines +245 to +248

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify current bounds/sanitization path for diagnosticLogMaxSizeMB.
rg -n 'diagnosticLogMaxSizeMB|maxFileSizeBytes|CombineLatest3' Thaw/Settings/Models/AdvancedSettings.swift -C3
rg -n 'diagnosticLogMaxSizeMB' Thaw/Settings/Models/Profile.swift Thaw/Settings/SettingsPanes/AdvancedSettingsPane.swift -C2

Repository: stonerl/Thaw

Length of output: 7302


🏁 Script executed:

# Find DiagnosticLogger and check RotationPolicy definition
fd -t f DiagnosticLogger -e swift | head -5
rg -n "class RotationPolicy|struct RotationPolicy" -A 10

Repository: stonerl/Thaw

Length of output: 1176


🏁 Script executed:

# Check if there's any validation in the Defaults system for diagnosticLogMaxSizeMB
rg -n "DefaultValue.diagnosticLogMaxSizeMB|.diagnosticLogMaxSizeMB.*case" -B 2 -A 2

Repository: stonerl/Thaw

Length of output: 2953


Clamp before multiplication to prevent overflow in maxFileSizeBytes derivation.

At line 247, UInt64(max(0, maxSizeMB)) * 1024 * 1024 can overflow when maxSizeMB contains out-of-range values from persisted or tampered profiles, potentially crashing during policy recomputation. While the UI constrains input to 1–100 MB, the persisted Int value bypasses this constraint and reaches the sink without validation.

Proposed fix
         .sink { maxSizeMB, retentionDays, interval in
             var policy = DiagnosticLogger.RotationPolicy()
-            policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024
+            let bytesPerMB: UInt64 = 1_048_576
+            let normalizedMB = max(1, maxSizeMB)
+            let clampedMB = min(UInt64(normalizedMB), UInt64.max / bytesPerMB)
+            policy.maxFileSizeBytes = clampedMB * bytesPerMB
             policy.retentionDays = max(1, retentionDays)
             policy.rotationInterval = interval.seconds
             DiagnosticLogger.shared.setRotationPolicy(policy)
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
.sink { maxSizeMB, retentionDays, interval in
var policy = DiagnosticLogger.RotationPolicy()
policy.maxFileSizeBytes = UInt64(max(0, maxSizeMB)) * 1024 * 1024
policy.retentionDays = max(1, retentionDays)
.sink { maxSizeMB, retentionDays, interval in
var policy = DiagnosticLogger.RotationPolicy()
let bytesPerMB: UInt64 = 1_048_576
let normalizedMB = max(1, maxSizeMB)
let clampedMB = min(UInt64(normalizedMB), UInt64.max / bytesPerMB)
policy.maxFileSizeBytes = clampedMB * bytesPerMB
policy.retentionDays = max(1, retentionDays)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Thaw/Settings/Models/AdvancedSettings.swift` around lines 245 - 248, In the
sink block where maxFileSizeBytes is calculated, clamp the maxSizeMB value to a
safe maximum before performing the multiplication by 1024 * 1024. Instead of
converting and multiplying the raw maxSizeMB value directly, first constrain it
to a reasonable upper limit (such as 100 to match UI constraints) using the max
function, then cast to UInt64 and perform the multiplication. This prevents
integer overflow when persisted or tampered profile data contains out-of-range
values that bypass UI validation.

policy.rotationInterval = interval.seconds
DiagnosticLogger.shared.setRotationPolicy(policy)
}
.store(in: &c)
$useLCSSortingOnNotchedDisplays.persistToDefaults(key: .useLCSSortingOnNotchedDisplays, in: &c)
$enableMenuBarItemOverflow.persistToDefaults(key: .enableMenuBarItemOverflow, in: &c)
$searchSectionOrder.persistToDefaults(
Expand Down
32 changes: 32 additions & 0 deletions Thaw/Settings/Models/Profile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ struct AdvancedSettingsSnapshot: Codable {
var showMenuBarTooltips: Bool
var iconRefreshInterval: TimeInterval
var enableDiagnosticLogging: Bool
var diagnosticLogMaxSizeMB: Int
var diagnosticLogRetentionDays: Int
var diagnosticLogRotationInterval: String
var useDoubleClickToShowAlwaysHiddenSection: Bool
var useOptionClickToShowAlwaysHiddenSection: Bool
var useLCSSortingOnNotchedDisplays: Bool
Expand All @@ -123,6 +126,9 @@ struct AdvancedSettingsSnapshot: Codable {
showMenuBarTooltips: settings.showMenuBarTooltips,
iconRefreshInterval: settings.iconRefreshInterval,
enableDiagnosticLogging: settings.enableDiagnosticLogging,
diagnosticLogMaxSizeMB: settings.diagnosticLogMaxSizeMB,
diagnosticLogRetentionDays: settings.diagnosticLogRetentionDays,
diagnosticLogRotationInterval: settings.diagnosticLogRotationInterval.rawValue,
useDoubleClickToShowAlwaysHiddenSection: settings.useDoubleClickToShowAlwaysHiddenSection,
useOptionClickToShowAlwaysHiddenSection: settings.useOptionClickToShowAlwaysHiddenSection,
useLCSSortingOnNotchedDisplays: settings.useLCSSortingOnNotchedDisplays,
Expand All @@ -149,6 +155,11 @@ struct AdvancedSettingsSnapshot: Codable {
settings.showMenuBarTooltips = showMenuBarTooltips
settings.iconRefreshInterval = iconRefreshInterval
settings.enableDiagnosticLogging = enableDiagnosticLogging
settings.diagnosticLogMaxSizeMB = diagnosticLogMaxSizeMB
settings.diagnosticLogRetentionDays = diagnosticLogRetentionDays
if let interval = LogRotationInterval(rawValue: diagnosticLogRotationInterval) {
settings.diagnosticLogRotationInterval = interval
}
settings.useDoubleClickToShowAlwaysHiddenSection = useDoubleClickToShowAlwaysHiddenSection
settings.useOptionClickToShowAlwaysHiddenSection = useOptionClickToShowAlwaysHiddenSection
settings.useLCSSortingOnNotchedDisplays = useLCSSortingOnNotchedDisplays
Expand All @@ -171,6 +182,9 @@ struct AdvancedSettingsSnapshot: Codable {
case showMenuBarTooltips
case iconRefreshInterval
case enableDiagnosticLogging
case diagnosticLogMaxSizeMB
case diagnosticLogRetentionDays
case diagnosticLogRotationInterval
case useDoubleClickToShowAlwaysHiddenSection
case useOptionClickToShowAlwaysHiddenSection
case useLCSSortingOnNotchedDisplays
Expand All @@ -193,6 +207,9 @@ struct AdvancedSettingsSnapshot: Codable {
showMenuBarTooltips: Bool,
iconRefreshInterval: TimeInterval,
enableDiagnosticLogging: Bool,
diagnosticLogMaxSizeMB: Int = Defaults.DefaultValue.diagnosticLogMaxSizeMB,
diagnosticLogRetentionDays: Int = Defaults.DefaultValue.diagnosticLogRetentionDays,
diagnosticLogRotationInterval: String = Defaults.DefaultValue.diagnosticLogRotationInterval.rawValue,
useDoubleClickToShowAlwaysHiddenSection: Bool,
useOptionClickToShowAlwaysHiddenSection: Bool,
useLCSSortingOnNotchedDisplays: Bool,
Expand All @@ -213,6 +230,9 @@ struct AdvancedSettingsSnapshot: Codable {
self.showMenuBarTooltips = showMenuBarTooltips
self.iconRefreshInterval = iconRefreshInterval
self.enableDiagnosticLogging = enableDiagnosticLogging
self.diagnosticLogMaxSizeMB = diagnosticLogMaxSizeMB
self.diagnosticLogRetentionDays = diagnosticLogRetentionDays
self.diagnosticLogRotationInterval = diagnosticLogRotationInterval
self.useDoubleClickToShowAlwaysHiddenSection = useDoubleClickToShowAlwaysHiddenSection
self.useOptionClickToShowAlwaysHiddenSection = useOptionClickToShowAlwaysHiddenSection
self.useLCSSortingOnNotchedDisplays = useLCSSortingOnNotchedDisplays
Expand Down Expand Up @@ -258,6 +278,15 @@ struct AdvancedSettingsSnapshot: Codable {
enableDiagnosticLogging = try container.decodeIfPresent(
Bool.self, forKey: .enableDiagnosticLogging
) ?? Defaults.DefaultValue.enableDiagnosticLogging
diagnosticLogMaxSizeMB = try container.decodeIfPresent(
Int.self, forKey: .diagnosticLogMaxSizeMB
) ?? Defaults.DefaultValue.diagnosticLogMaxSizeMB
diagnosticLogRetentionDays = try container.decodeIfPresent(
Int.self, forKey: .diagnosticLogRetentionDays
) ?? Defaults.DefaultValue.diagnosticLogRetentionDays
diagnosticLogRotationInterval = try container.decodeIfPresent(
String.self, forKey: .diagnosticLogRotationInterval
) ?? Defaults.DefaultValue.diagnosticLogRotationInterval.rawValue
useDoubleClickToShowAlwaysHiddenSection = try container.decodeIfPresent(
Bool.self, forKey: .useDoubleClickToShowAlwaysHiddenSection
) ?? Defaults.DefaultValue.useDoubleClickToShowAlwaysHiddenSection
Expand Down Expand Up @@ -486,6 +515,9 @@ struct Profile: Codable, Identifiable {
showMenuBarTooltips: Defaults.DefaultValue.showMenuBarTooltips,
iconRefreshInterval: Defaults.DefaultValue.iconRefreshInterval,
enableDiagnosticLogging: Defaults.DefaultValue.enableDiagnosticLogging,
diagnosticLogMaxSizeMB: Defaults.DefaultValue.diagnosticLogMaxSizeMB,
diagnosticLogRetentionDays: Defaults.DefaultValue.diagnosticLogRetentionDays,
diagnosticLogRotationInterval: Defaults.DefaultValue.diagnosticLogRotationInterval.rawValue,
useDoubleClickToShowAlwaysHiddenSection: Defaults.DefaultValue.useDoubleClickToShowAlwaysHiddenSection,
useOptionClickToShowAlwaysHiddenSection: Defaults.DefaultValue.useOptionClickToShowAlwaysHiddenSection,
useLCSSortingOnNotchedDisplays: Defaults.DefaultValue.useLCSSortingOnNotchedDisplays,
Expand Down
3 changes: 3 additions & 0 deletions Thaw/Settings/Models/SettingsResetter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,9 @@ extension AppSettings {
advanced.showMenuBarTooltips = Defaults.DefaultValue.showMenuBarTooltips
advanced.iconRefreshInterval = Defaults.DefaultValue.iconRefreshInterval
advanced.enableDiagnosticLogging = Defaults.DefaultValue.enableDiagnosticLogging
advanced.diagnosticLogMaxSizeMB = Defaults.DefaultValue.diagnosticLogMaxSizeMB
advanced.diagnosticLogRetentionDays = Defaults.DefaultValue.diagnosticLogRetentionDays
advanced.diagnosticLogRotationInterval = Defaults.DefaultValue.diagnosticLogRotationInterval
advanced.searchSectionOrder = AdvancedSettings.sanitizedSearchSectionOrder(
from: Defaults.DefaultValue.searchSectionOrder
)
Expand Down
Loading