Skip to content

fix(kbfs): don't crash on service notifications that arrive before KBFSOps is set - #29618

Open
chrisnojima wants to merge 10 commits into
masterfrom
nojima/HOTPOT-crash-fix
Open

fix(kbfs): don't crash on service notifications that arrive before KBFSOps is set#29618
chrisnojima wants to merge 10 commits into
masterfrom
nojima/HOTPOT-crash-fix

Conversation

@chrisnojima

@chrisnojima chrisnojima commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Problem

iOS crashes at launch with a Go nil-interface dereference. Xcode files these reports under its no-stack bucket: SetTraceback("crash") aborts from the Go runtime's own stack, so the Go frames never reach the report. The signatures are addr=0x140 (3 reports on 6.6.3, 1 on 6.7.0) and addr=0x158 (1 report on 6.6.3).

The 6.7.0 build writes the Go traceback to a file:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x140 ...]
libkbfs.(*KeybaseServiceBase).ReachabilityChanged  keybase_service_base.go:421
keybase1.ReachabilityProtocol.func2
rpc.(*notifyRequest).Serve

Cause

doInit created the keybase service connection before KBFSOps and MDOps existed. That connection registers KBFS's notification and request handlers, plus SimpleFS, KBFSGit and Fs, and the service can call them as soon as it's up. In that window, the handlers dereference a nil KBFSOps:

  • 0x140 is the itab slot for KBFSOps.PushConnectionStatusChange (method index 37), called from ReachabilityChanged.
  • 0x158 is KBFSOps.RefreshCachedFavorites (index 40), reached from FavoritesChanged or CurrentSessionserviceLoggedIn.

This is in 6.6.3

Between v6.6.3 and this PR's base, the init order, the handlers and the login/logout flows are unchanged. The KBFSOps interface block is byte-for-byte identical, so the itab offsets are the same. Running the pre-init scenarios against a v6.6.3 checkout reproduces the production signatures exactly:

Scenario at v6.6.3 Result
ReachabilityChanged before SetKBFSOps SIGSEGV addr=0x140
CurrentSessionserviceLoggedIn before SetKBFSOps SIGSEGV addr=0x158
FavoritesChanged before SetKBFSOps SIGSEGV addr=0x158

Fix

1. Set up what the handlers use before the service connection. doInit now creates KBPKI, KBFSOps, Notifier, KeyManager and MDOps, and sets up the disk limiter, before it creates the service connection.

  • Why it's safe: their constructors only store config. KBPKI reaches the service through config when it's called. NewKBFSOpsStandard starts two goroutines: the reidentify loop, which reads TLFValidDuration and Clock, and the debug dumper, which only touches its own state.
  • Why the disk limiter moved: the service can deliver a login, and a login can create the disk block cache, which expects the limiter. The limiter only reads local config.
  • What it removes: no handler can see a nil KBFSOps or MDOps, so they need no nil guards.

2. Handle what init still sets after the connection is live. That covers Chat, Crypto, the MD, key and block servers, the caches, the KBFS service and journaling.

  • Chat stays after the service, because chat notifications can reach KBPKI, which needs the service. serviceLoggedOut nil-checks it.
  • Init signals for the rest. KBFSOpsStandard now records init progress:
    • initReady() fires once Crypto, the servers, the caches and the KBFS service are set, just before journaling. It comes after the last step that can fail init (the sync batch size check), because requests it releases can't be recalled.
    • initDoneCh is closed only when init succeeds.
    • initFailedCh is closed if init fails.
  • SimpleFS, KBFSGit and Fs: every method of these protocols waits until KBFS is ready, bounded by the caller's context. A request fails with errKBFSNotInitialized if init failed. Journaling isn't waited for: its per-journal setup isn't bounded by its 60s timeout, and the service gives each SimpleFS call only 60s. The rpc receiver serves each request on its own goroutine, so waiting doesn't block the connection.
  • Service-initiated KBFS requests (GetTLFCryptKeys, GetPublicCanonicalTLFNameAndID, FSEditListRequest, StartMigration, FinalizeMigration): they check readiness first, without blocking, and return errKBFSNotInitialized until ready. That way none of them can block on an init that is itself waiting on the service. Chat treats that error as transient, and TLF upgrade logs it and retries later.
  • The edit-history and synced-TLF goroutines still wait for full init, and now return if init fails instead of running against a half-built config.
  • Compile-time check: KBFSOpsStandard must implement the gate's interface, so renaming its method can't silently open the gate.

