Fix/wasmclient#2074
Open
iscander01 wants to merge 7 commits into
Open
Conversation
1. Password logged in plaintext (CheckPasswordImpl:941) — removed TRACE(pass) from the debug log. dbName and res are still logged. 2. isValidPassword called twice (939-940) — deleted the leftover discarded call; the key-derivation check now runs once. 3. Unsubscribe signed/unsigned OOB (455-465) — added an explicit key < 0 || key >= size guard before any index math, eliminating the size() - 1 underflow path. The last-element case pops; anything else nulls in place. 4. Dead duplicate AddCallback (425-437) — removed. Only Subscribe/Unsubscribe are bound (subscribe/unsubscribe at 1053-1054), and nothing referenced AddCallback. 5. Moved-from recovery filename (StartWallet:555) — added m_CurrentRecoveryFile.clear(); after the std::move, so a subsequent StartWallet without a fresh ImportRecovery sees a deterministically empty filename.
4. Recovery blob left on persistent FS — recovery.bin is seed-equivalent material. Added RemoveRecoveryFile(), called from OnImportRecoveryProgress at completion (done == total || error), so it fires on both success and failure. It uses the fs::remove + FS.syncfs(false) pattern copied from DeleteWallet, uses an error_code overload (no throw), and removes only the literal recovery.bin we wrote — files passed to ImportRecoveryFromFile are untouched. m_CurrentRecoveryFile is already cleared in StartWallet, so no member reset needed here. 5. Unused WalletClient2::m_WalletApi (273) — removed. Confirmed it's dead: the class spans 97-275 and the member appeared only in its own declaration; all m_WalletApi uses (484/489/492/568) belong to WasmWalletClient's own member (still at 1015).
- #1 ASSERTIONS — CMakeLists.txt:61, -s ASSERTIONS=1 → 0. Drops the pervasive runtime checks/stack-trace glue from the shipping link. - #5 Logger level — wasmclient.cpp:405, BEAM_LOG_LEVEL_DEBUG → BEAM_LOG_LEVEL_INFO (both args). Kills the per-request/per-sync-tick DEBUG spam and its string-formatting cost on the main thread. I chose INFO over WARNING to keep operational logs; one-word change to WARNING if you want it quieter. Pairs with the earlier TRACE(pass) removal. - #6 Dispatch coalescing — wasmclient.cpp:191. Now captures wasEmpty under the lock and only schedules the proxied main-thread call + WeakPtr alloc on the empty→non-empty transition. I verified ProcessMessageOnMainThread (243-265) drains the entire queue in a while(true) loop, so this can't drop messages — worst case is one redundant no-op dispatch. Collapses toggleEvents bursts (N events) from N cross-thread calls + N allocs down to 1. Written -Werror-clean.
Bugs
- B1 � MountFS error path (wasmclient.cpp:~1017-1055). OnMountFS now takes (val* nullVal, int success); the JS syncfs callback calls dynCall('vii', $0, [$1, error == null ? 1 : 0]). s_Mounted is set only on success, the bogus pointer-deref is gone (always hands &s_Null, a valid val*), the FS error text is logged to console.error, and the mount promise resolves with a real Error on failure so JS can distinguish it. Also dropped the "mounting..." log (part of I2).
- B2 � OnResult reentrancy/UAF (:625). Index-based loop, and the val is copied before invoking, so a callback that synchronously (un)subscribes can't leave the loop on reallocated/freed storage.
- B3 � recovery blob leak with null callback (:658). finished is computed independently of the callback; RemoveRecoveryFile() now runs on completion even when ImportRecovery was called without a progress callback.
- B4 � stop-time abort (:235-243, :596-600). Confirmed reachable: the wallet thread calls onStopped() after run_ex returns (wallet_client.cpp:920), and queued SendResult messages hold shared_from_this(), so use_count can legitimately exceed 1 during stop. The five fatal Assert(wp.use_count()==N) are replaced with a non-fatal CheckUseCount (logs a warning, doesn't abort) � the custom Assert fires even under -s ASSERTIONS=0, so this was a real crash-on-stop-under-load. shared_ptr still guarantees destruction once the last owner releases, so correctness is unchanged.
- B5 � reinterpret_cast<int>(ptr) (:45-51). The emscripten headers aren't available locally (CI-only toolchain), so rather than an untestable EM_FUNC_SIG_VP migration I added a static_assert(sizeof(void*) == sizeof(int)). Correct on wasm32; a future MEMORY64 build now fails to compile at that guard instead of silently truncating pointers, with a comment pointing at the real fix.
Improvements
- I1 (:434) � Subscribe returns int consistently (size_t loop, static_cast<int> on both returns).
- I2 � removed the four debug lifecycle console.logs (wallet created / headless wallet created / wallet deleted / mounting), keeping the functionally-required FS.syncfs calls.
- I3 (:~968) � documented why CheckPassword's detached thread is safe (touches only copied strings + static isValidPassword).
- I4 (:~1055) � documented the m_WalletApi invariant (created/reset/used only inside makeIWTCall, i.e. the wallet thread) so the earlier de-dup isn't reintroduced as a bug.
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.
No description provided.