XM4 first-class support + parser fuzzing/hardening - #7
Open
SubliminalCoding wants to merge 16 commits into
Open
Conversation
…en tests In-progress work from the Codex session: XM4 (WH-1000XM4 v1) protocol support, 14 verified bug fixes (connection-thread crashes, command/notification races), 18 exact Bluetooth wire-format golden tests, and protocol-conformance harness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…parser Found uncommitted in the working tree (authored by the earlier audit pass, not this session); committed verbatim so the tree is clean and the analysis is not lost. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Message lives in the CommandSerializer namespace, and its seqNumber field is unsigned char; the harness used the unqualified name and narrowed through a signed char cast, so the libFuzzer target never built. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- -max_len referenced MAX_BLUETOOTH_MESSAGE_SIZE as a shell variable, which is never set, so 'set -u' killed the script before fuzzing; derive it from Constants.h instead. - Apple's clang++ has no libFuzzer runtime and the failure only appears at link time; probe with a real link and fall back to a Homebrew LLVM clang++. - When no libFuzzer-capable compiler exists at all, build driver_main.cpp: a deterministic seed-replay + structured-mutation driver (bit/byte flips, truncation, length-field tampering, escape/marker injection, splicing) over the same LLVMFuzzerTestOneInput entry point under ASan/UBSan. - Suppress libFuzzer's own never-joined RSS-monitor thread (lsan.supp) so leak detection stays on for the parser; skip detect_leaks on Apple ASan, which does not support it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both the fuzz campaign (8.2M execs, zero parser bugs) and the static audit flagged the same latent issue: three malformed-frame paths in unpackBtMessage threw bare std::runtime_error instead of the RecoverableException used by the sibling checks. Safe only because every current call site catches std::exception; a future site narrowing its catch would turn attacker- controlled input into std::terminate (remote DoS). Made all three throw RecoverableException(msg, true), matching the existing malformed-frame throws, and added regression assertions. Also broadened the LSan suppression from fuzzer::StartRssThread to fuzzer:: to cover an intermittent symbolizer leak in libFuzzer's PC printer (parser never on the stack). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…corruption) Covers the gap between the exact golden fixtures and the crash-only fuzzer with relational properties over programmatically generated inputs (fixed-seed splitmix64, fully deterministic): - P1 round-trip: unpack(strip_markers(pack(p, dt, seq))) recovers (p, dt, seq & 0xFF); pack output is also structurally verified against an independent re-implementation of the documented layout (declared BE size, mod-256 checksum, header bytes). - P2 escaping transparency: every escapable byte (60/61/62) at every payload position round-trips; the frame interior never contains a raw marker byte. - P3 seq domain: 0..255 round-trips exactly; wider values pack byte-identically to seq & 0xFF. - P4 framing: +/-1-byte damage at either end and marker-attached frames are all rejected with RecoverableException. - P5 determinism and inverse consistency: pack is byte-identical across calls; repacking an unpacked message reproduces the frame. - P6 corruption: exhaustive single-byte replacement (all 255 values, every position, four frames incl. escape-dense ones) plus per-bit flips on a 512-byte-payload frame - 31270 corrupted bodies, all rejected recoverably, none silently parsed. - P7 size boundary: 2039 plain payload bytes fill the 2048 frame exactly; 2040 overflows; escape-heavy payloads halve the budget (1016 x 0x3C fits, 1020 does not). Wired into Client/tests/run-macos.sh after the conformance suite, same clang++ -Wall -Wextra -Werror invocation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The RFCOMM data delegate buffered every frame unconditionally, but recv() only drains the queue during a command exchange, so a device streaming unsolicited frames grew receivedBytes without bound. Cap the buffered total at RECEIVE_BUFFER_CAP, dropping the oldest chunks; the wrapper's marker scan resynchronizes on what survives. A zero-length frame also made recv() return 0, which BluetoothWrapper reads as a closed connection - drop those at the delegate boundary instead of fabricating a disconnect. Covered by new offline tests that inject frames exactly as the delegate would, with no Bluetooth channel opened. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnKJraRSytaMh5E2G57ifx
[[device addressString] UTF8String] on a nil addressString hands std::string a null char*, which is undefined behaviour. getConnectedDevices() already guards the same call; apply the same check on the scan-and-connect path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MnKJraRSytaMh5E2G57ifx
56k relational checks (round-trip, escaping transparency, single-byte corruption rejection, framing/truncation) over the protocol layer. All green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bounds the macOS connector receive queue (fixes unbounded attacker-driven allocation / OOM), drops zero-length frames, guards a nil addressString. Regression tests for the buffer cap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> # Conflicts: # Client/tests/run-macos.sh
Every integer step of an ambient-level or custom-EQ slider drag was enqueuing a full Bluetooth exchange on the serial command queue (~70ms measured per exchange, up to 2500ms worst-case each), so a 20-step drag queued ~1.4s+ of stale intermediate sends that kept hitting the headset after the user released. HeadphonesModel now mirrors the in-flight gate the ImGui client already uses (CrossPlatformGUI _sendFeatureCommand): the first change sends immediately; changes arriving while an exchange is in flight only overwrite a pending snapshot; when the exchange completes, at most one more send fires, and only if the pending snapshot differs from what was last sent. The pending value is an explicit snapshot (not re-read from the model at completion time) so syncFromBridge or the 2s poll overwriting @published state cannot clobber the user's latest drag value. All gate state is main-thread-confined - bridge completions arrive on the main queue - so no locks are needed. Isomorphism: end-state is unchanged - the user's final value always reaches the device (on failure with a newer pending value, the pending value is still sent; pending is cleared before resending so it is never sent twice), single clicks still send immediately with no added latency, and a drag now costs <=2-3 exchanges instead of one per step. Discrete controls (mode buttons, toggles) are untouched. disconnect() drops pending snapshots so an intentional disconnect does not fire a doomed resend. Verified: 4 macOS test suites green (protocol safety, 18 golden, 56109 metamorphic checks, connector buffer), 12s fuzz smoke exit 0, Release xcodebuild clean with no new warnings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every 2s poll tick and every interaction completion wrote 8-12 @published properties unconditionally; each setter fires objectWillChange, so the whole ContentView body re-evaluated every tick even when nothing changed. The wire poll only refreshes ambient state, so most per-tick writes (EQ/DSEE re-read from cached bridge values) were provably no-ops that still forced a full re-render — the recurring 0.4-1.2% idle CPU blips in .hotspots/evidence/connected_idle_cpu_series.txt, pure waste on battery. Mechanism: a generic assignIfChanged(keyPath, value) helper writes a @published property only when the new value differs from the current one, applied to both the refreshDynamic poll completion and syncFromBridge. The eqBands candidate array is still built the same way, then compared before assignment instead of assigned unconditionally. Isomorphism: reads, read order, and poll cadence are untouched — only writes became conditional. Equal value -> no write -> visible state identical; different value -> write fires exactly as before, so every device-side change still lands in the UI on the same tick it did before. The slider-coalescing convergence from the previous commit is preserved: after a successful push, syncFromBridge still overwrites mode/ambientLevel whenever device truth differs from the UI, and skipping the write when they are equal is indistinguishable. First sync after connect still fires every write, since all values transition from their defaults. Verified: 4 test suites green, fuzz smoke exit 0, Release build succeeded. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Release configuration never chose a C++/ObjC++ optimization level: GCC_OPTIMIZATION_LEVEL was absent (Xcode default -Os), DEAD_CODE_STRIPPING resolved to NO, and SWIFT_COMPILATION_MODE was unpinned. Every C++ profile of the protocol stack was bounded by unchosen codegen (HOTSPOTS rank 3). Changes (Release blocks only, Debug untouched): - project-level Release: GCC_OPTIMIZATION_LEVEL = 2 (explicit -O2 — deliberate speed-over-size for the protocol paths; -O3 skipped, no measured win to justify vectorization bloat) - project-level Release: DEAD_CODE_STRIPPING = YES - target-level Release: SWIFT_COMPILATION_MODE = wholemodule (pins the Xcode-default; SWIFT_OPTIMIZATION_LEVEL already -O) Considered and skipped: LTO (unmeasured benefit vs real build-time and debuggability cost), C++ standard cleanup (gnu++14 vs c++17 hygiene, not this pass's lever). Isomorphism: config-only, zero source changes — -O2 vs -Os is a codegen-level change with identical observable semantics, and dead-code stripping removes only unreferenced symbols. Verification: - Resolved Release (was: GCC_OPTIMIZATION_LEVEL unset, DEAD_CODE_STRIPPING = NO, no SWIFT_COMPILATION_MODE) → now GCC_OPTIMIZATION_LEVEL = 2, DEAD_CODE_STRIPPING = YES, SWIFT_COMPILATION_MODE = wholemodule - Resolved Debug unchanged (0 / NO / -Onone) - Clean Release build: BUILD SUCCEEDED, no new warnings - Binary: 853,392 → 730,304 bytes (-14.4%; dead-strip outweighs -O2 growth) - Tests: all 4 suites green (protocol safety, 18 golden, 56,109 metamorphic checks, connector buffer); fuzz smoke exit 0 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…a sourceless run loop Live profiling (10s @1ms, connected to a physical XM4) proved the connectToMac pump loop dead weight: all 8537/8537 connector-thread samples sat in __psynch_cvwait at the wait_for, zero in the runUntilDate pump, while inbound RFCOMM data was delivered by -[IOBluetoothRFCOMMChannel stream:handleEvent:] on the MAIN thread's run loop (CFSocket source scheduled there). The connector thread's run loop has no sources; it never delivered anything. Change: replace the 100ms-pump/1000ms-timed-poll alternation with a plain cv wait on running. Removing the timed re-poll unmasks a lost-wakeup race (running is atomic, stored lock-free by notifiers; a notify landing between the waiter's predicate check and its block was previously bounded at 1s, now would sleep forever), so every running=false setter — disconnect(), handleChannelClosed(), connect()'s catch — now stores under disconnectionMutex before notifying. Hazards traced: 1. Channel lifetime: the wait keeps connectToMac's frame (and its strong `channel` ref) alive exactly as the loop did; release ordering at frame exit vs running=false/notify unchanged. closeConnection() retains its own strong ref before closeChannel; disconnect() still joins before the connector object is destroyed (destructor + connect() catch paths). 2. Open-complete: connectPromise.set_value() fires synchronously after openRFCOMMChannelAsync returns success, before the wait — connect() never depended on the delegate callback; rfcommChannelOpenComplete: exists only under SHC_DEBUG_PROTOCOL and only logs. All delegate callbacks arrive on main (profiled), so the pump was load-bearing for nothing. 3. connect() error path kept coherent: catch stores running=false under the mutex like the other setters. 4. recv()'s receiveDataConditionVariable path untouched. Isomorphism: connect, data flow, and disconnect end states unchanged (data never flowed through this thread). Disconnect wakeup is now immediate instead of worst-case ~1s, and the thread does zero spurious wakeups (was ~2/s). Offline test added: both running=false setters wake the exact connectToMac wait, 200 interleavings, lost wakeup = test failure. Verified: 4 test suites green, FUZZ_SECONDS=10 fuzz exit 0, Release xcodebuild BUILD SUCCEEDED. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Speak-to-Chat pauses audio whenever the mics hear speech, which is hostile to anyone who dictates constantly — every utterance stops the music. This adds a persisted preference (UserDefaults key KeepSpeakToChatOffOnConnect) that forces S2C off once per connect. Mechanism: connect() arms a per-connection flag; the refreshStatus completion — which fires a second time after probeCapabilities confirms optional features — consumes the flag the first time hasSpeakToChat is true, and only then sends a single setSpeakToChat(false) if the pref is on and the device reports S2C enabled. Devices without S2C, or with it already off, never receive a write; the flag also clears on disconnect, so the disable fires at most once per connection. The manual toggle is untouched: turning S2C on mid-session sticks until the next connect. The pref defaults to true per this build's user request; upstream may prefer defaulting to false — it's a one-line change (the `?? true` in HeadphonesModel.swift). The secondary "Keep off on connect" toggle sits under the S2C switch and is hidden with it when the capability is absent, matching the existing gating pattern. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
v2 devices only, byte layout proven by Gadgetbridge on the WH-ULT900N
(ULT WEAR): commits 8cbc309346 ("WIP add ConnectTwoDevices feature"),
e8520865f3 ("fix: setting Connect Two Devices"), 5d7b37d1b7 ("fix read
Connect Two Devices"), files SonyProtocolImplV2.java and
SonyHeadphonesProtocol.java. State rides touch-sensor subtype d1 with an
inverted enable bit (GET d6 d1 -> RET d7 d1 00 <00=on/01=off>); a SET
sends d8 d1 00 <00=on/01=off> then the fixed apply commit 98 00 06 01
for both directions. The system-control RET (97 00 06) always reports 01
and is never used as a state source.
v1 (WH-1000XM4) gets no multipoint on purpose: Gadgetbridge's
SonyProtocolImplV1 explicitly logs "Connect two devices not implemented
for V1" and returns null, so no proven v1 frames exist. The v1 test now
asserts the XM4 path never emits any multipoint opcode and that
setMultipoint throws.
The probe (d6 d1) is appended to the v2 probeCapabilities sequence; the
toggle is capability-gated and mirrors the device-persistent state with
no app-enforced default. Full research trail with the negative findings
in docs/MULTIPOINT_RESEARCH.md; exact framing pinned in
conformance/fixtures/ult900n_v2_multipoint.golden.
Co-Authored-By: Claude Opus 4.8 <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
2 participants
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.
Brings WH-1000XM4 (v1) support to first-class status, hardens the untrusted Bluetooth parsing layer, adds four measured performance fixes, and ships two features (Speak-to-Chat default control, multipoint toggle for v2 devices).
Protocol & correctness
CommandSerializer::unpackBtMessageunder ASan/UBSan with a no-libFuzzer fallback driver; an 8.2M-execution campaign found zero parser bugs. (The scaffolded harness didn't previously compile — fixed.)Client/fuzz/AUDIT_FINDINGS.md): no memory-safety/integer/escape/round-trip defects.Security hardening
addressStringguard on the connect path.RecoverableExceptionuniformly (was barestd::runtime_errorin 3 spots — a latent std::terminate/DoS if a future call site narrowed its catch); locked by regression tests.Performance (profiled first — evidence-driven, one lever per commit)
Profiled the Release app live against a physical XM4 (idle-connected CPU ~0.3%, ~70ms per poll exchange):
@Publishedwrites gated to real changes — the 2s poll re-wrote 8 properties unconditionally, re-rendering the whole view when provably nothing changed.-O2,DEAD_CODE_STRIPPING=YES,SWIFT_COMPILATION_MODE=wholemodule(C++ was silently at the -Os default); binary −14.4%.Features
?? truein HeadphonesModel.swift).docs/MULTIPOINT_RESEARCH.mdand the golden fixtures). The v1/XM4 path is deliberately untouched: no proven v1 frames exist anywhere (GadgetBridge explicitly declines v1), and this PR does not invent wire bytes. Capability-probed, exact-byte SET/GET tests + a 6-case golden fixture.Verification
sh Client/tests/run-macos.sh).Client/fuzz/run-macos.sh).🤖 Generated with Claude Code