Behavior changes during startup

  • Delayed calls: GUI SimpleFS calls that land before KBFS is ready now wait until it is, instead of running early and possibly crashing. The GUI marks KBFS connected when it first registers with the service, which is before init finishes. So online status, subscriptions, the files badge and settings can each be delayed by the local part of init.
  • Chat key lookups in that window get a transient error instead of a possible crash.

Not covered

  • After a failed init, the service connection stays registered. It uses a shared transport whose Close is a no-op. Requests that arrive afterwards fail fast.
  • Chat's connection keeps delivering notifications after a failed init. Those calls wait instead of crashing.
  • A brief window before init stores the service in config. Handlers are registered inside NewKeybaseDaemonRPC, and config.SetKeybaseService runs after it returns. Notifications are enabled after one round trip (SetNotifications). That window exists on master too.
  • Where initReady() sits within doInit isn't pinned by a test, because that would need a doInit that succeeds all the way through.

Also in this PR: text previews in hot dev

Separate from the crash fix, and dev-only. The hot-dev renderer loads from the Vite server (http://localhost:4000), so its text-preview XHRs to the KBFS http server are cross-origin, and that server sends no CORS headers, so previews fail to load. In hot dev only, the main window's session now adds Access-Control-Allow-Origin for the Vite origin to responses from the KBFS http server's /files/ paths. Packaged builds and cold dev load from file:// and are unaffected.

Testing

  • TestInitSetsUpKBFSBeforeService runs the real doInit with a fake service.

    • The fake calls into KBFS from inside NewKeybaseService (the earliest point), and again once init has set it: reachability, favorites, team changes, a login (CurrentSessionserviceLoggedIn), a paper-key rekey and a logout.
    • At both points it checks that service requests (GetTLFCryptKeys, StartMigration) get errKBFSNotInitialized, and that a gated SimpleFS-style request waits.
    • It then fails NewCrypto and checks that both kinds of request fail fast.
  • TestKeybaseDaemonRPCGatesAdditionalProtocolsOnInit checks the real NewKeybaseDaemonRPC wiring: a request waits while init is running, and goes through on initReady(), before the rest of init has finished.

  • Mutation checks. Each of these makes a test fail within seconds:

    • restoring the old init order, which reproduces the production panic, addr=0x140 in ReachabilityChanged
    • not signalling init failure
    • dropping the Chat nil check
    • forcing kbfsReady true
    • making the wrapper not wait
    • making initReady() a no-op
    • letting the edit-history goroutine ignore init failure
    • checking MDServer before readiness in StartMigration

    The synced-TLF goroutine's failure handling isn't pinned by the test, because it returns early when MDServer is unset.

  • go test -race passes for the new tests and for the related KBFSOps, favorites, edit-history, migration and session tests.

  • Builds and checks: go/bind, simplefs, libgit, fsrpc and kbfsfuse build. gofmt and vet are clean, and golangci-lint --new-from-rev master reports 0 issues.

  • Hot-dev CORS filter: a standalone Electron script confirmed that the http://127.0.0.1:*/files/* filter is accepted and matches requests on an arbitrary port.

@chrisnojima
chrisnojima requested review from zoom-ua and a balanced review from Copilot September 10, 2026 13:14

This comment was marked as outdated.

@chrisnojima
chrisnojima removed the request for review from zoom-ua September 10, 2026 14:00
@chrisnojima
chrisnojima marked this pull request as draft September 10, 2026 14:00
@chrisnojima
chrisnojima added this pull request to stack #29623 September 10, 2026 21:03
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-crash-fix branch from f7e7d88 to 40dca37 Compare September 10, 2026 21:26
@chrisnojima
chrisnojima requested a balanced review from Copilot September 10, 2026 21:26

This comment was marked as outdated.

@chrisnojima
chrisnojima requested a balanced review from Copilot September 11, 2026 01:58

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-crash-fix branch from c682ff0 to 10144bd Compare September 11, 2026 13:47
…FSOps is set

KBFS init connects to the service (and registers its notification
handlers) before it calls SetKBFSOps. A Reachability notification in that
window called PushConnectionStatusChange on a nil KBFSOps, crashing the
mobile app at launch with a nil dereference at addr 0x140.

Guard the notification handlers and serviceLoggedIn so they skip KBFSOps
work until init has set it.
serviceLoggedOut and serviceLoggedIn (via CurrentSession, including its
async setHomeTlfIdsForDbcAndFavorites) also dereference KBFSOps, and
logout touches Chat, which is set late too. Skip logout when KBFSOps is
unset (nothing is cached yet), and for login forget the session instead
so the first lookup after init runs the logged-in flow.
…ession exactly once

The TlfKeys, FSRequest and ImplicitTeamMigration handlers share the
service connection and dereference KBFSOps/MDOps too; return
errKBFSNotInitialized until init has set them.

Replace the after-the-fact session-cache clear with not caching the
session while KBFSOps is unset. A lookup now reports a new session only
if it cached it, so serviceLoggedIn runs exactly once per login even if
init finishes mid-lookup.
…ness check

SimpleFS, KBFSGit and Fs are registered on the same service connection as
the KBFS handlers, so their requests can also arrive before init has set
KBFSOps, MDOps, Notifier and the servers. Wrap every method of those
additional protocols so it waits until init has set them, bounded by the
caller's context (as SimpleFS's getKBPKI already does for KBPKI). Requests
run on their own goroutines, so waiting doesn't block the connection.

