Skip to content

fix(p2p): serialize access to p2pCtx/p2pCancel (#10839)#10861

Open
Anai-Guo wants to merge 1 commit into
mudler:masterfrom
Anai-Guo:fix-p2p-race-10839
Open

fix(p2p): serialize access to p2pCtx/p2pCancel (#10839)#10861
Anai-Guo wants to merge 1 commit into
mudler:masterfrom
Anai-Guo:fix-p2p-race-10839

Conversation

@Anai-Guo

Copy link
Copy Markdown
Contributor

Fixes #10839.

The race

Application.p2pCtx / p2pCancel are mutated from three places with inconsistent locking:

  • StopP2P() read and wrote both fields without ever taking a.p2pMutex.
  • StartP2P() reassigned both fields with no lock at all (p2p.go:40-41), and RestartP2P() invokes it from a background goroutine — so the write happens after RestartP2P has released the mutex.
  • The goroutine's error path read a.p2pCancel unlocked as well.

Both entry points are reachable from POST /api/settings (empty p2p_tokenStopP2P, non-empty → RestartP2P), so two concurrent requests race on the same struct fields (CWE-362), as the reporter's -race build shows.

The fix

  • StopP2P() now takes a.p2pMutex.
  • The teardown body is factored into stopP2PLocked() (documented as caller-holds-the-lock) so RestartP2P() reuses it instead of duplicating the cancel/clear/sleep block.
  • StartP2P() takes the mutex around the publication of ctx/cancel only. The node and discoverer setup below works off the local ctx variable, so the lock is deliberately not held across those network calls — this keeps StopP2P from blocking for the duration of a P2P bring-up.
  • The RestartP2P goroutine's error path now goes through StopP2P() rather than touching a.p2pCancel directly.

No deadlock is introduced: RestartP2P holds the mutex only until it returns, and it does not wait on the goroutine it spawns, so the goroutine's StartP2P lock acquisition just waits for that unlock.

This mirrors the mitmMutex convention already used in Application ("serializes Stop+Start").

🤖 Generated with Claude Code

StopP2P() read and wrote a.p2pCtx/a.p2pCancel without holding
a.p2pMutex, and StartP2P() reassigned both fields with no lock at
all -- including when RestartP2P() calls it from a background
goroutine after releasing the mutex. Both paths are reachable from
POST /api/settings (empty p2p_token -> StopP2P, non-empty ->
RestartP2P), so concurrent requests race on the same fields.

Take a.p2pMutex in StopP2P and around the field publication in
StartP2P, factor the shared teardown into stopP2PLocked() so
RestartP2P reuses it, and route the goroutine error path through
StopP2P instead of touching a.p2pCancel unlocked.

Signed-off-by: Anai-Guo <antai12232931@anaiguo.com>
@Anai-Guo
Anai-Guo force-pushed the fix-p2p-race-10839 branch from c06382a to 326f3f5 Compare July 16, 2026 13:07
@localai-bot

Copy link
Copy Markdown
Collaborator

Reviewed this against issue #10839. Verification done:

  • Grepped the whole repo for p2pCtx/p2pCancel/p2pMutex: all accesses live in core/application/p2p.go, and after this PR every one of them is under p2pMutex (StopP2P, the shared stopP2PLocked, the publish in StartP2P, RestartP2P, and the restart error path, which now goes through StopP2P instead of reading a.p2pCancel unguarded, which was itself a second unguarded access the old code had).
  • No deadlock: StartP2P only holds the lock for the two field assignments, not across the node/discoverer network calls, and the goroutine spawned by RestartP2P acquires the mutex fresh (it just blocks briefly if RestartP2P has not returned yet). The 200 ms shutdown sleep is now inside the lock, which serializes concurrent settings updates; that matches the intent and is bounded.
  • No lock copying or double unlock; stopP2PLocked's caller-must-hold contract is documented and both callers honor it.

One pre-existing quirk this PR does not (and does not need to) fix: because StartP2P runs async from RestartP2P, two rapid restarts can have the second publish overwrite the first cancel without invoking it, leaking the first node until the app context ends, and a StopP2P that lands before the async publish is a no-op. That race exists on master today and is orthogonal to the reported data race.

CI is green and the branch is mergeable. @mudler this looks good to merge: it closes the exact StopP2P/RestartP2P/StartP2P data race from the issue by putting every access to the shared fields under the existing mutex.

@localai-bot

Copy link
Copy Markdown
Collaborator

Reviewed against issue #10839 and the surrounding code in core/application.

Verdict: the race fix is correct and minimal. Good to merge from my side, pinging @mudler for the final call.

What I checked:

  • Coverage: p2pCtx/p2pCancel are declared in application.go and touched only in core/application/p2p.go. After this PR every access site is under p2pMutex: StopP2P (now locked), StartP2P (locked around the field publication), RestartP2P (already locked, teardown deduplicated into stopP2PLocked), and the restart goroutine's error path (now routed through StopP2P instead of reading a.p2pCancel unlocked). The exact read/write pair from the issue's -race report (StopP2P at p2p.go:21 vs RestartP2P at p2p.go:136) is gone. No unsynchronized accesses left.
  • Deadlock risk: none found. The lock is deliberately not held across the node/discoverer bring-up in StartP2P (it works off the local ctx), RestartP2P releases the mutex before its goroutine's StartP2P needs it, and StopP2P in the error path runs after StartP2P has returned, so there is no re-entrant acquisition. The 200ms sleep inside stopP2PLocked is held under the lock, but that matches the pre-existing RestartP2P behavior and just serializes stop-before-start, which is the point here. It also mirrors the existing mitmMutex convention.
  • Style: follows repo conventions (xlog for the new error log, locked-helper naming with a caller-must-hold comment).

Two notes, neither blocking:

  1. Scope: this fixes the data race (the DoS in -race builds and the memory-model violation in normal builds), which is what the issue reports as the bug. There is a residual logical race that predates this PR: two overlapping RestartP2P calls each fire a background StartP2P, and the second publication of ctx/cancel can overwrite the first, leaking a running node whose cancel func is lost. That is now safe memory-wise but still possible behaviorally. Fine to leave as a follow-up; just flagging it so it is a known limitation rather than an assumed fix.
  2. Auth: the issue also points out that POST /api/settings is effectively unauthenticated when no API keys / auth DB are configured (adminMiddleware resolves to a noop). That is LocalAI's general no-auth-by-default posture, a deployment/configuration concern separate from this race, and this PR correctly does not try to address it here. Worth tracking separately if the default should change.

One housekeeping nit: per .agents/ai-coding-assistants.md, AI-assisted commits should carry an Assisted-by: AGENT_NAME:MODEL_VERSION trailer in the commit message. The PR body discloses Claude Code but the commit itself lacks the trailer; would be nice to add on a rebase or let the maintainer squash with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug 1: Unauthenticated race condition in P2P settings update causes denial of service

2 participants