From d34db8f21559c63ce7b91fcf26c8ad5b60572b50 Mon Sep 17 00:00:00 2001 From: retoor Date: Tue, 27 Oct 2026 19:04:11 +0100 Subject: [PATCH] cpp-safety: add the playbook doc and AGENTS rule (missed by *.md ignore) docs/cpp-safety.md (the four-layer safe-C++ playbook and design decisions) and the AGENTS.md standing rule were skipped by taicpp/.gitignore's *.md pattern in 79ae6f2; force-add them so the policy is actually in the tree, matching how the other docs are tracked. Co-Authored-By: Claude Opus 4.8 --- taicpp/AGENTS.md | 30 +++++++++ taicpp/docs/cpp-safety.md | 137 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+) create mode 100644 taicpp/AGENTS.md create mode 100644 taicpp/docs/cpp-safety.md diff --git a/taicpp/AGENTS.md b/taicpp/AGENTS.md new file mode 100644 index 0000000..9dcfec6 --- /dev/null +++ b/taicpp/AGENTS.md @@ -0,0 +1,30 @@ +# AGENTS.md — detz (taicpp) workflow + +Source of truth: `../tai.py` (untouched) + `../test_tai.py`, `../tai_test.py`. +Reuse: moloapp (`/home/retoor/projects/moloapp`) — clients/SSE, llm providers, app patterns. Vendor/link only, never modify. + +## Workflow (per milestone, in order) +1. Spec delta: update `docs/.md` + parity table vs tai.py symbols. +2. Code: headers `.h` + impl `.cpp`, one class per file pair, `-std=c++23 -Wall -Wextra`, warning-free. +3. Tests: `tests/*_test.cpp` mirroring python cases; `make test` green. +4. Changelog: prepend `CHANGELOG.md` entry. + +## Commands +- `make` — build `build/detz` +- `make test` — build + run all tests +- `make lint` — warnings + clang-format check +- `make version-bump PART=patch|minor|major` +- `make changelog MSG="..."` +- `make tag` — annotated tag `detz-v$(cat VERSION)` +- `make release PART=x MSG="..."` — bump+changelog+commit+tag (never push unless asked) + +## Rules +- No outputs outside `build/`. +- `src/cli/version.hpp` generated from `VERSION` at build. +- Max depth/layering, maximally OO. argparse tree must stay molo-subcommand-ready. +- Safe C++ is mandatory, per [docs/cpp-safety.md](docs/cpp-safety.md): any code that + parses untrusted bytes uses overflow-safe size checks (`len <= size - pos`, never + `pos + len`), reads lengths unsigned and range-checked before any allocation or slice, + guards every shift/loop bound, and lets no exception escape a top-level dispatch loop. + A new parser is unfinished without a hostile-input test and a sanitizer run + (`tools/san-whatsapp.sh` is the pattern). We do not accept regressions here. diff --git a/taicpp/docs/cpp-safety.md b/taicpp/docs/cpp-safety.md new file mode 100644 index 0000000..06d8d39 --- /dev/null +++ b/taicpp/docs/cpp-safety.md @@ -0,0 +1,137 @@ +# Safe C++ in detz: the playbook + +detz is written in C++ and will stay that way. The language gives us speed and +control; it does not give us safety for free. This document is how we get both: +fast, object-oriented C++ that a hostile or buggy peer can waste our time with +but can never crash, corrupt, or silently mis-handle. It is a standing contract, +not a suggestion. **We do not accept regressions in it.** + +It exists because of incident WA-2026-10-27: a single malformed WhatsApp frame +aborted the whole process with `std::vector larger than max_size()`. The +research behind this playbook is archived under +[`../../research/safe-cpp-practices/`](../../research/safe-cpp-practices/) +(OpenSSF compiler hardening, the C++ Core Guidelines bounds profile, integer +overflow practice, libFuzzer/sanitizer CI, the sanitizer overhead ladder, +clang-tidy ratchets). The full incident write-up is in +[whatsapp.md section 13](whatsapp.md). + +## The principle + +A bug that a tool can catch must be caught by a tool, not by a careful reader. +Careful review found this class of bug twice and missed it both times; an +AddressSanitizer build found a second instance of it in seconds. So our safety +does not rest on discipline alone. It rests on four layers, cheapest first, each +backed by something that fails loudly when it is violated. + +## Layer 1: design out the bug (zero runtime cost) + +This is where most safety is won. The rules below are mandatory for any code +that touches bytes we did not produce (network frames, files, user input, IPC): + +1. **Size arithmetic never overflows a bounds check.** Never write + `pos + len <= size` or `pos + len > size`: if `len` comes off the wire, the + sum can wrap `size_t` and the check passes on a garbage value. Always write it + as a subtraction against an established invariant: with `pos <= size` known, + check `len > size - pos`. For products use `a != 0 && b > SIZE_MAX / a`. +2. **A length is a claim, not a fact.** Validate every wire-supplied length, + count, and index against the bytes actually present (and against a static + application cap) *before* it drives an allocation, a `resize`/`reserve`, a + range constructor, or a slice. A caller must never be able to make us ask for + a four-gigabyte vector. +3. **Read lengths unsigned, reject negatives.** A 4-byte length read into a + signed `int` can sign-extend to negative and then become a huge `size_t`. + Read into an unsigned type wide enough to hold the value; reject negatives at + the boundary. Make every narrowing conversion explicit and checked, never an + implicit `int`->`size_t`. +4. **Guard every shift and loop bound.** A varint decoder must stop before it + shifts by >= 64 bits (that is undefined behavior); a repeat count must be + bounded. This is the bug the sanitizer caught after review missed it. +5. **No exception escapes a top-level loop.** Any loop that reads and dispatches + untrusted input wraps the per-item work in `try/catch (const std::exception&)` + and turns a failure into a logged, skipped item. An uncaught `std::exception` + in a parser or dispatcher is a bug even when the throw "cannot happen". +6. **Parse at the boundary into a checked reader, then work on trusted types.** + Concentrate all the raw pointer/length/offset math in one small, reviewed, + bounds-checked reader (detz's `WaBinary` decoder and `ProtoReader` are this). + Above it, code works on decoded structs and `std::string`/`std::vector`, never + 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 + 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 + method, not manual acquire/release. + +## Layer 2: release hardening (cheap, always on) + +The shipping binary keeps the Layer 1 checks plus the OpenSSF "fast" hardening +set, which is constant-time and meant for production: + +- `_GLIBCXX_ASSERTIONS` (libstdc++ precondition checks: container bounds, valid + iterators) so a residual bad index is a defined, located abort, not undefined + behavior that corrupts silently. +- `_FORTIFY_SOURCE=3`, `-fstack-protector-strong`, `-fstack-clash-protection`, + RELRO, PIE, noexecstack. + +These are cheap enough that Linux distributions ship them by default. GCC's +`-fhardened` (and Clang's, as it lands) bundles the whole set behind one flag, +and is the intended future simplification once the toolchain baseline supports +it. Hardening catches memory and fortifiable bugs; it does not replace Layer 1. + +## Layer 3: the sanitizer test net (free at dev time, run in CI) + +Sanitizers are too expensive for release (AddressSanitizer is ~2-3x, ThreadSan +~5-15x; UBSan ~1.25x), so they are the **test** configuration, not the shipping +one. Every parser of untrusted input has: + +- a **hostile-input test**: the exact malformed shapes that have bitten us or + could (oversized lengths, truncations, overlong varints), asserting each + throws a *handled* exception and specifically never `std::length_error` or a + crash; plus a **random fuzz** of thousands of inputs where surviving the loop + is the proof. +- a **sanitizer build** of that test under + `-fsanitize=address,undefined -fno-sanitize-recover=all -D_GLIBCXX_ASSERTIONS`, + which aborts at the exact line of any over-read, over-allocation, or UB. + +For detz::wa this is `tests/whatsapp_test.cpp` (`test_hostile`) plus +[`../tools/san-whatsapp.sh`](../tools/san-whatsapp.sh). Run the sanitizer script +in CI and before any change to a parser. A new parser is **not finished** until +it has both. Vendored third-party C with intentional, portable UB idioms (the +ed25519 ref10 signed shifts) is excluded from UBSan explicitly and only there. + +## Layer 4: capture, so an unreproducible bug is a one-shot fix + +A wire bug that only a specific peer or timing triggers cannot be fixed by +staring at code. So a frame that breaks a parser has its exact bytes recorded +(bounded) to a capture file, and the dispatcher logs the node shape. The next +occurrence yields the precise input, which goes straight into the hostile-input +corpus as a permanent regression test. This is the "keep a corpus of crashing +inputs" rule, scaled to a single-process client. + +## What we deliberately did not do + +- **Adopt GSL** (`gsl::span`, `gsl::narrow`, `gsl::not_null`) wholesale: it is a + new dependency, and CLAUDE.md favors fewer moving parts. `std::span`-style + checked views plus a small reviewed reader cover the need. We still follow the + Core Guidelines *rules* (span over pointer+length, explicit checked narrowing). +- **Run sanitizers in release**: too costly; they are the net, not the product. +- **Rewrite the wire layer in another language**: the goal is safe C++, and the + crypto/wire core is proven byte-for-byte against golden vectors. + +## The next ratchet (named, not yet wired) + +A `clang-tidy` gate (`bugprone-*`, selected `cppcoreguidelines-*`, especially +`bugprone-narrowing-conversions`) would flag an implicit `int`->`size_t` +narrowing at author time, before a sanitizer run. The industry pattern is a +ratchet: advisory on changed files first, then warnings-as-errors, enabling one +parked check at a time with its fixes. This is the planned next layer; it is +recorded here so it is not forgotten, and so nobody believes the current state +is the finished state. + +## The one-line rule for a reviewer + +If a line computes a size or index from data we did not produce, it is wrong +unless the check is a subtraction against a known bound, the value is unsigned +and range-checked before use, and a thrown exception upstream is caught. If a +parser has no hostile-input test and no sanitizer run, it is unfinished.