Fix all four 2.11.2 production crash clusters (~70% of crashes) - #54
Open
forward-technologies wants to merge 7 commits into
Open
Fix all four 2.11.2 production crash clusters (~70% of crashes)#54forward-technologies wants to merge 7 commits into
forward-technologies wants to merge 7 commits into
Conversation
…thread)
Play vitals for 2.11.2 shows ~70% of crashes in four root causes. This addresses
three of them; the fourth (a MediaCodec CHECK abort) is not yet diagnosed.
ctrl thread lifecycle (~13.7%, shared lib, affects all platforms)
chiaki_ctrl_join() decided whether a thread existed by memcmp-ing ctrl->thread
against a zeroed ChiakiThread. ChiakiThread wraps an opaque pthread_t, so a stale
or partially written handle compares non-zero while still being invalid, and the
guard forwarded it to pthread_join(). bionic aborts the process on that rather
than returning an error:
invalid pthread_t 0x100000000 passed to pthread_join
Because the join never completed, teardown then destroyed notif_mutex under a
still-running ctrl thread, giving the paired FORTIFY aborts:
pthread_mutex_lock called on a destroyed mutex
Both signatures are confirmed in production stack traces. The guard now keys off
an explicit thread_started flag and never inspects the pthread_t. Upstream has no
such guard (plain chiaki_thread_join), so this moves us back toward upstream.
Also in ctrl_connect():
- rudp init response size was computed as data_size - 8 before validating
data_size, underflowing size_t on a short peer message and turning the VLA into
a ~2^64 byte stack allocation. That matches the ctrl_connect SIGSEGVs, which
crash at an unmapped PC rather than a readable fault address.
- an inner `err` shadowed the outer one, so every goto error in the rudp block
returned the outer err's initial CHIAKI_ERR_SUCCESS, reporting a failed RUDP
init as a successful connect.
RecyclerView stable-id collision (~28.9%, Android)
CloudGameAdapter returned productId.hashCode() as a stable id. Colliding ids make
RecyclerView throw IllegalStateException from handleMissingPreInfoForChangeError.
Identity is now positional, matching Qt (GridView over a plain array model); iOS
keys off the productId string itself. Uniqueness of productId is the lib's job
(dedupe_contract_product_ids), so the adapter does not merge or drop rows — the
rendered set is unchanged.
EncryptedSharedPreferences AEADBadTagException (~21.1%, Android)
Auto Backup saved secure_tokens.xml while its Android Keystore master key stayed
device-bound, so a restore left ciphertext with no key. SecureTokenManager's
constructor was the only method that rethrew instead of degrading. It now resets
and reopens the store, and falls back to a signed-out state — the same place Qt
lands with a missing settings key and iOS with an unreadable Keychain item. The
file is also excluded from cloud backup and device transfer so the mismatch stops
happening.
Adds test/ctrl_lifecycle.c covering the join guard; the poisoned-handle case
aborts the test binary under the old implementation.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g join Resolves the remaining two 2.11.2 crash clusters from Play vitals (the MediaCodec CHECK abort at 6.5% and the chiaki_mutex_lock ANR), both rooted in video-decoder.c teardown: - kill_decoder() locked codec_mutex itself while set_surface() called it already holding that same non-recursive mutex — a guaranteed self-deadlock on the UI thread whenever the surface was removed with a live codec. That is the "[apk] chiaki_mutex_lock / Input dispatching timed out" ANR verbatim. The lock contract is now: caller holds codec_mutex, kill_decoder drops it only around the join. - set_surface()'s surface-removed path returned while still holding codec_mutex, so even without the deadlock every later decode call blocked forever. Now exits through the unlock. - kill_decoder()'s fallback path (no input buffer for the EOS frame) deleted the codec and reset shutdown_output without ever joining the output thread, which was still blocked in AMediaCodec_dequeueOutputBuffer on that codec — a use-after-free inside MediaCodec, consistent with the libc.so abort cluster (MediaCodec.cpp CHECK(mActivityNotify == NULL) failed, sampled on an Android 11 TV box). The join now happens on every path, strictly before AMediaCodec_delete. - shutdown_output doubles as a kill-in-progress marker so the two teardown entry points (sessionFree and sessionSetSurface(NULL), separate JNI calls) cannot double-join the output thread across the dropped-lock window. - audio-decoder.c's reinit path joined its output thread without stopping the codec first; that thread only exits on a dequeue error or EOS, so the join could hang forever. It now stops first, same as its fini path. Also: chiaki_ctrl_init() explicitly clears thread_started rather than relying on the session-level memset, and the ctrl lifecycle tests gain a real-thread positive path (create, join through the guard, verify idempotence). Verified: full unit suite 130/130 on macOS arm64 (script-configured build); Android arm64 debug APK builds (Kotlin + NDK, shared lib under NDK clang); macOS app launches, authenticates, and loads the cloud catalog. Decoder files are Android-only platform glue — Qt and iOS have their own decoders, so no cross-platform behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The previous guard capped data_size at sizeof(rudp_recv_buf) (520), but chiaki_rudp_send_recv's recv buffer is 1500 bytes and its parse clamps data_size to ~1492 — so a legitimate large init response could have been rejected. The rudp layer also already enforces data_size >= min_data_size (8) on the success path, so the underflow cannot occur through it; the check is retained as defense-in-depth with the cap raised to 1500, which matches the recv buffer and therefore can never reject a message that layer can produce. Full unit suite re-run: 130/130. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ag, token-store hardening Three independent reviews of the crash-fix branch surfaced real gaps; all HIGH and MEDIUM findings are addressed here. Shared lib + frontends (session thread lifecycle): - chiaki_session_join had the exact unguarded-join defect chiaki_ctrl_join was just cured of, and it is the more reachable one: Android's StreamSession discarded session.start()'s error and published the session anyway, so shutdown joined a never-created thread (pthread_join on a zero handle - bionic abort in the same chiaki_thread_join frame as the vitals cluster). chiaki_session_join now keys off session_thread_started, and StreamSession disposes and raises CreateError instead of publishing a failed session. - Qt's session_started flag was inverted (set only in Start()'s failure branch): on success the destructor skipped chiaki_session_join and ran chiaki_session_fini under live session/ctrl threads - destroying notif_mutex beneath a running chiaki_cond_timedwait, the FORTIFY destroyed-mutex signature - and on failure it joined a thread that never existed. Upstream sets the flag on success; restored. - ctrl_connect: two more goto-error paths returned CHIAKI_ERR_SUCCESS (one reachable with a long hostname, reporting "Ctrl connected" with nothing sent); both now set CHIAKI_ERR_BUF_TOO_SMALL. The failed-TCP-connect branch leaked its socket fd; closed. Android decoders (review of the teardown rework): - Output thread dequeues with a 100ms timeout instead of blocking forever: teardown's join relied on vendor MediaCodec stop() cancelling an infinite dequeue, which is exactly what the affected TV-box stacks get wrong. Now the thread re-checks shutdown_output at least every 100ms on any device. - Teardown entry points wait on a new teardown_cond while a kill is in flight instead of skipping: skipping let fini destroy codec_mutex that the in-flight kill was about to relock, and relied on Kotlin-side call ordering for safety. video_sample skips quietly during teardown, and both decoders guard the buffer pointers MediaCodec can return as NULL once stopped (audio's output path could memcpy from NULL when stop landed mid-iteration). - kill_decoder releases the ANativeWindow it previously leaked each cycle. Android token store: - SecureTokenManager's store is now process-wide (companion, lock, cached): it is constructed per-screen and sometimes on background threads, so two failing opens could each run the corruption reset, delete the master key the other just recreated, and let a stale SharedPreferencesImpl resurrect old keysets under a new key - recreating the AEADBadTagException loop. The destructive reset now requires a GeneralSecurityException (or Tink keyset corruption) in the cause chain - transient Keystore/IO failures no longer delete a valid token - and runs at most once per process. - saveNpssoToken reports failure, and PsnLoginActivity surfaces it instead of toasting success and looping back to the login screen. Android TV focus: - CloudGameAdapter updates via DiffUtil keyed on the productId string instead of notifyDataSetChanged: without stable ids a full reset made RecyclerView recover focus at index 0, yanking D-pad focus to the first card on every sort/favorite/refresh. Diff dispatch keeps unchanged views attached (focus survives), and a duplicate productId degrades animations rather than crashing. showOwnershipBadge rebinds in place via notifyItemRangeChanged. Verified: full unit suite 130/130 (macOS arm64, script build); Android arm64 debug APK builds; macOS app rebuilds and launches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… delete, waiter stranding Round-2 adversarial review of the previous fix commit found one HIGH and four MEDIUMs, all newly introduced or newly activated by that commit; all fixed. Holepunch ownership (HIGH, Android): The new failed-start path called session.dispose() — whose chiaki_session_fini finis the holepunch session — and then threw into a catch block that fini'd the same holepunch session again: double-free of its strings, pthread_join on an already-joined ws thread (bionic abort), lock of destroyed mutexes. The contract is now uniform: sessionCreate (chiaki-jni.c) consumes the holepunch pointer on EVERY outcome — its pre-init error paths fini it explicitly, and chiaki_session_init's own error label already did — so StreamSession.kt nulls its reference before constructing the Session and no catch block can touch it again. This also closes a pre-existing double-fini when chiaki_session_init itself failed. Qt session replacement (MEDIUM): Restoring the join in ~StreamSession exposed the one deletion site that deleteLater'd a possibly-live session without Stop(): the cloud sessionCreated handler. Joining a live session thread from the GUI thread deadlocks if that thread is blocked in a BlockingQueuedConnection into the GUI loop. The handler now detaches its signal handlers (the SessionQuit lambda operates on the member pointer and would otherwise delete the NEW session when the old one quits), then uses quit-driven deletion for started sessions (Stop() + SessionQuit -> deleteLater) and a plain deferred delete for never-started ones (the guarded join is a no-op). Adds StreamSession::IsStarted(). Decoder teardown (MEDIUM + LOWs, Android): - fini waits for teardown_waiters (threads parked in set_surface's new wait loop) as well as shutdown_output before destroying the mutex/condvar — previously it could fini them under a waiter still inside pthread_cond_wait. - The output thread re-checks shutdown at the top of every iteration, so the teardown join stays bounded even on a vendor codec that keeps yielding output buffers after stop(). - The audio NULL-buffer bail hands its buffer index back before exiting, and video_sample's teardown skip reports the frame as not processed so videoreceiver's reference-frame accounting stays honest. Token store (MEDIUMs, Android): - isUnrecoverableCryptoFailure subtracts the transient GeneralSecurityException subtypes (KeyStoreException, NoSuchProvider/Algorithm, UnrecoverableKey): androidx/Tink funnel keystore-unavailable through the same base class as real corruption, and a keystore hiccup must never delete a valid token. Cause-walk is depth-bounded. - A failed open now backs off 30s before retrying: encryptedPrefs is a computed getter, and hot main-thread callers (catalog observer) would otherwise rerun Keystore + file I/O per call in the degraded state. - saveNpssoToken uses commit() and returns its result: apply() is asynchronous and swallows disk failures, which would resurrect the silent login loop the return value exists to prevent. Verified: unit suite 130/130; Android arm64 APK builds (native libs confirmed rebuilt); macOS app rebuilds and launches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… wait, backoff at boot Round-3 review of the previous fix commit; all HIGH/MEDIUM findings addressed. - Qt session replacement: disconnect(old, nullptr, this, nullptr) missed every connection whose receiver isn't the backend — FfmpegFrameAvailable uses frame_obj as context and its lambda derefs the member `session` (nulled at that point: a queued frame would crash the frame thread, and a surviving connection would keep pulling frames out of the NEW session's decoder), and QmlMainWindow connects SessionQuit straight to QGuiApplication::quit in direct-stream mode (a late quit from the old session would exit the app). Now severs ALL outgoing connections (old_session->disconnect()) before attaching the quit-driven deleteLater. - video-decoder fini: a set_surface caller could slip into kill_decoder's dropped-lock window, park on teardown_cond, and still be inside pthread_cond_wait when fini destroyed the condvar/mutex. fini now sets fini_requested (waking set_surface callers bail out instead of touching the dying decoder) and re-waits for teardown_waiters after its kill completes, so the primitives are destroyed with no thread inside them. - Token store backoff: lastOpenFailureMs started at 0, and elapsedRealtime() counts from boot — an app launch within 30s of device boot (routine on TV boxes) would skip the very first open and read as signed out. Starts at -BACKOFF now. - Holepunch leaks on two rare failure paths: the log-file creation in StreamSession.kt now runs before the ownership handoff (an IOException there landed after the field was nulled, leaking the live holepunch session), and sessionCreate's host-strdup OOM path — which fails before connect_info.holepunch_session is assigned — finis the pointer read directly from the Java field, completing the consume-on-every-outcome contract. Verified: unit suite 130/130; Android arm64 APK builds (native rebuilt); macOS app rebuilds and launches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e metacalls disconnect() prevents future emissions but cannot retract QMetaCallEvents already posted to the frame thread's queue, and both FfmpegFrameAvailable lambdas hard-deref the member `session`, which is deliberately nulled during session replacement and teardown — a stale queued frame event in that window crashed the frame thread. Both lambdas now early-return on null. (A stale event that instead lands after the new session is registered pulls one frame from the live decoder — benign.) Also two trivial hardening guards closing the only reachability of the decoder-fini race noted in review: sessionSetSurface (JNI) ignores a NULL session like sessionFree does, and Kotlin setSurface ignores a zeroed nativePtr like dispose() does. Round-4 verdict on everything else: confirmed clean — the full disconnect() severs nothing teardown depends on (SessionQuit is emitted via a direct C++ call, not a connection), the teardown_waiters protocol has no lost wakeups, the host-OOM holepunch fini is legal JNI with no double-fini, and the backoff/boot arithmetic is correct. Verified: Android APK builds; Qt target compiles; unit suite unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Play vitals for 2.11.2 (28d) put ~70% of production crashes in four root-cause clusters. This branch fixes all four, then went through four rounds of adversarial Opus review; every HIGH and MEDIUM finding across all rounds is fixed in-branch. Final round verdict: converged (sole remaining MEDIUM fixed with the reviewer's prescribed 2-line guard; every other check CONFIRMED clean).
The bugs, the evidence, and the fixes
Cluster A — RecyclerView
handleMissingPreInfoForChangeError(28.9% of crashes)Bug:
CloudGameAdapterusedproductId.hashCode()as a stable id; 32-bit collisions (or catalog duplicates, unfixed in 2.11.2's lib) crash RecyclerView during change animations. Trace confirmed: pure framework frames offdispatchLayoutStep3.Fix: stable ids removed; updates dispatch through DiffUtil keyed on the productId string (the identity iOS uses; Qt renders a plain array model). A duplicate id now degrades animations instead of crashing.
showOwnershipBadgerebinds in place so Android TV D-pad focus survives refreshes — review caught that a barenotifyDataSetChangedwould send focus to card 0 on every sort/favorite/refresh (verified against RecyclerView 1.3.0 focus-recovery bytecode).Why safe: renders exactly the list the lib emits — nothing merged, dropped, or re-keyed; productId uniqueness stays the lib's job (
dedupe_contract_product_ids). Review confirmed no.games =assignment runs during layout/scroll.Cluster B — Tink
AEADBadTagExceptionon launch (21.1%)Bug: Auto Backup restored
secure_tokens.xmlwhile its Keystore master key stayed device-bound → undecryptable store; the constructor was the only path that rethrew.Fix: (1)
secure_tokens.xmlexcluded from cloud backup and device transfer; review verified by decompiling security-crypto 1.1.0-alpha06 that both Tink keysets live inside that same file. (2) Store is process-wide (companion + lock + DCL): concurrent failing opens could each run the reset and delete the other's master key. (3) The destructive reset requires a real crypto failure — transient Keystore subtypes (KeyStoreException,NoSuchProvider/Algorithm,UnrecoverableKey) are explicitly subtracted, since androidx funnels them through the same base class as corruption — and runs at most once per process. (4) Failed opens back off 30s (boot-safe arithmetic) so hot main-thread callers don't jank-loop. (5)saveNpssoTokenusescommit()and reports failure; the login screen surfaces it instead of toasting success into an infinite login loop.Why safe: unreadable-store behavior matches Qt (missing QSettings key → empty) and iOS (unreadable Keychain → nil): signed out, never crashed. All
hasNpssoToken()consumers treat false as "show login" (audited).Cluster C — ctrl/session thread lifecycle:
chiaki_thread_joinSIGABRT,chiaki_cond_timedwaitFORTIFY aborts,ctrl_connectSIGSEGV (13.7%)Bug (traces confirmed):
invalid pthread_t 0x100000000 passed to pthread_joinandpthread_mutex_lock called on a destroyed mutex.chiaki_ctrl_joinmemcmp'd an opaque pthread_t against zero — unsound; the abort left teardown destroyingnotif_mutexunder the live ctrl thread. Review then proved the same defect existed at two more sites, both concrete producers of the same signatures:chiaki_session_joinwas unguarded, and Android discardedsession.start()'s error and published the session — shutdown then joined a never-created thread. Both fixed (lib guard + Kotlin raisesCreateError).session_startedflag was inverted (set only in the failure branch): success never joined and fini'd under live threads; failure joined a never-created thread. Restored to upstream semantics.Also fixed in
ctrl_connect: a shadowederrreporting failed RUDP inits as success; two moregoto errors returning SUCCESS (one reachable — "Ctrl connected" with nothing sent, then a hang); a leaked fd per failed TCP connect; a defense-in-depth [8,1500] bound before a peer-sized VLA (bounds match the rudp layer's enforced invariants — an earlier 520 cap was itself caught in review as a would-be regression and corrected).Why safe: zero platform files call
chiaki_ctrl_*(grep-verified) — one code path serves all three platforms, so divergence is structurally impossible; the memcmp guard was Pylux-local, so removal converges toward upstream. Review verifiedthread_startedneeds no atomics (all access serialized), the new bool lands in struct padding (no iOS ABI impact), andchiaki_session_finiis safe on init'd-but-never-started sessions.Cluster D — MediaCodec
CHECK(mActivityNotify == NULL)abort +chiaki_mutex_lockANR (7.8%)Bug (both in
video-decoder.cteardown):kill_decoderself-deadlocked when called fromset_surface(non-recursive mutex already held) — the ANR verbatim; the surface-removed path returned holding the mutex; the fallback path deleted the codec without joining the output thread still blocked indequeueOutputBufferon it — UAF inside MediaCodec, matching the CHECK abort on Android 11 TV boxes.Fix: one lock contract (caller holds; dropped only around the join); join on every path strictly before delete; teardown re-entry waits on a condvar (with a registered-waiter count plus a
fini_requestedbail sofininever destroys primitives with a thread inside them — two review rounds tightened this window to zero); the output thread polls at 100ms and re-checks shutdown every iteration, so the teardown join stays bounded regardless of vendorstop()behavior — the infinite dequeue previously trusted exactly what the crashing vendor stacks get wrong; NULL-buffer guards where MediaCodec legally returns NULL after stop; audio reinit stops before joining; a leakedANativeWindowref per surface cycle fixed.Why safe: Android-only platform glue — Qt and iOS have their own decoders. The lib treats a declined video sample as a dropped frame (verified in
videoreceiver.c), and the decoder now reports undecoded frames honestly so reference-frame accounting stays correct.Found and fixed along the way (review rounds 2–4)
sessionCreateconsumes the holepunch pointer on every outcome (all pre-init error paths fini it; a pre-existing double-fini viachiaki_session_initfailure is closed by the same contract), and Kotlin drops its reference at the call.sessionCreatedhandler deleteLater'd a possibly-live session — with the join restored, that deadlocks the GUI thread againstBlockingQueuedConnection. Now: sever all of the old session's outgoing connections (itsSessionQuitlambda operates on the member pointer and would delete the new session; a survivingSessionQuit → QGuiApplication::quitwould exit the app; a surviving frame connection would deref a nulled member), then quit-driven deletion (Stop()+SessionQuit → deleteLater) for started sessions, plain deferred delete for never-started ones (IsStarted()added). Frame lambdas guard against metacalls already queued before the disconnect.commit()— see Cluster B.Review process
Four adversarial rounds, three parallel reviewers in round 1 (shared-lib concurrency / NDK MediaCodec threading / Kotlin+AndroidX), then one scoped reviewer per subsequent delta. Every finding was independently re-verified against the code before fixing; two reviewer claims were corrected during verification. Finding severity shrank monotonically: architecture bugs → one ownership bug → signal wiring → a 2-line guard. No HIGH or MEDIUM finding remains unfixed. Remaining LOWs (documented, deliberately deferred): plaintext RP tokens in default prefs still back up (benign half-signed-in restore state; needs a migration), and a pre-existing self-restart race in Qt's
CHIAKI_EVENT_QUITretry window.Verification (re-run after every round)
ctrl_lifecycletests — the poisoned-handle case uses the exact0x100000000from production and fails under the old guardscripts/build-macos.sh, launches, authenticates with PSN, loads the live catalogios/build.shpass standalone; CI compiles the full platform matrix🤖 Generated with Claude Code