Replace the per-site checks with kbfsOpsReady (KBFSOps and MDOps) for the
login/logout flows, since serviceLoggedIn reaches MDOps and init sets it
just after KBFSOps, and kbfsServersReady (plus the key and block servers)
for service-initiated requests that fetch keys or blocks.
…arding handlers

The crashes came from init creating the service connection, which
registers KBFS's handlers, before KBFSOps and MDOps existed. KBPKI,
KBFSOps, KeyManager and MDOps only store config at construction, and the
test config already builds them before the service, so create them first
and drop the per-handler nil guards and the session-cache gate.

What's still set after the service connection is live:
- Chat: serviceLoggedOut nil-checks it.
- The key and block servers: service-initiated requests return
  errKBFSNotInitialized, and SimpleFS/git/fs requests wait for them.

If init fails after the service is set, shut the service connection down
so the service stops routing to a half-initialized KBFS; that also
cancels requests waiting for the servers.

TestInitSetsUpKBFSBeforeService runs the real doInit with a fake service
that delivers notifications, a login and a logout mid-init, and checks
the connection is shut down when init fails.
…g the connection down

The failed-init service.Shutdown() didn't disconnect anything: the shared
transport's Close is a no-op, so the handlers stayed registered and
waiting requests weren't cancelled. For passthrough child inits (libgit,
search) it also stopped the parent service's keepalive. Drop it.

Instead, KBFSOpsStandard records how init ended: initDoneCh is now closed
only on success, and a new initFailedCh on failure.
- SimpleFS/git/fs requests wait for that (bounded by the caller's
  context) instead of polling for the key and block servers, so they
  also wait out journaling setup, and fail fast if init failed.
- Service-initiated KBFS requests check it without blocking.
- The edit-history and synced-TLF goroutines return on failure instead
  of running against a half-built config.

Also set up the disk limiter before the service connection: a login it
delivers can create the disk block cache, which expects the limiter.

TestInitSetsUpKBFSBeforeService now calls into KBFS from inside the fake
NewKeybaseService (the earliest point) as well as after the service is
set, checks that requests wait during init and fail once init fails, and
shuts KBFSOps down before its TempDir is removed.
TestKeybaseDaemonRPCGatesAdditionalProtocolsOnInit checks the real
NewKeybaseDaemonRPC wiring.
… journaling

Gating on all of doInit held SimpleFS/git/fs requests through
EnableJournaling, whose journal FBO setup isn't bounded by its 60s
context, so early GUI calls (online status, subscriptions, badge) could
exceed the service's 60s SimpleFS timeout where master served them.
Service-initiated requests (chat's TLF key lookups) also failed for that
whole tail.

Add a separate ready signal: doInit calls kbfsOps.initReady() once Crypto,
the servers, the caches and the KBFS service are set, just before
journaling. Requests wait for (or check) that instead of full init; a
failed init still makes them fail fast. The edit-history and synced-TLF
goroutines keep waiting for full init.

Also assert at compile time that KBFSOpsStandard implements
kbfsInitWaiter, so renaming its method can't silently open the gate.
… hot dev

The hot-dev renderer loads from http://localhost:4000, so text preview
XHRs to the KBFS http server are cross-origin and blocked by CORS. Add
the allow-origin header to those responses in hot dev only.
…ck it first in StartMigration

The sync batch size validation ran after initReady(), so an invalid value
released waiting requests and then failed init; released requests can't be
recalled. Validate and set it before initReady().

StartMigration checked MDServer before readiness. MDServer is nil until
init is ready, so it returned "no mdserver" instead of the transient
errKBFSNotInitialized. Check readiness first, and cover it in
TestInitSetsUpKBFSBeforeService.
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-crash-fix branch from 10144bd to bee0168 Compare September 11, 2026 13:47
@chrisnojima
chrisnojima marked this pull request as ready for review September 11, 2026 13:54
@chrisnojima
chrisnojima requested a review from zoom-ua September 11, 2026 13:54

@zoom-ua zoom-ua left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

3 participants