From bfc117b0c5e64470afdcfd38ac70cc60e0a60964 Mon Sep 17 00:00:00 2001 From: retoor Date: Tue, 27 Oct 2026 19:25:50 +0100 Subject: [PATCH] whatsapp: fix the real root cause of the pair-success crash (lifetime UB in serialize) Replaying the captured failing frame proved the std::vector max_size() abort was never a parser bug. It was an object-lifetime bug in WaDevice::serialize(): put_blob(b, Bytes((jid ? *jid : "").begin(), (jid ? *jid : "").end())); (jid ? *jid : "") builds a fresh temporary std::string for each evaluation, so begin() and end() came from two different, already-destroyed objects; the range constructor computed a garbage length and vector threw. It only misfired once jid was set (after the phone scanned), which is why every parser test and the initial unpaired save passed. Fixed by binding the value to a named const std::string before taking iterators. - src/whatsapp/wa_device.cpp: bind jid/lid before iterating; audited the tree for the same two-temporaries pattern (no other instances). - tests/whatsapp_test.cpp: test_device round-trips a PAIRED device (jid+lid set, synthetic identifiers), the populated case that triggers the bug; runs under tools/san-whatsapp.sh. - docs/whatsapp.md 13.7: records the real root cause and closes the open item. - docs/cpp-safety.md: adds the object-lifetime rule (never take iterators from a temporary you rebuild) as a distinct bug class from size overflow. Verified: replaying the captured pair-success frame now serializes, saves, and encodes the response with no throw; build, whatsapp_test, make lint, and tools/san-whatsapp.sh all green. Co-Authored-By: Claude Opus 4.8 --- taicpp/docs/cpp-safety.md | 3 ++- taicpp/docs/whatsapp.md | 34 +++++++++++++++++++++++++++++++ taicpp/src/whatsapp/wa_device.cpp | 11 ++++++++-- taicpp/tests/whatsapp_test.cpp | 26 +++++++++++++++++++++++ 4 files changed, 71 insertions(+), 3 deletions(-) diff --git a/taicpp/docs/cpp-safety.md b/taicpp/docs/cpp-safety.md index 06d8d39..d49d070 100644 --- a/taicpp/docs/cpp-safety.md +++ b/taicpp/docs/cpp-safety.md @@ -57,7 +57,8 @@ that touches bytes we did not produce (network frames, files, user input, IPC): on raw offsets. This is the std::span / "parse don't validate" discipline; note that `std::span::operator[]` and `subspan` do **not** bounds-check in C++20, so the reader validates, not the view. -7. **RAII and the rule of zero.** Resources are owned by objects that release +7. **Never take iterators from a temporary you rebuild.** `Bytes((c ? *o : "").begin(), (c ? *o : "").end())` evaluates the expression twice, so `begin()` and `end()` come from two different, already-destroyed temporaries: undefined behavior, and the garbage length aborted the paired-device save. Bind the value to a named variable first, then take its iterators. More generally, a range `[a, b)` must come from one live object. This is the object-lifetime class, distinct from size overflow; both are caught by the sanitizer net only if a test exercises the populated path. +8. **RAII and the rule of zero.** Resources are owned by objects that release them; types that manage no raw resource declare no special members. A bug that cannot exist needs no test. This is also our object-orientation: small classes with invariants established in the constructor and preserved by every diff --git a/taicpp/docs/whatsapp.md b/taicpp/docs/whatsapp.md index 9ac26ce..a49582f 100644 --- a/taicpp/docs/whatsapp.md +++ b/taicpp/docs/whatsapp.md @@ -523,3 +523,37 @@ The honest open item: the exact frame the user hit is still unreproduced in-session (the server did not resend it within a 160 s sanitizer run). The capture hook exists precisely so that gap closes on its next occurrence rather than by more guessing. + +### 13.7 Root cause, found and fixed (the capture hook paid off) + +The capture hook from 13.6 did its job: the next `--whatsapp-link` run recorded +the exact 726-byte frame to `/whatsapp/last-error.log`, and replaying it +made the bug deterministic in minutes. The verdict overturns every earlier +guess: **it was never a parser bug.** The frame decoded perfectly (it was a +normal `pair-success` with a `device-identity`, `device`, `biz`, and +`encryption-metadata`). The `std::vector larger than max_size()` was thrown by +`WaDevice::serialize()`, in `WaClient::handle_pair_success` saving the newly +paired device: + +```cpp +put_blob(b, Bytes((jid ? *jid : "").begin(), (jid ? *jid : "").end())); +``` + +`(jid ? *jid : "")` builds a **fresh temporary `std::string` for each +evaluation**, so `.begin()` and `.end()` point into two different, already +destroyed objects. The range constructor computed a garbage length and the +vector threw. It only misfired once `jid` was set, which is why it appeared +only after the phone scanned, and why the initial unpaired save and every +parser test passed. The fix binds the value to a named `const std::string` +before taking iterators. Verified by replaying the captured frame end to end +(serialize, save, encode the response: no throw) and by a paired-device +serialize round-trip test (`tests/whatsapp_test.cpp` `test_device`) that runs +under the sanitizer net. + +The lesson is in [cpp-safety.md](cpp-safety.md): this was a C++ object-lifetime +bug, a different class from the earlier size-overflow, and it slipped past the +parser-focused tests and the first sanitizer runs because those never exercised +`serialize()` with a set optional. Two defenses were strengthened as a result: +the capture hook (which turned an unreproducible failure into a deterministic +one), and the rule that a round-trip test must cover the populated case, not +just the empty one. The honest open item from 13.6 is now closed. diff --git a/taicpp/src/whatsapp/wa_device.cpp b/taicpp/src/whatsapp/wa_device.cpp index 37603f1..9758d35 100644 --- a/taicpp/src/whatsapp/wa_device.cpp +++ b/taicpp/src/whatsapp/wa_device.cpp @@ -140,8 +140,15 @@ Bytes WaDevice::serialize() const { put_u32(b, registration_id); put_blob(b, Bytes(adv_secret.begin(), adv_secret.end())); b.push_back(jid ? 1 : 0); - put_blob(b, Bytes((jid ? *jid : "").begin(), (jid ? *jid : "").end())); - put_blob(b, Bytes((lid ? *lid : "").begin(), (lid ? *lid : "").end())); + // Bind the optional's value to a named string before taking iterators: a + // `(jid ? *jid : "")` expression builds a fresh temporary for each of + // .begin() and .end(), so the range would span two different objects (UB, + // and the garbage length aborted the client after pairing). See + // docs/cpp-safety.md. + const std::string jid_s = jid.value_or(""); + const std::string lid_s = lid.value_or(""); + put_blob(b, Bytes(jid_s.begin(), jid_s.end())); + put_blob(b, Bytes(lid_s.begin(), lid_s.end())); put_blob(b, Bytes(push_name.begin(), push_name.end())); put_blob(b, account); return b; diff --git a/taicpp/tests/whatsapp_test.cpp b/taicpp/tests/whatsapp_test.cpp index 5f6f885..ad2ceff 100644 --- a/taicpp/tests/whatsapp_test.cpp +++ b/taicpp/tests/whatsapp_test.cpp @@ -11,6 +11,7 @@ #include "json.h" #include "wa_binary.h" #include "wa_crypto.h" +#include "wa_device.h" #include "wa_noise.h" #include "wa_proto.h" #include "wa_signal.h" @@ -337,6 +338,30 @@ static void test_hostile() { check(ok, "2000 random inputs: no length_error, no crash"); } +// ---- device serialize round-trip (regression for incident WA-2026-10-27) ---- +// A paired device serializes its optional jid/lid. The bug was a two-temporaries +// iterator range over `(jid ? *jid : "")` that only misfired once jid was set, +// so this test must cover the PAIRED case, not just a fresh device. Under the +// sanitizer net it also proves there is no lifetime UB in the path. +static void test_device() { + std::printf("[device]\n"); + WaDevice d = WaDevice::generate(); + d.jid = "15551230000:7@s.whatsapp.net"; // synthetic, not a real account + d.lid = "100000000000000:7@lid"; + d.push_name = "test"; + d.account = WaCrypto::random_bytes(186); + Bytes blob = d.serialize(); // used to abort here with length_error + auto back = WaDevice::deserialize(blob); + bool ok = back && back->jid == d.jid && back->lid == d.lid && back->push_name == d.push_name && + back->account == d.account && back->registration_id == d.registration_id && + back->jid_device() == 7 && back->jid_user() == 15551230000ULL; + check(ok, "paired device serialize/deserialize round-trip (jid+lid set)"); + // also the unpaired case (optionals empty), the other temporary branch + WaDevice u = WaDevice::generate(); + auto ub = WaDevice::deserialize(u.serialize()); + check(ub && !ub->paired(), "unpaired device serialize/deserialize round-trip"); +} + int main(int argc, char** argv) { DIR = argc > 1 ? argv[1] : "tests/whatsapp_data"; test_crypto(); @@ -344,6 +369,7 @@ int main(int argc, char** argv) { test_client_payload(); test_noise(); test_signal(); + test_device(); test_hostile(); std::printf("%s\n", g_fail == 0 ? "whatsapp: ALL OK" : "whatsapp: FAILURES"); return g_fail == 0 ? 0 : 1;