Skip to content

Commit 7d03f48

Browse files
myleshortonclaude
andcommitted
smc: defer subscription cleanup + strict arg validation across handlers
Six Copilot findings: 1. lantern-core/core.go's listenPeerConnectionEvents subscribed to unbounded.ConnectionEvent and started a goroutine waiting on ctx.Done to unsubscribe. If client.PeerConnectionEvents returned an error while ctx was still live, the function returned but the ctx-watcher goroutine + subscription leaked for the rest of the process. Replaced with 'defer unbSub.Unsubscribe()' so cleanup runs on both exit paths (normal ctx cancel + unexpected stream exit). 2. iOS handler comment said the SmC setter helpers use a 'detached Task' but the implementation uses 'Task {}'. Updated the comment to describe the actual choice — plain Task is fine for the millisecond-range PatchSettings calls because inheriting the current actor's executor is cheap; the probeUPnP case is the one exception that uses Task.detached because its M-SEARCH wait is multi-second. 3-5. macOS / iOS handlers had unsafe defaults on the SmC setters: - setPeerProxyEnabled: defaulted enabled=false on missing arg (would silently disable sharing on caller bugs) - setPeerManualPort: defaulted port=0 on missing arg (would silently clear the user's manual port override, since 0 has the real semantic of 'no manual port') - setUnboundedEnabled: same Bool-defaults-to-false issue (Copilot didn't flag this one but it has the identical problem) All four now route through requireArg, which surfaces a FlutterError on missing/invalid argument shape instead of defaulting silently. 6. Android setPeerManualPort defaulted port=0 the same way. Switched the elvis '?: 0' to '?: error("Missing port")' to match the SetPeerProxyEnabled pattern on Android. Go build clean. The Swift / Kotlin changes are mechanical and follow patterns already established in the same files (requireArg / error()-on-missing). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 690494b commit 7d03f48

4 files changed

Lines changed: 40 additions & 24 deletions

File tree

android/app/src/main/kotlin/org/getlantern/lantern/handler/MethodHandler.kt

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1251,7 +1251,11 @@ class MethodHandler : FlutterPlugin,
12511251

12521252
Methods.SetPeerManualPort.method -> {
12531253
scope.handleResult(result, "set_peer_manual_port") {
1254-
val port = call.argument<Int>("port") ?: 0
1254+
// error() surfaces the failure rather than silently
1255+
// defaulting to 0 — which would clear the user's
1256+
// manual port override on caller bugs. Matches the
1257+
// SetPeerProxyEnabled pattern above.
1258+
val port = call.argument<Int>("port") ?: error("Missing port")
12551259
Mobile.setPeerManualPort(port.toLong())
12561260
}
12571261
}

