Skip to content

Commit ab1c8b1

Browse files
committed
Fix critical safety bugs in lock, mount, watchdog
- CMLock: add post-write PID verification to close TOCTOU race on orphaned lock takeover; replace mtime-based stuck detection with a persistent `stuck-since` file measuring uninterrupted U/D time to avoid killing legitimately long-running processes - MountService: treat `rclone lsf` failure as hard error instead of "not exists" to prevent overwriting existing backups; fix `--bwlimit` to pass raw bytes/s (mbps * 125_000) instead of binary-ambiguous `M` suffix - MountHealth: move `stopTimeMachineIfRunning` outside the `isMounted` branch in watchdog so TM is always stopped before any rclone kill/unmount; add `logPrefix` param for per-caller log attribution; fix success/timeout log inversion
1 parent ec91751 commit ab1c8b1

5 files changed

Lines changed: 145 additions & 45 deletions

File tree

‎mac-app/Sources/CloudMachineApp/Views/MachinesConfigView.swift‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,25 @@ struct MachinesConfigView: View {
122122
)
123123
.font(.caption2)
124124
.foregroundStyle(.secondary)
125+
126+
// WAZNE: `--bwlimit` jest wpisywany w argumenty demona rclone
127+
// TYLKO przy starcie (`startRcloneDaemon`) - nie ma interfejsu
128+
// rc/reload, wiec zmiana suwaka NIE dziala na juz dzialajacy
129+
// proces. Automatyczny remount przy kazdym tyknieciu suwaka
130+
// podczas przeciagania bylby gorszy (wielokrotne, niepotrzebne
131+
// przerywanie aktywnego backupu) - zamiast tego jawnie mowimy
132+
// uzytkownikowi, kiedy zmiana faktycznie zacznie obowiazywac.
133+
if controller.status.mountState == .mounted {
134+
HStack(spacing: 4) {
135+
Image(systemName: "info.circle")
136+
.foregroundStyle(.secondary)
137+
Text(
138+
"Zmiana zacznie obowiązywać po następnym odmontowaniu i zamontowaniu (ręcznym lub automatycznej naprawie)."
139+
)
140+
}
141+
.font(.caption2)
142+
.foregroundStyle(.secondary)
143+
}
125144
}
126145

127146
Divider()

‎mac-app/Sources/CloudMachineCore/CMLock.swift‎

