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 <noreply@anthropic.com>
This commit is contained in:
2026-10-27 19:25:50 +01:00
co-authored by Claude Opus 4.8
parent d34db8f215
commit bfc117b0c5
4 changed files with 71 additions and 3 deletions
+2 -1
View File
@@ -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
+34
View File
@@ -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 `<home>/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.
+9 -2
View File
@@ -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;
+26
View File
@@ -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;