ios/Runner/Handlers/MethodHandler.swift

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -269,8 +269,10 @@ class MethodHandler {
269269
// the router. The whole stack is exposed on iOS rather than
270270
// desktop-only.
271271
case "setPeerProxyEnabled":
272-
let data = call.arguments as? [String: Any]
273-
let enabled = data?["enabled"] as? Bool ?? false
272+
// requireArg surfaces a FlutterError on missing/invalid
273+
// argument shape instead of silently defaulting to false
274+
// (which would disable sharing on caller bugs).
275+
guard let enabled: Bool = requireArg(call: call, name: "enabled", result: result) else { return }
274276
self.setPeerProxyEnabled(result: result, enabled: enabled)
275277

276278
case "isPeerProxyEnabled":
@@ -279,8 +281,12 @@ class MethodHandler {
279281
}
280282

281283
case "setPeerManualPort":
282-
let data = call.arguments as? [String: Any]
283-
let port = data?["port"] as? Int ?? 0
284+
// requireArg surfaces a FlutterError on missing/invalid
285+
// argument shape instead of silently defaulting to 0
286+
// (which has the real semantic of clearing the manual port
287+
// override — caller bugs would silently wipe the user's
288+
// setting).
289+
guard let port: Int = requireArg(call: call, name: "port", result: result) else { return }
284290
self.setPeerManualPort(result: result, port: port)
285291

286292
case "getPeerManualPort":
@@ -289,8 +295,7 @@ class MethodHandler {
289295
}
290296

291297
case "setUnboundedEnabled":
292-
let data = call.arguments as? [String: Any]
293-
let enabled = data?["enabled"] as? Bool ?? false
298+
guard let enabled: Bool = requireArg(call: call, name: "enabled", result: result) else { return }
294299
self.setUnboundedEnabled(result: result, enabled: enabled)
295300

296301
case "isUnboundedEnabled":
@@ -1173,8 +1178,12 @@ class MethodHandler {
11731178
}
11741179

11751180
// Share My Connection (samizdat) toggle. Mirrors the macOS handler's
1176-
// pattern: blocking Mobile call goes onto a detached Task; result
1177-
// and error delivery hop to MainActor.
1181+
// pattern: blocking Mobile call goes onto a Task; result and error
1182+
// delivery hop to MainActor. Plain Task (not Task.detached) is fine
1183+
// for these PatchSettings calls — they finish in milliseconds, so
1184+
// inheriting the current actor's executor is cheap. probeUPnP is
1185+
// the one exception that uses Task.detached because its M-SEARCH
1186+
// wait is multi-second.
11781187
func setPeerProxyEnabled(result: @escaping FlutterResult, enabled: Bool) {
11791188
Task {
11801189
var error: NSError?

lantern-core/core.go

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -425,11 +425,12 @@ func (lc *LanternCore) listenPeerConnectionEvents() {
425425
// hit here today. Worth revisiting if Unbounded ever moves out of
426426
// process.
427427
//
428-
// The subscription is tied to ctx via a separate goroutine that
429-
// calls Unsubscribe on Done. Without this, a reinitialization of
430-
// LanternCore over the process lifetime would leak handlers and
431-
// events.Emit would fan out each connection event to N stale
432-
// registrations.
428+
// defer Unsubscribe so cleanup runs on EITHER path that exits
429+
// this function: ctx cancel (normal) OR PeerConnectionEvents
430+
// returning an error (unexpected stream exit). The earlier
431+
// ctx-watching goroutine missed the error path and leaked both
432+
// the goroutine and the subscription whenever the SSE stream
433+
// died while ctx was still live.
433434
unbSub := events.Subscribe(func(evt unbounded.ConnectionEvent) {
434435
jsonBytes, err := json.Marshal(map[string]any{
435436
"state": evt.State,
@@ -442,10 +443,7 @@ func (lc *LanternCore) listenPeerConnectionEvents() {
442443
}
443444
lc.notifyFlutter(EventTypePeerConnection, string(jsonBytes))
444445
})
445-
go func() {
446-
<-lc.ctx.Done()
447-
unbSub.Unsubscribe()
448-
}()
446+
defer unbSub.Unsubscribe()
449447

450448
// peer.ConnectionEvent: subscribe via the IPC client's SSE stream.
451449
// The events package's globals are process-scoped — events.Emit in

macos/Runner/Handlers/MethodHandler.swift

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -251,13 +251,19 @@ class MethodHandler {
251251
}
252252

253253
case "setPeerProxyEnabled":
254-
let data = call.arguments as? [String: Any]
255-
let enabled = data?["enabled"] as? Bool ?? false
254+
// requireArg surfaces a FlutterError on missing/invalid
255+
// argument shape rather than silently defaulting to false
256+
// (which would disable sharing on caller bugs).
257+
guard let enabled: Bool = requireArg(call: call, name: "enabled", result: result) else { return }
256258
self.setPeerProxyEnabled(result: result, enabled: enabled)
257259

258260
case "setPeerManualPort":
259-
let data = call.arguments as? [String: Any]
260-
let port = data?["port"] as? Int ?? 0
261+
// requireArg surfaces a FlutterError on missing/invalid
262+
// argument shape rather than silently defaulting to 0
263+
// (which has the real semantic of clearing the manual port
264+
// override — caller bugs would silently wipe the user's
265+
// setting).
266+
guard let port: Int = requireArg(call: call, name: "port", result: result) else { return }
261267
self.setPeerManualPort(result: result, port: port)
262268

263269
case "getPeerManualPort":
@@ -266,8 +272,7 @@ class MethodHandler {
266272
}
267273

268274
case "setUnboundedEnabled":
269-
let data = call.arguments as? [String: Any]
270-
let enabled = data?["enabled"] as? Bool ?? false
275+
guard let enabled: Bool = requireArg(call: call, name: "enabled", result: result) else { return }
271276
self.setUnboundedEnabled(result: result, enabled: enabled)
272277

273278
case "isUnboundedEnabled":

0 commit comments

Comments
 (0)