Lines changed: 60 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,23 @@ public final class CMLock {
4141
return false
4242
}
4343
writePid()
44+
// WAZNE: `removeItem` + `createDirectory` powyzej NIE jest atomowe -
45+
// dwa procesy przejmujace ta sama osierocona blokade w nakladajacym
46+
// sie oknie moga obie odczytac "martwy PID", obie usunac i utworzyc
47+
// katalog na nowo, obie zapisac swoj PID - i obie zwrocic `true`.
48+
// Odczytujemy wlasnie zapisany plik z powrotem: jesli inny proces
49+
// zdazyl go nadpisac swoim PID-em pomiedzy naszym `writePid()` a tym
50+
// odczytem, wiemy, ze przegralismy wyscig, i wycofujemy sie zamiast
51+
// dzialac w falszywym przekonaniu o wylacznosci. Nie eliminuje to
52+
// calkowicie okna (obie strony moga jeszcze przejsciowo "wygrac" tuz
53+
// przed ta weryfikacja), ale gwarantuje, ze co najmniej jedna z nich
54+
// to wykryje i cofnie.
55+
guard
56+
let verifyPidString = try? String(contentsOf: pidFile, encoding: .utf8),
57+
pid_t(verifyPidString.trimmingCharacters(in: .whitespacesAndNewlines)) == getpid()
58+
else {
59+
return false
60+
}
4461
acquired = true
4562
return true
4663
}
@@ -57,27 +74,54 @@ public final class CMLock {
5774
/// (dogananie kolejki uploadow, weryfikacja checksumow mogace trwac godziny).
5875
private static let stuckLockThreshold: TimeInterval = 15 * 60 // 15 minut
5976

77+
/// Znacznik "od kiedy nieprzerwanie widzimy ten proces w stanie U/D" -
78+
/// CELOWO osobny od mtime `lockDir` (czasu PRZEJECIA blokady). Legalna
79+
/// dlugotrwala operacja (weryfikacja checksumow trwajaca godziny) moze
80+
/// trzymac te sama blokade dlugo PRZED tym, jak w ogole wpadnie w U/D -
81+
/// liczenie progu od czasu przejecia blokady (jak robil wczesniejszy kod)
82+
/// zabija taki legalny, dlugo dzialajacy proces przy pierwszej probce w
83+
/// U/D po uplywie progu, co jest dokladnie tym, przed czym ostrzega
84+
/// komentarz w naglowku tego pliku (orphan detection PO PID, NIE po
85+
/// wieku). Ten znacznik zapisujemy przy PIERWSZYM zaobserwowanym U/D i
86+
/// kasujemy, gdy proces wroci do normalnego stanu - wiec mierzy faktyczny,
87+
/// NIEPRZERWANY czas trwania zawieszenia.
88+
private var stuckSinceFile: URL { lockDir.appendingPathComponent("stuck-since") }
89+
6090
private func isProcessAlive(_ pid: pid_t) -> Bool {
6191
guard kill(pid, 0) == 0 else { return false }
6292

6393
// Synchronicznie pobierz stan procesu z `ps`.
64-
if let psState = processState(pid: pid) {
65-
// Stan 'U' (macOS uninterruptible NFS wait) lub 'D' (Linux disk sleep)
66-
// - oba oznaczaja zawieszenie w jadrze, z ktorego SIGKILL nie wybudza.
67-
if psState.uppercased().hasPrefix("U") || psState.uppercased().hasPrefix("D") {
68-
// Czy blokada czeka juz od ponad progu? mtime lock.d == czas przejecia.
69-
if isLockDirOlderThan(seconds: Self.stuckLockThreshold) {
70-
CMLogger.log(
71-
"[CMLock] OSTRZEZENIE: PID \(pid) trzymajacy blokade '\(lockDir.lastPathComponent)'"
72-
+ " jest zawieszony w stanie U (NFS hang?) od ponad \(Int(Self.stuckLockThreshold/60)) min."
73-
+ " Przejmuje blokade. SIGKILL na zawieszony proces (ignorowany przez jadro, ale"
74-
+ " sprzatnie PID po odmontowaniu)."
75-
)
76-
kill(pid, SIGKILL)
77-
return false
78-
}
79-
}
94+
// Stan 'U' (macOS uninterruptible NFS wait) lub 'D' (Linux disk sleep)
95+
// - oba oznaczaja zawieszenie w jadrze, z ktorego SIGKILL nie wybudza.
96+
guard let psState = processState(pid: pid),
97+
psState.uppercased().hasPrefix("U") || psState.uppercased().hasPrefix("D")
98+
else {
99+
// Proces dziala normalnie (albo `ps` chwilowo nie odpowiedzialo) -
100+
// kasujemy znacznik, gdyby proces przejsciowo wpadl w U/D i wrocil.
101+
try? FileManager.default.removeItem(at: stuckSinceFile)
102+
return true
80103
}
104+
105+
if let stuckSinceString = try? String(contentsOf: stuckSinceFile, encoding: .utf8),
106+
let stuckSinceEpoch = TimeInterval(
107+
stuckSinceString.trimmingCharacters(in: .whitespacesAndNewlines))
108+
{
109+
let stuckDuration = Date().timeIntervalSince1970 - stuckSinceEpoch
110+
guard stuckDuration > Self.stuckLockThreshold else { return true }
111+
CMLogger.log(
112+
"[CMLock] OSTRZEZENIE: PID \(pid) trzymajacy blokade '\(lockDir.lastPathComponent)'"
113+
+ " jest NIEPRZERWANIE zawieszony w stanie U/D (NFS hang?) od ponad"
114+
+ " \(Int(Self.stuckLockThreshold/60)) min. Przejmuje blokade. SIGKILL na zawieszony"
115+
+ " proces (ignorowany przez jadro, ale sprzatnie PID po odmontowaniu)."
116+
)
117+
kill(pid, SIGKILL)
118+
try? FileManager.default.removeItem(at: stuckSinceFile)
119+
return false
120+
}
121+
// Pierwsza probka w stanie U/D - zapisujemy poczatek okna i czekamy na
122+
// kolejne probki, zanim uznamy proces za utkniety.
123+
try? "\(Date().timeIntervalSince1970)".write(
124+
to: stuckSinceFile, atomically: true, encoding: .utf8)
81125
return true
82126
}
83127

@@ -100,17 +144,6 @@ public final class CMLock {
100144
return String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines)
101145
}
102146

103-
/// Zwraca `true` jesli katalog blokady (lock.d) ma mtime starszy niz `seconds`.
104-
/// mtime katalogu == czas przejecia blokady przez mkdir() - nie jest pozniej
105-
/// modyfikowany, dobrze sluzy jako znacznik czasu przejecia.
106-
private func isLockDirOlderThan(seconds: TimeInterval) -> Bool {
107-
guard
108-
let attrs = try? FileManager.default.attributesOfItem(atPath: lockDir.path),
109-
let mtime = attrs[.modificationDate] as? Date
110-
else { return false }
111-
return Date().timeIntervalSince(mtime) > seconds
112-
}
113-
114147
private func writePid() {
115148
let pidFile = lockDir.appendingPathComponent("pid")
116149
try? "\(getpid())".write(to: pidFile, atomically: true, encoding: .utf8)

‎mac-app/Sources/CloudMachineCore/MountHealth.swift‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -170,24 +170,36 @@ public enum MountHealth {
170170
/// sposob ~49GB juz przeslanego backupu w trakcie stall-retry). KAZDY
171171
/// wywolujacy kod, ktory moze zabic/wymusic odmontowanie rclone dla tego
172172
/// remote, powinien wywolac to NAJPIERW. `tmutil stopbackup` NIE wymaga sudo.
173-
public static func stopTimeMachineIfRunning() async {
173+
/// `logPrefix` pozwala wywolujacemu zachowac wlasna konwencje logow (np.
174+
/// mount-watchdog prefiksuje kazda linie "[mount-watchdog] ") - bez tego
175+
/// te linie wypadaly z atrybucji przy grepowaniu logu per-watchdog.
176+
public static func stopTimeMachineIfRunning(logPrefix: String = "") async {
174177
guard await TimeMachineStatus.isRunning() else { return }
178+
// WAZNE: celowo "przed wymuszonym odmontowaniem/zabiciem rclone", NIE
179+
// "przed montowaniem" - ta funkcja jest wywolywana zarowno przed mount-
180+
// owaniem (MountService), jak i przed czystym odmontowaniem
181+
// (UnmountService) - wczesniejsza tresc mowila zawsze o "montowaniu",
182+
// co bylo faktycznie mylace podczas odmontowania.
175183
CMLogger.log(
176-
"Time Machine aktywnie kopiuje - zatrzymuje backup przed wymuszona operacja na montowaniu, zeby nie uszkodzic sparsebundle."
184+
"\(logPrefix)Time Machine aktywnie kopiuje - zatrzymuje backup przed wymuszonym odmontowaniem/zabiciem rclone, zeby nie uszkodzic sparsebundle."
177185
)
178186
_ = try? await ProcessRunner.run("/usr/bin/tmutil", ["stopbackup"], timeout: 20)
179187
var waited = 0
188+
var stopped = false
180189
while waited < 30 {
181-
if !(await TimeMachineStatus.isRunning()) { break }
190+
if !(await TimeMachineStatus.isRunning()) {
191+
stopped = true
192+
break
193+
}
182194
try? await Task.sleep(nanoseconds: 2_000_000_000)
183195
waited += 2
184196
}
185-
if await TimeMachineStatus.isRunning() {
197+
if stopped {
198+
CMLogger.log("\(logPrefix)Backup zatrzymany po \(waited)s.")
199+
} else {
186200
CMLogger.log(
187-
"OSTRZEZENIE: Time Machine nie zatrzymalo sie po \(waited)s - kontynuuje mimo to, ryzyko uszkodzenia sparsebundle."
201+
"\(logPrefix)OSTRZEZENIE: Time Machine nie zatrzymalo sie po \(waited)s - kontynuuje mimo to, ryzyko uszkodzenia sparsebundle."
188202
)
189-
} else {
190-
CMLogger.log("Backup zatrzymany po \(waited)s.")
191203
}
192204
}
193205

‎mac-app/Sources/CloudMachineCore/MountService.swift‎

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -77,15 +77,34 @@ public enum MountService {
7777
// konfiguracyjnych bezposrednio przez NFS).
7878
let sparsebundlePath = "\(remotePath)/backup.sparsebundle"
7979
let listResult = try? await ProcessRunner.runRclone(["lsf", sparsebundlePath])
80-
// WAZNE: `rclone lsf` zwraca exit 0 z PUSTYM stdout gdy folder istnieje,
81-
// ale jest pusty (np. po `rclone delete` ktory zostawia katalog bez plikow).
80+
// WAZNE: `try?`/`succeeded != true` NIE ROZROZNIA "sprawdzenie sie nie
81+
// udalo" (blad sieci, przejsciowy 429 z Google Drive API, timeout) od
82+
// "potwierdzone, ze pusto/nie istnieje". Traktowanie porazki sprawdzenia
83+
// jako "nie istnieje" jest niebezpieczne - ponizej kod TWORZY nowy pusty
84+
// sparsebundle i `rclone copy` go NA WIERZCH istniejacej sciezki, co przy
85+
// prawdziwym, w pelni przeslanym backupie nadpisuje/kasuje jego dane.
86+
// Dlatego przerywamy montowanie zamiast zgadywac, gdy samo sprawdzenie
87+
// zawiodlo - "nie wiem" nie moze pociagac za soba destrukcyjnej akcji.
88+
guard let listResult else {
89+
CMLogger.log(
90+
"BLAD: nie udalo sie sprawdzic czy sparsebundle istnieje w chmurze (blad polaczenia z rclone) - przerywam, zeby nie ryzykowac nadpisania istniejacego backupu."
91+
)
92+
return false
93+
}
94+
guard listResult.succeeded else {
95+
CMLogger.log(
96+
"BLAD: 'rclone lsf' zakonczylo sie bledem podczas sprawdzania sparsebundle w chmurze - przerywam, zeby nie ryzykowac nadpisania istniejacego backupu. Sprobuje ponownie przy nastepnym cyklu."
97+
)
98+
return false
99+
}
100+
// `rclone lsf` zwraca exit 0 z PUSTYM stdout gdy folder istnieje, ale
101+
// jest pusty (np. po `rclone delete` ktory zostawia katalog bez plikow).
82102
// Pusty folder != istniejacy sparsebundle - trzeba sprawdzic czy lsf zwrocil
83103
// jakikolwiek content (poprawny sparsebundle ZAWSZE ma co najmniej Info.plist).
84104
// Bez tego warunku MountService pomijal tworzenie nowego sparsebundle i probowal
85105
// zamontowac pusty folder -> "hdiutil: image not recognized" w nieskonczonosc.
86106
let sparsebundleExists =
87-
listResult?.succeeded == true
88-
&& !(listResult?.stdout.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty ?? true)
107+
!listResult.stdout.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty
89108

90109
if !sparsebundleExists {
91110
guard let limitGB = config.limitGB(forMachineKey: machineKey) else {
@@ -276,14 +295,20 @@ public enum MountService {
276295
}
277296
}
278297

279-
/// Konwertuje Mbps (megabity/s - jednostka, w ktorej dostawcy internetu
280-
/// podaja predkosc lacza) na argument `--bwlimit` rclone, ktory oczekuje
281-
/// megabajtow/s (dzielimy przez 8). `nil`, gdy limit wylaczony (<=0) - dla
282-
/// symetrii z `Optional` zamiast magicznej wartosci w liscie argumentow.
298+
/// Konwertuje Mbps (megabity/s dziesietnie - jednostka, w ktorej dostawcy
299+
/// internetu podaja predkosc lacza, 1 Mbps = 1_000_000 b/s) na argument
300+
/// `--bwlimit` rclone w BAJTACH/s (liczba bez sufiksu). WAZNE: rclone
301+
/// interpretuje sufiksy "M"/"k"/"G" binarnie (1024^n), NIE dziesietnie -
302+
/// gdyby przekazac np. "12.50M" dla 100 Mbps, rclone odczytalby to jako
303+
/// 12.5 MiB/s (~13.1 MB/s), czyli limit ~4.9% wyzszy niz oczekiwany.
304+
/// Podanie surowej liczby bajtow/s omija te niejednoznacznosc calkowicie
305+
/// i jest przy tym dokladne (mbps * 125_000 to zawsze liczba calkowita).
306+
/// `nil`, gdy limit wylaczony (<=0) - dla symetrii z `Optional` zamiast
307+
/// magicznej wartosci w liscie argumentow.
283308
private static func bwLimitArg(mbps: Int) -> String? {
284309
guard mbps > 0 else { return nil }
285-
let megabytesPerSecond = Double(mbps) / 8
286-
return String(format: "%.2fM", megabytesPerSecond)
310+
let bytesPerSecond = mbps * 125_000
311+
return String(bytesPerSecond)
287312
}
288313

289314
private static func startRcloneDaemon(remotePath: String, localDir: URL, bwLimitMbps: Int)

‎mac-app/Sources/CloudMachineCore/MountWatchdogService.swift‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,14 +212,25 @@ public enum MountWatchdogService {
212212
// jest zamontowane, NIE wolno tu z wyprzedzeniem zabijac rclone -
213213
// moglibysmy trafic w sam srodek juz trwajacego, zdrowego montowania.
214214
if nfsListed {
215+
// WAZNE: wolamy to BEZWARUNKOWO, przed jakimkolwiek demontazem/killem
216+
// ponizej - nie tylko przed `hdiutil detach` na sparsebundle. rclone
217+
// moze trzymac brudny `--vfs-write-back` cache z wczesniejszego
218+
// okresu, gdy sparsebundle BYL zamontowany i Time Machine aktywnie
219+
// pisal, nawet jesli TERAZ (w tym przebiegu) `spMount` juz nie jest
220+
// zamontowany - `killRcloneForRemote` ponizej i tak przerwie upload
221+
// tych zaleglych danych. Wczesniej to wywolanie bylo zagniezdzone
222+
// tylko w gałęzi `isMounted(spMount.path)`, wiec `forceUnmount`/
223+
// `killRcloneForRemote` dla samego NFS mogly wykonac sie bez ochrony -
224+
// dokladnie ten sam typ bledu, ktory ta funkcja ma zapobiegac (patrz
225+
// incydent z utrata ~49GB backupu).
226+
await MountHealth.stopTimeMachineIfRunning(logPrefix: "[mount-watchdog] ")
215227
if await MountHealth.isMounted(spMount.path) {
216228
// WAZNE: jesli Time Machine akurat aktywnie pisze na ten wolumin,
217229
// `hdiutil detach -force` w trakcie zapisu potrafi uszkodzic
218230
// metadane sparsebundle - wtedy przy nastepnym mouncie wyglada to
219231
// jak "zniknal z chmury" i cala historia backupu zaczyna sie od
220232
// zera. Dajemy Time Machine szanse grzecznie sie zatrzymac, tak
221233
// jak robi to rowniez `UnmountService` przy recznym odmontowaniu.
222-
await MountHealth.stopTimeMachineIfRunning()
223234
_ = try? await ProcessRunner.run("/usr/bin/hdiutil", ["detach", "-force", spMount.path])
224235
await MountHealth.forceUnmount(spMount.path, timeoutS: 10)
225236
}

0 commit comments

Comments
 (0)