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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user