From 1b6402f7860c1dfb8dc0cfa934da77692f65eacf Mon Sep 17 00:00:00 2001 From: Ednunp <29843396+Ednunp@users.noreply.github.com> Date: Mon, 27 Jul 2026 10:02:31 +0100 Subject: [PATCH] NVDA hang fix: background the 600k PBKDF2 + Core session key cache The 5.6 password-strength raise made key derivation 6x slower (~1s on old hardware, run twice for key+fingerprint), and RecomputeAudioCrypto ran it synchronously on the UI thread on profile load / first connect - a full NVDA freeze for a screen-reader user. Fixed two ways: 1. Core session cache (RemSoundCrypto): ForPlainPassword now caches (key,fingerprint) per plaintext password for the process, with Prewarm (derive off-thread without returning) and IsCached (is it a hit?). The slow PBKDF2 runs ONCE per distinct password per process, so a profile switch back to a used password is instant. This also fixes the efficiency-review finding that the SERVICE re-derived un-cached on every audio-device hot-plug - it now hits the same cache. 2. App off-thread derive (MainForm.RecomputeAudioCrypto): the fast cases (password unchanged / already cached / empty / weak - no PBKDF2) still apply synchronously; the ONLY slow case (a strong password's first use this session, with the window live) derives on a worker and applies on the UI thread when done. The key is held null until it lands - audio simply waits (mandatory encryption never streams keyless) rather than the UI freezing - and a generation counter discards a stale result if the password changed again meanwhile. Falls back to synchronous before the window handle exists (startup construction). Also: the weak-password explanation is factored into ExplainWeakPasswordIfNeeded and its one-shot flag now re-arms on a real password change, so a SECOND weak-password profile switched to in one session is still explained (review nit #6). Tests: PasswordRules now pins the cache (miss->hit, same-instance on repeat, Prewarm warms, empty/weak = no work). Gate 69/69. Co-Authored-By: Claude Fable 5 --- src/RemSound.App/MainForm.cs | 118 ++++++++++++++++++++-------- src/RemSound.App/SelfTest.cs | 16 +++- src/RemSound.Core/RemSoundCrypto.cs | 41 ++++++++-- 3 files changed, 138 insertions(+), 37 deletions(-) diff --git a/src/RemSound.App/MainForm.cs b/src/RemSound.App/MainForm.cs index cfb4718..ac0f4b3 100644 --- a/src/RemSound.App/MainForm.cs +++ b/src/RemSound.App/MainForm.cs @@ -8276,52 +8276,108 @@ public sealed partial class MainForm : Form } } - /// Derive the audio key + fingerprint from the current profile password (cached so - /// the slow derivation only runs when the password actually changes) and push them down to - /// the sender and receiver. No password → null key → no audio flows (encryption is + /// Derive the audio key + fingerprint from the current profile password and push them + /// to the sender and receiver. No/weak password → null key → no audio flows (encryption is /// mandatory). Called on password change, when audio is (re)configured, and on the streaming - /// gate. 2026-05-31. + /// gate. 2026-05-31. + /// + /// The 5.6 PBKDF2 raise (600k, run twice) costs up to ~1 s on old hardware, so the FIRST use of + /// a strong password this session is derived OFF the UI thread — otherwise a blind user gets a + /// full NVDA freeze on profile load / first connect (2026-07-27 review). The key is held null + /// until it lands (audio simply waits — mandatory encryption means it never streams keyless), a + /// generation guard drops a stale result if the password changed again meanwhile, and the fast + /// cases (unchanged / cached / empty / weak — no PBKDF2) still apply synchronously so nothing + /// races on the common path. private void RecomputeAudioCrypto() { - if (currentProfilePassword != lastDerivedPassword) + var pw = currentProfilePassword; + if (pw == lastDerivedPassword) { - // Key + fingerprint always together, through the one shared rule (same as the service). - (currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(currentProfilePassword); - lastDerivedPassword = currentProfilePassword; - // Since 5.6 that rule also refuses a WEAK password (key comes back null), so a profile - // that auto-connects at startup with an old guessable password must not just sit - // silently dead — say why, once, and point at the fix. The interactive tick path has - // its own guided flow (EnsureStreamingPassword); this catches every other route in. - if (!string.IsNullOrEmpty(currentProfilePassword) && currentAudioKey is null && !weakPasswordExplained) + PushAudioCrypto(); // no change — re-push the cached key/fp (or null) + return; + } + lastDerivedPassword = pw; + var gen = ++cryptoGeneration; + // The password genuinely changed — re-arm the one-shot weak-password explanation so a SECOND + // weak-password profile switched to in the same session is still explained (not just the first). + weakPasswordExplained = false; + + // Fast path: null/empty/weak (resolves to (null,null) with no PBKDF2), or an already-cached + // strong password. Apply synchronously — no freeze possible. Also covers the pre-window + // case where the form has no handle yet (BeginInvoke would throw), so startup stays correct. + if (RemSoundCrypto.IsCached(pw) || !IsHandleCreated) + { + (currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(pw); + PushAudioCrypto(); + ExplainWeakPasswordIfNeeded(pw); + return; + } + + // Slow path: a strong password not yet derived this session. Hold the key null (audio waits) + // and do the PBKDF2 on a worker; apply on the UI thread when it lands, unless superseded. + currentAudioKey = null; + currentAudioFingerprint = null; + PushAudioCrypto(); + logFile.Event("audio crypto: deriving key off-thread (strong password, first use this session)"); + var pwLocal = pw; + Task.Run(() => + { + RemSoundCrypto.Prewarm(pwLocal); // the ~1 s PBKDF2, off the UI thread + try { - weakPasswordExplained = true; - logFile.Event("audio crypto: profile password fails the 5.6 strength rule — no audio until it's changed"); - var advice = PasswordStrength.Critique(currentProfilePassword) ?? ""; BeginInvoke(() => { - var page = new TaskDialogPage - { - Caption = "Password needs strengthening", - Heading = "No audio until this profile's password is stronger", - Text = "From this version, RemSound refuses to stream on a password that's easy to guess. " - + advice + " Change it via the File menu, “Change this profile's password” — on every machine that uses it.", - Icon = TaskDialogIcon.Warning, - Buttons = { TaskDialogButton.OK }, - DefaultButton = TaskDialogButton.OK, - AllowCancel = true, - }; - ForegroundDialog.Show(owner => TaskDialog.ShowDialog(owner, page)); + if (gen != cryptoGeneration) return; // a newer password change won; drop this result + (currentAudioKey, currentAudioFingerprint) = RemSoundCrypto.ForPlainPassword(pwLocal); // now a cache hit + PushAudioCrypto(); + logFile.Event("audio crypto: key ready"); }); } - } + catch { /* form closed / no handle — nothing to apply to */ } + }); + } + + private void PushAudioCrypto() + { sender.AudioKey = currentAudioKey; sender.AudioFingerprint = currentAudioFingerprint; receiver.AudioKey = currentAudioKey; receiver.AudioFingerprint = currentAudioFingerprint; } - // One-shot flag for the weak-password explanation above — the dialog must not re-fire on every - // profile reapply within a session (the log line still records each derivation refusal). + /// Since 5.6 the derivation rule refuses a WEAK password (key comes back null), so a + /// profile that auto-connects at startup with an old guessable password must not just sit + /// silently dead — say why, once, and point at the fix. The interactive tick path has its own + /// guided flow (EnsureStreamingPassword); this catches every other route in. + private void ExplainWeakPasswordIfNeeded(string? pw) + { + if (string.IsNullOrEmpty(pw) || currentAudioKey is not null || weakPasswordExplained) return; + weakPasswordExplained = true; + logFile.Event("audio crypto: profile password fails the 5.6 strength rule — no audio until it's changed"); + var advice = PasswordStrength.Critique(pw) ?? ""; + BeginInvoke(() => + { + var page = new TaskDialogPage + { + Caption = "Password needs strengthening", + Heading = "No audio until this profile's password is stronger", + Text = "From this version, RemSound refuses to stream on a password that's easy to guess. " + + advice + " Change it via the File menu, “Change this profile's password” — on every machine that uses it.", + Icon = TaskDialogIcon.Warning, + Buttons = { TaskDialogButton.OK }, + DefaultButton = TaskDialogButton.OK, + AllowCancel = true, + }; + ForegroundDialog.Show(owner => TaskDialog.ShowDialog(owner, page)); + }); + } + + // Bumped on every password change so a slow background derive that finishes AFTER a newer change + // knows to discard its now-stale result (see RecomputeAudioCrypto). + private int cryptoGeneration; + // One-shot flag for the weak-password explanation — the dialog must not re-fire on every profile + // reapply within a session (the log line still records each derivation refusal). Reset on a + // profile switch so a SECOND weak-password profile in one session is still explained. private bool weakPasswordExplained; /// The "you need a password before any audio can flow" gate. Called when the user diff --git a/src/RemSound.App/SelfTest.cs b/src/RemSound.App/SelfTest.cs index 85c23b9..b2b49f3 100644 --- a/src/RemSound.App/SelfTest.cs +++ b/src/RemSound.App/SelfTest.cs @@ -2474,7 +2474,21 @@ internal static class SelfTest var (goodKey, goodFp) = RemSoundCrypto.ForPlainPassword("kettle9tiger42moon"); Check(goodKey is { Length: 32 } && goodFp is { Length: 8 }, "a strong password must derive the full key + fingerprint"); - return "weak + common refused with concrete advice; derivation choke-point refuses weak on every path"; + // Session cache (the NVDA-hang fix): a distinct strong password is a cache MISS until derived + // or prewarmed, then a HIT — so the ~1s PBKDF2 runs once per password per process and the + // UI-thread path can stay synchronous only when it's a hit. A second derive returns the SAME + // arrays (proof it wasn't recomputed). Empty/weak always count as "no work" (never block). + Check(RemSoundCrypto.IsCached("kettle9tiger42moon"), "a password just derived must read back as cached"); + var again = RemSoundCrypto.ForPlainPassword("kettle9tiger42moon"); + Check(ReferenceEquals(again.Key, goodKey), "a cached derive must return the same key instance, not recompute it"); + var fresh = "prewarm7melon42anchor"; + Check(!RemSoundCrypto.IsCached(fresh), "an unused strong password must read as a cache miss (would block the UI thread)"); + RemSoundCrypto.Prewarm(fresh); + Check(RemSoundCrypto.IsCached(fresh), "Prewarm must derive off-thread so the later UI-thread lookup is an instant hit"); + Check(RemSoundCrypto.IsCached("") && RemSoundCrypto.IsCached("Games"), + "empty and weak passwords must count as 'no work' (they never trigger a blocking derive)"); + + return "weak+common refused with advice; derivation refuses weak everywhere; results cached (no repeat PBKDF2)"; } /// The relay address-proof (2026-07-27): an AddrCheck cookie arriving at the receiver diff --git a/src/RemSound.Core/RemSoundCrypto.cs b/src/RemSound.Core/RemSoundCrypto.cs index b2a53b2..6ae457e 100644 --- a/src/RemSound.Core/RemSoundCrypto.cs +++ b/src/RemSound.Core/RemSoundCrypto.cs @@ -71,6 +71,14 @@ public static class RemSoundCrypto private static readonly byte[] ObfuscationKey = Encoding.UTF8.GetBytes("RemSound-profile-password-scramble-v1"); + // Session cache of derived credentials, keyed by plaintext password. The 600k-iteration PBKDF2 + // (run TWICE per derive, for key + fingerprint) costs up to ~1 s on old hardware; caching means + // a given password pays that ONCE per process, so a profile SWITCH back to a used password — or + // the service re-deriving on an audio-device hot-plug — is instant. The derived key already lives + // in process memory (currentAudioKey), so this doesn't widen exposure; it's cleared on exit like + // everything else. Bounded: a user has a handful of distinct passwords, not thousands. + private static readonly System.Collections.Concurrent.ConcurrentDictionary derivedCache = new(StringComparer.Ordinal); + /// The one rule for turning a PLAIN password into the audio credentials: null/empty → /// (null, null) → no audio flows (encryption is mandatory); since 5.6 a password that fails /// ALSO yields (null, null) — enforced here, at the single @@ -79,11 +87,34 @@ public static class RemSoundCrypto /// explains and walks the user to a stronger one). Otherwise the key AND the fingerprint, always /// together — the peer verifies the fingerprint before accepting a stream, so a key without its /// fingerprint gets the audio silently rejected at the far end (a divergence that already bit - /// the service once). - public static (byte[]? Key, byte[]? Fingerprint) ForPlainPassword(string? plainPassword) => - string.IsNullOrEmpty(plainPassword) || PasswordStrength.Critique(plainPassword) is not null - ? (null, null) - : (DeriveKey(plainPassword), Fingerprint(plainPassword)); + /// the service once). Result is cached per password (see ); the slow + /// PBKDF2 runs once per distinct password per process. Callers that must not stall the UI thread + /// should off-thread first (the app does). + public static (byte[]? Key, byte[]? Fingerprint) ForPlainPassword(string? plainPassword) + { + if (string.IsNullOrEmpty(plainPassword) || PasswordStrength.Critique(plainPassword) is not null) + return (null, null); + var (k, f) = derivedCache.GetOrAdd(plainPassword, static pw => (DeriveKey(pw), Fingerprint(pw))); + return (k, f); + } + + /// Derive-and-cache a password's credentials WITHOUT returning them — for calling on a + /// background thread so the subsequent on the UI thread is a cache + /// hit and never blocks on PBKDF2. Null/empty/weak passwords are a no-op (nothing to warm). + public static void Prewarm(string? plainPassword) + { + if (!string.IsNullOrEmpty(plainPassword) && PasswordStrength.Critique(plainPassword) is null) + derivedCache.GetOrAdd(plainPassword, static pw => (DeriveKey(pw), Fingerprint(pw))); + } + + /// True when 's credentials are already cached, so + /// will return instantly without running PBKDF2. Lets the caller + /// decide whether it can derive on the UI thread (cached / weak / empty = fast) or must go + /// off-thread (a strong password's first use this session). Empty/weak count as "no work". + public static bool IsCached(string? plainPassword) => + string.IsNullOrEmpty(plainPassword) + || PasswordStrength.Critique(plainPassword) is not null + || derivedCache.ContainsKey(plainPassword); /// Derive the 256-bit AES key for a password. Cache the result; never call per packet. public static byte[] DeriveKey(string? password) =>