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;