# Build and measurement

## Tooling

| Tool | Purpose |
|------|---------|
| `gcc` / `clang` | `-Wall -Wextra -Werror -Wconversion -Wshadow` from the first commit on a new project; on an existing tree, the repo's profile plus zero *newly introduced* warnings |
| ASan + UBSan | `-fsanitize=address,undefined -fno-omit-frame-pointer`: default for test builds |
| `valgrind --leak-check=full` | Leak and uninitialized-read detection where ASan cannot be linked |
| `clang-tidy` | Lint (`bugprone-*`, `cert-*`, `clang-analyzer-*`) |
| `cppcheck` | Second opinion; catches different classes than clang-tidy |
| `gdb` / `lldb` | `bt full`, `p *ptr`, watchpoints on corrupted fields |
| `clang-format` | Formatter, driven by the repo's `.clang-format`, never a personal preference |

Compiler warnings are the cheapest static analysis available, and a build nobody can get clean has no signal left in it. Turning `-Wconversion` or `-Werror` on globally over a mature tree produces thousands of unrelated failures, so raise the bar on the diff rather than the repository unless the whole codebase is in scope.

- A faulting load with a base register and a displacement is a field access, so name the field by counting the struct's offsets by hand with alignment and padding respected, and classify the base register against null, kernel space, the heap range, and the neighbourhood of the stack pointer. A non-null heap-shaped base whose displacement lands on unmapped memory fits both "the caller passed null" and "the object was freed between creation and use", and the fix that re-derives the pointer from a live anchor closes both, so prefer it over reproducing the exact trigger.
- Warning suppressions that vendored code needs must not ride the shared flag string. One `CFLAGS` disabling a warning class disables it for first-party code too, and the comment claiming the first-party code is clean without them is unverifiable in the build that carries them. Register vendored sources as their own target with their own flags, and keep the first-party set unsuppressed under `-Werror`.


## Testing

C has no dominant framework, so follow the repo's: Unity, Check, CMocka, Criterion, or plain assert-and-exit driven by the build. Whichever it is, run the suite under `-fsanitize=address,undefined` in CI, and make each new test fail against the unfixed code before accepting it.

- A quarantined test is a memory-safety blind spot, not a compatibility note. Whatever a skipped or expected-fail case exercises stops being watched by the sanitizer lane for as long as the skip lasts, and its label was written by whoever quarantined it, usually from a glance, so "known leak" is the standing guess for anything that merely looked wrong about memory. Re-run every skipped test touching lifetimes, ownership, or teardown under the instrumented build before trusting its label, and when one turns out to be a real fault, move it into the instrumented suite rather than leaving it skipped.

For generic test discipline (anti-patterns, real assertions, rationalization resistance), see the `ia-writing-tests` skill.


## Trusting the build

A verification result is a claim about a binary, not about a diff, and the two separate quietly.

- A failing test in untouched, correct-reading code: rebuild from clean before debugging (stale-artifact procedure: the `ia-debugging` skill's specialized-patterns reference).
- An incremental build can exit zero, echo the compile line for the file just edited, and still skip the link. Compare the artifact's timestamp against every source touched this session; neither the exit code nor the echoed command proves the binary changed. A result that matches the pre-edit behavior exactly is the tell, and forcing the rebuild sometimes surfaces a compile error the stale artifact had been hiding.
- Never take a copy of a configured tree as the second variant of a comparison. Dependency files written by the configure step hold absolute paths into the original tree, so the copy's build evaluates prerequisites against files nobody edited, compiles nothing, and links an artifact differing only in build metadata. Take a fresh checkout and re-run configure per variant, and confirm the change is actually present with one cheap behavioral probe, or an observable side channel such as an output size, before spending time measuring.
- A `CFLAGS=` handed to a configure script that inherits its base flags from elsewhere replaces them rather than appending, which silently drops the optimization level and produces a large unexplained regression from a flag that could not cause one. Keep the level explicit when adding a flag, and grep the generated makefile for it before believing any number the build produced.
- A call to a function the target platform does not declare is a warning, not an error. The compiler emits an implicit declaration, where an indirection-level or implicit-declaration warning at the assignment is the tell; the linker leaves the symbol undefined in a shared object; and `dlopen` succeeds because resolution is lazy, so the failure arrives as a symbol lookup error the first time that path runs, on the platform CI runs last. Build with `-Werror=implicit-function-declaration` and `-Wl,--no-undefined`, and exercise every conditionally compiled path on the oldest supported platform. Once a tree carries `#ifdef` splits for these calls, a caller added on a POSIX host compiles clean everywhere locally and breaks every Windows lane at once: every lane of one platform failing at compile while the others stay green is that signature, not a flake.
- A shared object is only self-contained on a machine that lacks its dependencies. `-fvisibility=hidden` hides nothing that a vendored header re-exports through its own `visibility("default")` macro, and `-static-libstdc++` and `-static-libgcc` are driver flags the C driver ignores, so a module that loads on the build host fails `dlopen` elsewhere on undefined `std::` or `__cxxabi` symbols. Gate the artifact instead: `nm -D --defined-only` shows no vendor symbols, `nm -D -u` shows no unexpected undefined ones, and the load-and-exercise check runs in a clean minimal container.
- Probe a library capability with a link test, not a header grep. A header newer than the installed library passes the grep and fails at link, and from GCC 14 and Clang 16 onward the implicit declaration is a hard error rather than a warning. Use the link-test macro in every build system the project ships, and where a capability is genuinely unavailable on one platform, state that platform consequence in the release notes.


## Measuring a change

- Never measure on a sanitizer or debug build. It inflates absolute time several-fold, which everyone remembers, and it distorts the ratio between implementations, which they do not: the per-access and per-allocation overhead falls hardest on allocation-heavy code, so a comparison against a differently-shaped competitor reads far better than it is. Worse, it systematically over-rewards the entire class of micro-optimization whose theory is "fewer allocation or append calls", which is how a genuine regression ships as a measured win. The sign flips on a release non-debug build, so check the optimization level too, not only the absence of a sanitizer.
- Interleave the two variants per round and carry a case the change cannot reach. A uniform move across untouched code is the harness, not the code: on a machine with mixed core types, one variant's heavy cases leave the core throttled for the other's, and run-to-run spread for a single unchanged binary reaches double digits where a quiet machine gives a few tenths of a percent. The failure does not look noisy; it is a clean table of consistent wrong numbers.
- A number stored from an earlier session is a different variant. Rebuild the old binary and measure it alongside the new one, because the comparison is what goes wrong, not either measurement.
- Instruction counts are deterministic and immune to frequency and core type, so use them to reject a candidate cheaply and to bound a claimed win, never to assert one: a removed load plus a predicted branch retires nearly free. Read a zero delta as "this instrument cannot see this change", which is the correct reading for anything that only alters allocation timing or buffer headroom.
- A measured win on a path the diff cannot reach is code layout, not the change. Confirm it on a second architecture, or rebuild the baseline with alignment flags only and watch the same case move by the same amount. Layout differences are deterministic per binary, so they produce large stable effects that survive any amount of repetition, and the resulting confidence is entirely misplaced.
- Removing instructions the processor was already hiding is context-fragile; removing repeated work from the always-executed path survives context. An isolated tight loop keeps the branch predictor trained and the working set resident, so it overstates the first kind and can overstate the ceiling of a hotspot that is not addressable at all. Validate in a realistic mixed workload before believing either.
- `__attribute__((optimize(...)))` is an inlining barrier, not a per-function optimization knob: the attributed function is not inlined into its callers and they are not inlined into it. It can isolate an optimization level for a self-contained local loop, cannot capture any win that depended on inlining, and pinning a hot function below its translation unit's level inserts a call wall that regresses past a uniform build at either level. Use a separate translation unit compiled at the other level.
- A/B a compile-time-selected path from one tree by injecting the disable macro through the compiler variable. Gate the `#ifdef` branch behind a single disable token and rebuild with `make CC='cc -DDISABLE_X'`, because re-running configure with a flags variable replaces the base flags, including the optimization level. Confirm from a runtime banner which path each build selected, assert that the two produce identical output, and carry a null control: an operation the gated path cannot reach must move by roughly 0% between the builds.

# C correctness traps that pass review

Bug classes that survive code review, compile clean, and often pass their own tests, because the failing case is a locale, a short read, an extreme input, or a platform the author never ran. Load when reviewing or writing code that formats numbers for machines, reads from a stream, derives a range from user input, or hands an integer to a foreign API.

Distinct from [memory-safety.md](./memory-safety.md): several of these end in memory corruption, but the root cause is arithmetic, environment, or an API contract rather than a lifetime or bounds mistake.

## Locale-dependent float formatting corrupts machine-parsed output

The whole `printf` float family (`%f`, `%g`, `%e`) honours `LC_NUMERIC`'s `decimal_point`. Under a comma-decimal locale (`de_DE`, `fr_FR`, `nl_NL`) `12.5` formats as `12,5`.

The locale is **process-global and shared across threads**. One `setlocale(LC_NUMERIC, ...)` anywhere in the process — application i18n, a library, a neighbouring request in a shared worker — flips it for every subsequent format call. The code that breaks never opts in and gets no error.

This is catastrophic where the output is machine-parsed, because a comma is a *structural* separator in the target grammar:

| Format | What a comma does |
|---|---|
| SVG | Coordinate separator. `points="12,5 30,8"` silently re-segments into four numbers instead of two `(x,y)` pairs: corrupted geometry, no parse error |
| JSON | Syntax error, or a different value |
| CSS, numeric config | Syntax error or silent misparse |

Human-facing **label text** is the opposite case: there `12,5` is correct localised display. The rule is per-purpose, not global. Machine-parsed numerics always emit `.`; display strings may localise.

Two fixes:

1. **Format then normalise.** Emit with `snprintf` as before, then rewrite the decimal separator to `.`. A fixed-notation result is `[-]digits<sep>digits`, so the separator is the lone run of bytes outside `[-0-9]`; collapse that run to a single `.`, which also handles a multi-byte separator. Rounding stays `snprintf`'s, so C-locale output is byte-identical and exact-string tests do not churn.
2. **Hand-roll** integer and fraction emission with `.` hardcoded. Also removes the `printf` parse from a hot path.

**Trap on composite formats.** Never run a whole `rgba(%d,%d,%d,%.3f)` result through a separator normaliser: the argument-separator commas are indistinguishable from the decimal comma. Emit the integer channels separately and pass only the fraction through.

Detection, since there is usually no `setlocale` call anywhere to find and the *absence* is the bug:

```bash
grep -nE '"[^"]*%[-+ 0-9.*]*[fgeFGE]' -r src/
```

Triage each hit by whether it feeds machine-parsed output (must be `.`) or display text (may localise).

## Read loops: index the destination by bytes, not by the running count

`&buf[n]` on a **typed** pointer scales by `sizeof(*buf)`. A chunked read loop that advances a typed pointer by its byte count writes far past the buffer:

```c
gdFont *font = malloc(sizeof(gdFont));
size_t b = 0;
while (b < hdr_size && (n = stream_read(s, (char *)&font[b], hdr_size - b)) > 0)
    b += n;                       /* BUG: &font[b] is font + b*sizeof(gdFont) */
```

`font` is `gdFont *`, so `(char *)&font[b]` is `(char *)font + b * 24` on LP64, not `+ b` bytes. Fix by casting first, then adding: `(char *)font + b`.

**The bug hides on a single full read**, because `b` jumps straight to `hdr_size` and the loop never iterates again. It only fires on a short read, where iteration 2 writes at `font + b*24`. If a sibling read in the same function already indexes a `char *` correctly, that asymmetry is the tell.

### Short reads are deliverable, so write the loop for them

Do not assume a read returns the full requested size until EOF. A partial read (`0 < n < requested`) is a normal outcome for network sockets, pipes, and any pluggable stream layer, and some implementations loop-to-fill for *some* backends only, which makes the partial case look impossible in local testing while remaining reachable in production. Every read loop must be correct under `0 < n < requested`, not just `n == size` or `n == 0`.

### A read returning zero or less is two different events

`n <= 0` collapses "the stream ended" and "the stream failed". A loop that breaks on it falls through to whatever completion code follows, so a failure part-way through commits the partially parsed prefix as though the input had ended there, silently and successfully. Classify before leaving the loop: negative is an error, and zero is an error unless the stream separately reports EOF. Layers below are not consistent about which they return, and a pluggable or user-supplied stream commonly reports failure as zero, so the EOF query is the only thing that separates them. Only a fault-injecting stream reproduces it; no real file will.

## Bounds arithmetic: validate before deriving

`end = start + count - 1` is undefined behavior when `start` is extreme, **even if** a later validation would have rejected it. The overflow happens first; the check never runs.

Correct shape for range writes and slice APIs:

1. Validate the start coordinate.
2. Compare the count, as an unsigned value, against the remaining capacity after the validated start.
3. Only then compute the end coordinate, now proven in range.

The same rule covers the mirror image, a guard that subtracts: every assertion or guard macro containing a subtraction of unsigned operands is a candidate for this audit, and because an unsigned wrap is defined behavior, no sanitizer flags the ones that slip through.

Probe with the type's extremes (`INT_MIN`/`INT_MAX`, `SIZE_MAX`) plus a multi-element input, under UBSan. A function that correctly returns "invalid" can still have signed-overflow UB on the way there.

## Wide integers narrowing into a foreign API

A 64-bit value that passes a `>= 0` check can still narrow to something entirely different in a call taking `int`. `4294967296` passes a sign check and arrives as `0`, selecting or destroying the wrong object.

Require `0 <= value <= INT_MAX` at every site where a wide integer crosses into a foreign `int`, enum, or ID parameter. Three refinements that each cost a review round when skipped:

- **Grep the vendor's enum header for negative members before applying a blanket `0..INT_MAX` guard.** Negative sentinels are usually valid inputs, and a naive guard rejects an API's own documented default. Where one exists, use `value < -1 || value > INT_MAX`, or check the parameter's declared default first.
- **The boundary is every crossing site, not the setters.** Constructors, `add*`/builder methods, rule and enum parameters all reach the same foreign `int`. Define the boundary as a predicate up front and fix every site in one pass; a constructor that throws needs the throwing form of the check, not the one that returns an error.
- **Listing call sites is not auditing them.** Script it. Parse each function body, collect the wide-integer variables, find those passed by value into a foreign call, and flag any whose body lacks an *upper* bound for that variable. A sign-only `< 1` or `<= 0` check must not count as validated. Run to zero and keep the script with the review notes, because eyeballing which sites are "already guarded" is what turns one review into six.

## A decoder's accept set must match its conversion arithmetic

A hand-rolled decoder makes two independent decisions, "is this byte acceptable" and "what does it decode to", and both must agree on the exact character set. A case-insensitive accept check (`isxdigit`, an `[0-9a-fA-F]` class) in front of a conversion branching on one case boundary (`c >= 'A' ? c - 0x37 : c - 0x30`) accepts more than the arithmetic handles: every byte in the gap passes validation and decodes to the wrong value, with no error, no rejection, and no crash. Lowercase hex is the usual gap, and encoders that emit uppercase by specification do not stop real input from arriving lowercase.

This is the opposite of over-lenient validation, which drops characters. Here validation is correct and the transform is narrow, so the corruption is silent and the round trip is what exposes it. Prefer the project's shared digit helper to a re-rolled magic-offset ternary, since the helper already covers every case; where one must be written, check the accept set against every case the arithmetic can be handed.

## A classifier predicate and a hand-enumerated switch drift apart

A tree that carries an `is_x(node)` predicate beside switches enumerating the same category by hand holds two definitions of one set, and adding a member updates whichever one the author was looking at. The switch that was missed falls through to its default arm and yields a value the caller assumed could never be missing, so the fault surfaces as a null dereference or a quietly dropped case somewhere else entirely.

When adding a member to any category that has a predicate, grep every site that enumerates that category and fix them in the same commit. Where a construct is resynthesized per pass, key any memo table on the stable original rather than on the regenerated copy, or the memo misses and the category question gets asked again of a different object. A null check at the dereference site is not the fix: it converts the forgotten case into a silent skip and leaves the two enumerations disagreeing.

## NUL truncation is accidental protection, and removing it resurrects the injection

A value carried as a NUL-terminated `char *` loses everything from the first embedded NUL, so a reader splicing it into protocol text never sees a payload hidden behind one. That truncation masks injection rather than preventing it, and nothing in the code says so.

Converting such a value to a length-carrying type for fidelity is the moment the masked surface goes live, and it needs two changes, not one. Replace `strcspn`-style scanning, which stops at the first NUL and therefore under-scans a length-carrying value, with `memchr` over the real length, taking the earliest terminator found. Then audit every reader of the value, classifying each by which append form it uses rather than by the field name: a length-aware append and a `strlen`-bounded one differ by one token and read as type cleanup in review. Each reader either gains the length-bounded scan or deliberately keeps the truncating form.

The test that proves the conversion did work carries a NUL before the terminator sequence. On the old code the value truncates, nothing fires, and the test is genuinely red before and green after; without that case the change looks like a no-op refactor. Before calling any such delta a regression, check whether the same sink is fed by other sources that were already length-aware and already unguarded: consistency with an unguarded sibling is a smaller finding than a fresh hole.

## A zero-length token underflows the length arithmetic and stops forward progress

A generated lexer whose condition has no default rule backtracks, on unmatched input, to the nearest accept state, and that can be a zero-length accept: the cursor never moves and the token length is zero. Two failures then compound. Unsigned length arithmetic in the rule body, `len = token_len - prefix_len`, underflows to an enormous size and reaches the allocator, and the unmoved cursor makes the next call match zero length at the same offset forever.

Clamping the subtraction fixes only the first and converts the crash into an infinite loop emitting empty tokens, so the fix belongs at the accept: give the unmatched input a rule that consumes at least one byte, either a default rule for the condition or a narrow rule for the offending characters. An allocation size near `(size_t)-N` is the signature of an unsigned underflow rather than a real request, and the generator's undefined-control-flow warning names exactly which input strings reach the undefined state.

## Function-like macros must not shadow caller variables

A macro that declares its own locals can shadow a caller's variable of the same name, and the argument expression then silently reads the macro's variable instead of the caller's:

```c
#define RETURN_FORMATTED(b) do {                       \
    char *s = alloc(36);                               \
    format36((b), s);                                  \
    return s; } while (0)
```

A caller whose own input is named `s` passes `RETURN_FORMATTED(get_bytes(s))`. After expansion, the argument resolves against the macro's freshly-allocated, uninitialised `s`. The output is garbage, often the recycled contents of a previous call's buffer, and therefore **nondeterministic across runs**.

This is a wrong-data bug, not a memory-safety one: it writes 36 bytes into a 36-byte buffer, so sanitizers stay silent, and a naive exact-output test cannot pin non-deterministic garbage. A round-trip identity assertion catches it immediately (`decode(encode(x)) == x`).

Two rules for any function-like macro that declares locals:

- Prefix every internal name so it cannot collide with a caller's variable (`_mod_tmp`, not `s`).
- Evaluate arguments into locals **at the top**, before declaring anything that could shadow them.

## Portability checks worth running before release

- **32-bit is a different program.** `size_t` is 32-bit there, so a 64-bit length from the wire truncates on assignment. `time_t` can be 32-bit independently of any other type's width, so epoch arithmetic overflows on inputs a 64-bit build handles. A 32-bit container is real coverage; a cross-compile that never executes is not.
- **POSIX-only functions that MSVC lacks**: `timegm` is the classic (`_mkgmtime` is the documented equivalent, same semantics). Never substitute `mktime`, which interprets the `tm` as **local** time and silently shifts results by the runner's timezone.
- **`const int` is a constant expression to GCC and Clang and not to MSVC's C front end.** `const int n = 10; int buf[n + 1];` compiles as a VLA on every POSIX lane and fails on MSVC with "expected constant expression". Use an enumerator or a function-scoped macro for any array bound.
- **Windows headers define `min` and `max` as macros**, which breaks any use of `std::min`/`std::max` and any templated code containing `(`. Define `NOMINMAX` before any Windows header, project-wide rather than per-file.
- **`a * b + c` is a fused-multiply-add candidate and `FP_CONTRACT` is on by default.** Whether the compiler emits one fused operation with a single rounding or a separate multiply and add with two is a property of the target, and the two results differ by a representable step at integer boundaries. A digit-accumulation parser (`v = 10.f * v + digit`) or any Horner-form evaluation therefore lands on a different value per machine, and a test asserting the specific branch that value selects fails only elsewhere. The fingerprint is a numeric test passing on older x86-64 baselines and failing on arm64 and on distributions that raised their baseline to require FMA. Reproduce it anywhere by toggling `-ffp-contract=off` against `-ffp-contract=fast`; the hardware is not needed to confirm the hypothesis. Assert the contract rather than the branch where both outcomes are correct, and pin contraction per translation unit only when a specific parsed value is load-bearing.
- **Hand-written ELF inline asm writes `call sym@PLT`, never a bare `call sym`.** Assemblers since binutils 2.31 emit a PLT-capable relocation for a bare branch, but some distributions carry a patch reverting that for branches, and the older relocation against a preemptible symbol cannot be resolved in a shared object: the link either demands a position-independent rebuild or produces a text relocation, which is a runtime segfault risk wherever indirect functions are in play. Compiler-generated code always emits the explicit form, so only hand-written asm is exposed, and the failure appears on one vendor's toolchain while every other CI lane stays green. The suffix changes the relocation and never the instruction encoding, so byte-pattern checks over the emitted sequence are unaffected and the object is identical on toolchains that already default to it.

# Implementation structure

## File and module layout

Every `.c` file in this order: file comment naming what the module owns; system includes, blank line, project includes; constants (enums first, `#define` for strings and conditional compilation only); types; prototypes for every static function, each with its contract comment; public definitions in header order; static definitions in call order.

Every `.h`: include guard, includes, constants, types, prototypes. What a header must not carry is a *definition* with external or tentative linkage, meaning a non-inline function body or a variable that allocates storage. A `static inline` definition is fine and is the only way to publish one; an `extern` declaration is fine and is sometimes required.

A reader who finishes the first screen holds the module's complete vocabulary and never meets an unresolved symbol.


## Naming

- Module prefix on every symbol with external linkage, and on statics too: `rb_push`, `net_send`.
- Functions are verb_object. Predicates start `is_`/`has_` and are never negated: `is_valid`, not `is_not_ready`.
- Lifetime pairs are exact and carry meaning: `_create`/`_destroy` implies heap allocation with ownership transfer, `_init`/`_deinit` implies caller-owned storage, `_open`/`_close`.
- Precise beats verbose: `retry_count`, not `number_of_connection_retry_attempts`.
- Name length scales with the distance between declaration and last use. `i` is fine in a five-line loop; anything crossing 20 lines gets a real name.
- Units live in the name: `TIMEOUT_MS`, `MAX_PAYLOAD_BYTES`.

Naming is the primary navigation channel for both greps and models, not decoration. A magic number is a fact with no grep anchor; a named constant is editable in exactly one place.


## Functions

Apply the name test **first**, before any decomposition rule below: if the most honest name for a candidate helper merely paraphrases its body, inline it and stop. A helper earns existence by naming a concept, owning an error value, or isolating a side effect. Nothing else counts.

Having passed it:

- One job per function. A contract comment needing the word "and" means two functions.
- Target 15 lines, hard cap 40. Nesting depth 2. Guard clauses first, happy path at the left margin.
- Parameter order: context pointer, outputs, pure inputs. A buffer and its length stay adjacent, buffer first. Past 4 parameters, the list is a struct trying to exist.
- No static locals except `static const` lookup tables.
- Classify every function as orchestrator (helper calls, status checks, branches on named predicates), leaf (straight-line logic calling only accessors and pure utilities), or adapter (wraps exactly one foreign call and translates its convention). Never a mix. Public visibility is a separate axis, not a fourth altitude.


## Control flow

- Early return over else chains.
- `goto` only where the repo sanctions it, or for one forward jump to one cleanup label when three or more interdependent resources are live. A `goto` whose label only returns is indirection buying nothing.
- Every `switch` case ends in `break` or an explicit `/* fallthrough */`. Require `default` when switching on an open-ended integer or an externally supplied value. On a closed internal enum, prefer *omitting* `default` with `-Wswitch-enum` enabled, because that is what makes adding an enumerator produce a warning at every switch that needs updating; a `default` silences exactly the diagnostic worth having. If the control flow needs proving, add a real `assert(0)`, never an unreachable annotation (see the UB table in [memory-safety.md](./memory-safety.md) for why). The consequence worth carrying here: because reaching one is UB rather than a diagnostic, a bug filed as "assertion failure on a debug build" is usually also a live user-visible bug on stock release builds, wearing a completely different symptom. A foreign library's enum is the opposite case, since the compiler cannot warn about members it was never shown: a `default` arm returning a plausible value such as null or zero turns every member the switch forgot into silent data loss, so make that arm fail loudly and re-enumerate the foreign enum against the switch on every dependency upgrade.
- A loop body over 10 lines becomes a named function.
- Give an explicit named bound to every loop whose trip count comes from untrusted or externally-supplied data. Traversals bounded by a structure's own size invariant (`while (fgets(...))`, a list walk, a scan to a terminator) do not need one; name the invariant in a comment or an assert instead. A deliberate event pump carries a comment saying exactly that.
- No recursion over externally-supplied input. Convert to a loop over an explicit bounded worklist: stack depth becomes visible and termination checkable. Unbounded recursion over attacker-controlled nesting is a live CVE class in parsers and serializers.
- No side effects inside conditions. No assignment inside `if`. No nested ternaries.


## Errors

- Every fallible function returns a status. Adopt the project's type if one exists; otherwise one enum per module, success 0 and named (`RB_OK`), values prefixed (`RB_ERR_ALLOC`).
- Never return `bool` from anything that can fail more than one way.
- Never mix errno-style and enum-style inside module code. Wrap libc at the boundary and convert once.
- Every fallible call is checked. Status propagates upward unchanged; only the top of the chain logs, converts, or decides.
- Minimize producers per error value. `grep RB_ERR_FULL` landing on one producing line turns a failure report into a location.


## Types and data

- Pick the type from the value's domain. Exact-width types (`uint32_t`, `int64_t`) where the representation is externally fixed: wire formats, file layouts, registers, exact modular arithmetic. `size_t` for object sizes, counts, and indices; `ptrdiff_t` for pointer differences. Ordinary `int` is correct for an ordinary counter or status whose guaranteed range suffices, and churning established `int` usage to exact-width changes ABI and warning behavior for nothing.
- `const` on every pointer parameter not written through.
- Initialize every object at declaration, and declare it at the smallest scope and the latest point where its first value is already valid.
- One level of dereference per expression. `a->b->c->d` smuggles three lifetimes and three nullability questions into one term; bind intermediates.
- Every union carries a tag. Structs use designated initializers, and any invariant tying two fields together is stated in a comment above the struct.
- Function pointers belong in `static const` dispatch tables, or as a documented callback parameter (a `qsort` comparator, a visitor walk). What to avoid is a function pointer stored loose in mutable state, where the reachable targets cannot be enumerated from the code.


## Macros

Uppercase names, every argument and the whole body parenthesized, multi-statement bodies in `do { } while (0)`. No macro evaluates an argument twice. Prefer `static inline` wherever types allow.

Hidden control flow inside a macro makes visible code lie about its own paths, so a macro containing `return`, `goto`, `break`, or `continue` is banned unless the project already sanctions one. Where a project has none and unchecked calls are a recurring bug, a single `MODULE_TRY(expr)` beside the status enum is a defensible exception, restricted to functions that acquire nothing. Adding a second hidden-return mechanism to a codebase that already has one is a net loss.


## Refactoring existing C

Short, flat, guarded, and free of magic numbers is not the same as done. Before accepting an existing function, run the near-miss test: duplicated mutation, data encoded as control flow, interleaved concepts, declarations sitting above their first valid value.

Judge any proposed refactor by the cost of the next change, not by line count. For the full three-stage worked example with its change-cost proof, and the deeper normative rules behind the sections above, load [legibility-standard.md](./legibility-standard.md).

# C legibility: the deep rules and a worked refactor

Supplement to the SKILL.md body. Load when refactoring existing C, when a review needs a rule stated precisely, or when deciding how far to push a decomposition.

Adapted from [write-legible-c](https://github.com/7etsuo/write-legible-c) by 7etsuo (MIT), with the repository-level and formatting sections dropped and the decomposition guard strengthened.

## Why these rules exist

Every rule targets one cost: the tokens and working memory needed to reason about any region of code. A rule earns its place by making a symbol greppable to few sites, keeping an edit local, or removing a path the reader must simulate.

The empirical case, briefly:

- Stripping identifiers degrades model performance even on execution tasks that should depend only on structure, so naming is a semantic channel and not decoration (arXiv 2510.03178). Human comprehension studies attribute up to a 30 percent effect to good names (Feitelson et al., ICPC).
- Models generate better against precise concise identifiers than verbose composites (arXiv 2508.06414).
- Model-perceived complexity, driven by semantic hierarchy depth and branching breadth, correlates with task performance after controlling for code length; semantics-preserving rewrites that reduce it improve downstream results (arXiv 2602.07882).
- Standard complexity metrics predict whether generated code passes, which is why the function budget below is numeric rather than a feeling (arXiv 2505.23953).

Bounded loops, the recursion restriction, the assert-density rule, smallest scope, and checking every return come from Gerard Holzmann's Power of 10 (NASA/JPL, 2006). Three of its rules are deliberately weakened here, because they are flight-software law rather than general practice: the ban on dynamic allocation after initialization is not adopted at all; the two-assertions-per-function floor is reduced to one assert per state-mutating leaf; and the statically-bounded-loop rule is scoped to loops driven by external input, since ordinary I/O and container traversal in hosted C is bounded dynamically.

MISRA C and CERT C are adopted in spirit: no reliance on undefined behavior, every warning an error. Their single-exit-point rule is rejected, because early returns are what keep branch depth low.

## The decomposition guard

State this before any splitting rule, and apply it as a filter on every proposed helper:

> If the most honest name for a candidate helper merely paraphrases its body, inline it and stop.

A helper earns existence by naming a concept, owning an error value, or isolating a side effect. Nothing else counts. Applied late, the line-count and nesting rules below produce ravioli code, where a hundred two-line functions turn every read into a pointer chase, and the cure is worse than the disease.

The one sanctioned exception is a pair of accessors that own every touch of one storage layout, so an indexing or locking convention lives in two adjacent lines and "what mutates this" greps to one answer. That is isolating a side effect, not paraphrasing a body, but it is the only shape where a one-line accessor survives the test.

## Cognitive complexity budget

Target 8 per function, hard cap 15, measured by the Sonar rules that charge each break in linear flow and charge nesting progressively with depth (G. Ann Campbell, SonarSource, 2017/2023). Code that already conforms to the depth cap and the decomposition guard sits far under budget. The metric is a tripwire, not a goal, and a function that trips it is asking for a name, not a `#pragma`.

## Full pre-delivery checklist

Run against the final diff. This is a gate, not a substitute for reading each section.

Two standing exemptions, because a metric must never drive an interface change. The numeric items below are tripwires for new or substantially rewritten *internal* code; they do not license restructuring untouched legacy code that a scoped fix merely passes through. And no item here justifies altering a frozen public signature — a released API, an ABI-stable export, a wire contract, or generated code. Where a rule and a frozen boundary collide, the boundary wins and the deviation gets a comment at the declaration.

1. Any literal that is not 0 or 1? Name it.
2. Any function over 40 lines, or nested past depth 2? Split it.
3. Any contract comment containing "and"? Split the function.
4. Any `goto` the repo does not already sanction? Decompose, or justify at the label.
5. Every fallible call checked and propagated?
6. Prototypes at the top of the file, matching every definition?
7. Each new error value: how many producers? Reduce toward one.
8. Any logic pasted twice? Extract it.
9. Any parameter list past 4? Struct it.
10. Header exposes only what callers need?
11. Any function mixing helper calls with inline logic? Push the logic into a leaf.
12. Any helper whose name paraphrases its body? Inline it.
13. Parameters out of context, outputs, inputs order? Reorder.
14. Any loop driven by external input without a named bound, or a nonterminating loop without a marker? Bound or mark it.
15. Any recursion reachable from external input? Convert to a bounded worklist.
16. Any state-mutating leaf with zero asserts? State the invariant.
17. Any pointer parameter not written through that lacks `const`?

## Worked refactor: good but not good enough

Most C written by a model fails subtly, not grossly. The function below passes generic review: short, flat, guarded, no magic numbers. It still fails.

```c
uint16_t map_eat(map_t *map, map_pos_t pos)
{
    map_cell_t cell;
    if (map == NULL)
        return 0;
    if (!map_is_inside(pos))
        return 0;
    cell = map->cells[pos.row][pos.col];
    if (cell == MAP_CELL_PELLET) {
        map->cells[pos.row][pos.col] = MAP_CELL_EMPTY;
        map->pellet_count--;
        return MAP_SCORE_PELLET;
    }
    if (cell == MAP_CELL_POWER) {
        map->cells[pos.row][pos.col] = MAP_CELL_EMPTY;
        map->pellet_count--;
        return MAP_SCORE_POWER;
    }
    return 0;
}
```

The tells, in order of weight:

1. **The consume block is pasted twice.** Clearing the cell and decrementing the count is one concept written in two places. The moment the second branch was written, `map_consume_cell` should have been born. An editor later adding a side effect to consumption, a sound cue or a dirty flag, patches one copy and misses the other, because nothing links them.
2. **The branches encode data as control flow.** Cell type to score is a mapping, not logic. A mapping belongs in one lookup leaf, where the next cell type costs one line instead of one pasted block.
3. **Three concepts interleave in one body**: deciding edibility, awarding score, and mutating the map. No single question about this function has a single home.
4. **`map_cell_t cell;` sits uninitialized above the guards.** Declare at the point where the first valid value exists.

### Stage one: decompose, signature preserved

```c
/* True when the cell can be eaten. Pure. */
static bool map_cell_is_edible(map_cell_t cell);

/* Score for consuming a cell. Zero for inedible cells. Pure. */
static uint16_t map_cell_score(map_cell_t cell);

/* Empties the cell and updates pellet accounting. */
static void map_consume_cell(map_t *map, map_pos_t pos);

uint16_t map_eat(map_t *map, map_pos_t pos)
{
    if (map == NULL)
        return 0;
    if (!map_is_inside(pos))
        return 0;

    map_cell_t cell = map->cells[pos.row][pos.col];
    if (!map_cell_is_edible(cell))
        return 0;

    map_consume_cell(map, pos);
    return map_cell_score(cell);
}

static bool map_cell_is_edible(map_cell_t cell)
{
    return cell == MAP_CELL_PELLET || cell == MAP_CELL_POWER;
}

static uint16_t map_cell_score(map_cell_t cell)
{
    switch (cell) {
    case MAP_CELL_PELLET:
        return MAP_SCORE_PELLET;
    case MAP_CELL_POWER:
        return MAP_SCORE_POWER;
    default:
        return 0;
    }
}

static void map_consume_cell(map_t *map, map_pos_t pos)
{
    map->cells[pos.row][pos.col] = MAP_CELL_EMPTY;
    map->pellet_count--;
}
```

**The proof is change cost, not line count.** Add a fruit cell: the original grows a third pasted block and the next editor patches two of three copies. The refactor grows one line in `map_cell_is_edible` and one in `map_cell_score`. Grep improves the same way, because "what mutates cells" now has exactly one answer.

If edibility is exactly "scores nonzero", both pure leaves collapse into one `static const` score table indexed by cell type, with zero branches. State that invariant in a comment above the table when taking that step.

### Stage two: separate failure from result

Stage one kept one violation deliberately: the signature fuses failure with score, returning 0 for a NULL map, an out-of-bounds position, and an ordinary empty cell alike. A caller cannot distinguish a bug from a normal move.

```c
/* Consumes the cell at pos if edible. Writes the score awarded,
 * zero when nothing edible is there. Fails with MAP_ERR_ARG on a
 * NULL pointer and MAP_ERR_BOUNDS on an out-of-range position. */
map_status_t map_eat(map_t *map, uint16_t *out_score, map_pos_t pos);
```

```c
/* Consumes the cell at pos if edible. Returns the score awarded,
 * zero otherwise. */
static uint16_t map_consume_at(map_t *map, map_pos_t pos);

/* True when the cell can be eaten. Pure. */
static bool map_cell_is_edible(map_cell_t cell);

/* Score for consuming a cell. Zero for inedible cells. Pure. */
static uint16_t map_cell_score(map_cell_t cell);

/* Empties the cell and updates pellet accounting. */
static void map_consume_cell(map_t *map, map_pos_t pos);

/* The only two functions that touch cell storage. */
static map_cell_t map_cell_at(const map_t *map, map_pos_t pos);
static void map_set_cell(map_t *map, map_pos_t pos, map_cell_t cell);

map_status_t map_eat(map_t *map, uint16_t *out_score, map_pos_t pos)
{
    if (map == NULL || out_score == NULL)
        return MAP_ERR_ARG;
    if (pos.row >= (size_t)MAP_ROWS || pos.col >= (size_t)MAP_COLS)
        return MAP_ERR_BOUNDS;

    *out_score = map_consume_at(map, pos);
    return MAP_OK;
}

static uint16_t map_consume_at(map_t *map, map_pos_t pos)
{
    map_cell_t cell = map_cell_at(map, pos);
    if (!map_cell_is_edible(cell))
        return 0;

    map_consume_cell(map, pos);
    return map_cell_score(cell);
}

static void map_consume_cell(map_t *map, map_pos_t pos)
{
    assert(map->pellet_count > 0);
    map_set_cell(map, pos, MAP_CELL_EMPTY);
    map->pellet_count--;
}

static map_cell_t map_cell_at(const map_t *map, map_pos_t pos)
{
    return map->cells[pos.row][pos.col];
}

static void map_set_cell(map_t *map, map_pos_t pos, map_cell_t cell)
{
    map->cells[pos.row][pos.col] = cell;
}
```

`map_cell_is_edible` and `map_cell_score` are declared above and carried over unchanged from stage one; their bodies are omitted here only to keep the listing focused.

What the final stage bought, item by item:

- Every failure has a name and exactly one producing site, and normal gameplay no longer wears an error's clothes: an empty cell is `MAP_OK` with score zero.
- `map_cell_at` and `map_set_cell` own every touch of cell storage, so the row-major indexing convention lives in two adjacent lines. This is the sanctioned accessor exception from the decomposition guard above, and it is the only reason those one-line helpers survive the name test.
- `map_consume_cell` asserts the accounting invariant instead of re-validating, because the public boundary already proved the arguments.
- Parameter order follows context, outputs, inputs throughout.

Stage two also forced two amendments to the function rules: orchestrators may branch on named predicates, and leaves may call the module's own accessors. That is the meta-lesson. A standard is grown by feeding it code that breaks it, and each break becomes a rule or an amendment. Apply the same process to any codebase adopting this document.

### When to stop

Stage two is not automatically the target. Choose the stage by the task's scope:

| Situation | Stop at |
|---|---|
| Bug fix inside an existing function | The smallest behavior-preserving decomposition that removes the duplication causing the bug |
| Feature work touching the function | Stage one |
| The module's public API is being designed or is not yet frozen | Stage two |
| The signature is frozen by ABI, a wire format, or generated code | Stage one, with a deviation comment above the declaration |

# C memory safety and undefined behavior

Load when writing or reviewing code that allocates, parses untrusted input, does pointer arithmetic, or recurses. This covers the failure modes the legibility rules do not address: a perfectly legible function can still be a heap overflow.

## Auditing scope

Every hit from a grep for a dangerous primitive (`strcpy`, `sprintf`, `alloca`, unchecked `malloc` size arithmetic) gets exactly one disposition -- safe by construction, mitigated upstream, finding, or needs-trace -- never a sample from a large result set.

A vendored or submodule directory name does not establish ownership. Code modified in-tree is first-party and gets the full audit above; unmodified third-party code is traced only at the host-library bridge (the wrapper, the size conversion, the ownership handoff), never at its internal primitives. File no finding whose location exists only behind a gitlink -- that defect belongs to the submodule's own repository.

## Sanitizers

```bash
# Default test build. ASan and UBSan compose; MSan does not compose with ASan.
cc -g -O1 -fno-omit-frame-pointer -fsanitize=address,undefined ...

export ASAN_OPTIONS=detect_leaks=1:detect_stack_use_after_return=1:abort_on_error=1
export UBSAN_OPTIONS=print_stacktrace=1:halt_on_error=1
```

- **UBSan is non-fatal by default.** It prints a diagnostic and keeps going, so the process still exits 0 and the suite still passes green with undefined behavior in it. Set `halt_on_error=1`, or compile with `-fno-sanitize-recover=undefined`, or the sanitizer is decoration.
- **MSan (`-fsanitize=memory`) requires every linked dependency to be instrumented**, libc++ included. An uninstrumented library produces false positives that waste more time than the bug. Reach for it only in a fully instrumented build.
- **Valgrind where ASan cannot link** (a plugin loaded by an uninstrumented host, or a preloaded allocator). Slower, catches uninitialized reads ASan misses, and needs no rebuild.
- A custom pooling allocator hides bugs from both tools. Disable it for sanitizer runs, or the clean report means nothing.
- **GCC's `-fsanitize=undefined` group omits `float-cast-overflow`; clang's includes it.** Converting an out-of-range or NaN `double` to an integer type is undefined, and x86-64 `cvttsd2si` and arm64 `fcvtzs` saturate quietly instead of trapping, so the bug is invisible at runtime until an optimizer acts on the assumed-in-range value. Add `-fsanitize=float-cast-overflow` explicitly anywhere externally supplied numbers drive coordinate, index, or size arithmetic, and confirm the check is live against a one-line `(int)1e300` program before trusting a green run.
- **A clean sanitizer run is evidence only against a positive control.** Before reading a report-free run as absence of the bug, add a deliberate fault of the same class, allocate then free then read one byte, and confirm the run aborts on it. A mismatched allocator, an uninstrumented object, or a runtime that failed to interpose reports nothing for the real bug and nothing for the planted one, and the two look identical from the outside.
- **A shared object loaded into a sanitized host is checked only where it was itself compiled with the sanitizer.** The host's runtime still reports the stack overflow, because the signal handler is process-global, but a heap use-after-free executed by an instruction inside an uninstrumented plugin passes silently even though the freeing code poisoned the block correctly. Build the plugin with the same flags and verify with `nm <plugin> | grep -c __asan_`, which is zero on an uninstrumented build.
- **ASan and Valgrind cannot both instrument one process.** An ASan-linked binary aborts under Valgrind at shadow-memory setup, before any program code runs, and the whole suite then reports as leaks. Check the binary with `nm -D <bin> | grep -c __asan` before starting a Valgrind pass. On a machine carrying several builds, an install path's name says nothing about its instrumentation.
- **Match the sanitizer runtime family across a `dlopen` boundary.** A plugin built with one compiler's ASan will not load into a host linked against the other's, and preloading the shared runtime over a host that linked it statically aborts at startup with an incompatible-runtimes error. Read the host's family from `ldd` and build to match. Do not copy a flag string written for the other compiler either: `-fno-sanitize=function` and `-fno-sanitize=vptr` are clang spellings that GCC rejects, and inside a configure script that surfaces as the unrelated "C compiler cannot create executables".
- **Prefer a narrow leak-suppression list to disabling leak detection.** One-time registration tables and a vendored library's process-lifetime singletons report as leaks and tempt a blanket `detect_leaks=0`, which then hides every leak introduced afterwards. Suppress the known frames by substring instead, and silence the suppression summary: the leak checker prints it to stderr by default, where any test comparing exact output folds it in and fails on the extra lines, which reads as a leak failure and misdirects debugging effort. When a stripped binary yields no frames at all, the only workable entry is the library name, and a suppression that coarse also hides a forgotten release of that library's own handles in first-party code: state what the entry masks, cover that teardown path with a structural test, and drop the entry once a fixed build ships.
- **`detect_stack_use_after_return` is off unless the harness sets it**, so a fix whose only observable is a stale read of a returned frame has no red-before signal in any lane that leaves the default. Find the leak co-located on the same faulty path and assert that instead; leak detection is on whenever ASan is linked, so it is the reliable red-on-unpatched signal.
- **A use-after-free test that is green before the fix is usually missing ballast.** A pooling allocator leaves the freed bytes intact, so the stale consumer reads the original data and the case passes with and without the bug. Allocate a deterministic non-header payload after the borrowed temporary dies and before the first stale read, sized by allocation class rather than by exact length. State the result as "the ballast reclaimed and overwrote the stale buffer", never as "only an exact-length allocation triggers it", and back it with a run under a sanitizer with the pooling allocator disabled.
- **Make a growth-boundary use-after-free deterministic by pre-filling to capacity.** Construct the container with a small capacity, pad it through the public API until `count == capacity`, then invoke the operation under test: the next append is guaranteed to reallocate.
- **Settle undocumented copy-versus-reference semantics with a sanitizer rather than by reading the call.** Hand an object owned by container B into container A, destroy B, then use A under ASan: a clean run proves the API copied, a heap-use-after-free proves it took a reference. Guard only the sites the probe proves are references.
- **When observation makes the bug vanish, stop observing through I/O.** A timing- or layout-sensitive fault that reproduces under the real harness, exits clean under a debugger, and disappears the moment a `printf` or a verbose dump enters the hot path is being perturbed by the instrument's own syscalls and allocation timing. Record into a preallocated static array with plain stores, and dump it only at the fault from `__asan_set_error_report_callback`, where I/O is free. Carry an invocation counter in each entry: it separates a re-dispatch inside one call from a fresh call seeded by an earlier one, which is the difference between a control-flow bug and a stale-state bug.

`-fsanitize=fuzzer` plus a corpus is the highest-yield tool for any function parsing untrusted bytes. One overnight run finds what a review will not.

## Integer rules

Signed overflow is undefined; the optimizer is entitled to assume it never happens, which is how `if (x + 1 < x)` gets deleted. Unsigned overflow wraps, which is defined and still usually a bug.

- **Check before the operation, never after.** `if (b != 0 && a > SIZE_MAX / b) return ERR;` before `a * b`, or use `__builtin_mul_overflow(a, b, &out)` / `__builtin_add_overflow`. The `b != 0` guard is not optional when `b` is itself derived from input; the division traps otherwise.
- **Every size computed from input is a multiply waiting to wrap.** `malloc(count * sizeof(elem))` with attacker-controlled `count` is the classic heap overflow. Use an overflow-checked helper for every allocation whose size is not a compile-time constant.
- `size_t` is 32-bit on 32-bit builds. A `uint64_t` length field from the wire truncates silently on assignment. Validate against `SIZE_MAX` before narrowing.
- Integer promotion turns `uint16_t * uint16_t` into `int` arithmetic wherever `int` can represent every `uint16_t` value, which is every ordinary 32-bit-`int` target. The product can then overflow *signed* even though both operands were unsigned. Cast one operand to a sufficiently wide unsigned type before multiplying, not after.
- Build with `-Wconversion`. Most truncation bugs announce themselves there and nowhere else.

## Undefined behavior worth memorizing

| Pattern | Fix |
|---|---|
| Type-punning through a cast (`*(float *)&i`) | `memcpy` into the target type; the compiler elides it |
| Unaligned load through a cast pointer | `memcpy`, or a `packed` struct where the ABI guarantees it |
| Shift by a count `>=` the type width, or a negative count | Mask or check the count first |
| Left-shifting a signed value into or past the sign bit | Compute in the unsigned type. Keep the result unsigned, or range-check against the signed maximum before converting: an out-of-range unsigned-to-signed conversion stays implementation-defined through C23, which fixed the *representation* but not this conversion |
| Ordering (`<`, `>`) or subtracting pointers into different objects | Compare integer offsets instead. Equality (`==`, `!=`) between unrelated pointers is well-defined and needs no fix |
| Dereferencing one-past-the-end | Forming that pointer is legal; reading it is not |
| Passing `NULL` to `memcpy`/`memmove` with length 0 | Guard the call; UB even at zero length through C23 (C2y adopts N3322, which defines it) |
| `isalpha(c)` and the rest of `<ctype.h>` on a plain `char` | Cast through `unsigned char`: `isalpha((unsigned char)c)`. Every `ctype` function is defined only for values representable as `unsigned char` or `EOF`; plain `char` has implementation-defined signedness and is signed on x86 and x86-64, so any byte above 0x7F arrives negative. Unsigned-`char` targets such as ARM hide it, which is why this ships |
| Reaching `__builtin_unreachable()` / `std::unreachable()` | Not an assertion. It is a promise to the optimizer, so reaching it is UB on release and the compiler may fold the path into a neighbouring branch. Use `assert(0)` where a check is wanted |
| A non-`volatile` local modified between `setjmp` and `longjmp` | Declare it `volatile`. `-Wclobbered` (which rides on `-Wextra`) flags this, and the diagnostic is **function-scoped**: it covers every non-volatile local in a function that calls `setjmp`, not just statements lexically between the two, so hoisting code out of the guarded block does not silence it |

## Allocation and lifetime

- State ownership at the interface: in the name (`_create` transfers, `_init` does not) and in the contract comment (who frees, and on which paths).
- Check every allocation unless the project's allocator is documented as non-returning on failure.
- `calloc` when the caller will read before writing every field. A `malloc` plus partial init leaks whatever was on the heap into whatever reads it.
- On realloc failure, the original pointer is still valid and must not be leaked: assign to a temporary, check, then commit.
- Set a pointer to `NULL` immediately after freeing it when the containing object outlives the free. A later use-after-free then becomes a NULL deref, which crashes honestly. Note the trade: a double free of the NULLed pointer becomes `free(NULL)`, a defined no-op, so the bug is absorbed silently and ASan can no longer see it. Nulling buys a loud use-after-free at the cost of a quiet double free.
- **Free on exactly one path.** A function that frees a resource on some error paths and hands ownership on to a callee on others is where double frees live. Decompose so the acquiring function is also the sole releasing function, or use the project's `goto cleanup` idiom with one label.
- **A deferred free relocates the free site.** Converting an immediate release into "release when the last holder drops it" moves the free into whatever call drains the deferral, so every caller still holding the pointer across such a call becomes a fresh use-after-free. Grep every caller of the draining functions and ask, per caller, whether it dereferences the same pointer afterwards; pin it across the call using the same counter the deferral already exposes, or re-check through a side channel. The test that proved the original bug reaches a different entry point and will not catch these.
- **A pointer into a container's backing store does not survive a call that can run foreign code.** Caching the base and end of the storage before a walk and then invoking a callback, a destructor, or any user-supplied hook inside the loop is a use-after-free the moment that code inserts into the same container and forces a reallocation. The freed slot usually still reads as plausible bytes, so the visible symptom is merely wrong output and only a sanitizer turns it into a hard failure. Take a reference on the container for the duration of the walk, which also forces a copy-on-write for the aliased case, or re-derive the cursor from an index after every reentrant call. The guard is one refcount pair per container, not per element, so it is not a measurable cost.
- **Re-deriving the cursor is not re-validating the element.** A loop that type-checks a caller-supplied slot once and then invokes foreign code can find the same slot holding a different type on the next pass, so re-check the type after every point at which foreign code could run. A test that hands the container in by value may be unable to reproduce this at all under copy-on-write; mutate through a shared reference instead.
- **One reload does not cover a sibling read.** After a call that can reallocate, re-deriving the cached element pointer fixes the store and leaves any other operand still read through the old pointer: `f(p->a); f(p->b); p = reload; p->a = x;` still reads freed memory. Hoist every field the sequence needs into locals before the first reallocating call.
- **Pin the elements before iterating a set whose entries the loop can free.** Dropping a refcount inside the walk runs a destructor that can free a different element of the same set. Detach the set from its owner first so a reentrant removal is a no-op against a table nobody can reach, then make three passes: acquire a reference on every element, do the work, release every reference.
- **Unregister from an external registry while the handle is still valid, not at the registrant's own teardown.** Once the descriptor is closed it cannot be re-derived, and removing later by a remembered descriptor number cancels whoever inherited that number in the meantime. A kernel-side registration is the harder case: there is no userspace table to scan for a stale entry, and an interest registered against a descriptor that was later duplicated or inherited stays live after one copy is closed, so the registry keeps handing back a pointer to a freed registrant. Hook the resource's own free path and remove there, and cache the handle and the owning object directly on the registrant rather than re-deriving either at teardown, when the surrounding bookkeeping may already be invalidated.
- **An object being torn down must not be handed to code its own destructor calls.** A teardown that flushes buffers or invokes a user callback runs while the object is already marked dead, so exposing the handle to that callback lets it be captured in a trace or stashed for later, and the eventual release is a use-after-free plus a double free. Guard the exposure with the project's liveness predicate at the exposure point rather than suppressing the teardown work. An "in teardown" flag is the wrong guard, because the same flush runs on the ordinary close path where the object is live.
- **A copy helper that deep-copies conditionally and a destroy helper that frees unconditionally do not compose.** The pair is correct only when the source dies with the copy. Where the source outlives the borrow, the copy shares whatever the condition declined to duplicate and the destroy frees it out from under the source, leaving a dangling field in a live object. Borrow by taking references on the individual owned fields instead of duplicating the aggregate, and save those pointers locally so the release still works if the source itself is destroyed during the call.
- **Handing a buffer to an API that binds rather than copies rebinds its lifetime to the handle, not to its owner.** A deferred-binding call reads through the stored address at every later operation until the binding is replaced or the handle is freed, so releasing the buffer when its owning descriptor dies frees memory that is still bound. Track such buffers on the handle and release them after the handle is destroyed; resetting every binding from the owner's destructor unbinds the siblings too.
- **A container initialized with no element destructor owns nothing, and that is a contract binding every insertion site.** Destroying such a container releases its own storage and leaks every payload the entries point at, and an update against an existing key drops the previous value with no release at all, which is where the dangling alias comes from. The destructor argument and the insert call sites usually live in different files, which is why review misses the combination. Register a destructor, or prove all three: keys never collide, nothing owned is stored, and the container outlives every value in it.
- **Any structure a fault or signal handler can traverse must be mutated atomically, publishing only fully initialized nodes.** Publishing a node before its child pointers are set is a live reentrancy bug wherever the handler walks that structure: the first initializing write faults, the handler walks the half-built node, and the fault nests. Build the node in a local and publish it with one trailing store. The diagnostic shape is a *read* of NULL or near-NULL while 64-bit sanitizer lanes stay clean and only a compact-heap build crashes, which points at reentrancy rather than a use-after-free, so instrument the fault handler before attempting any lifecycle fix: a clean integrity walk disproves corruption and leaves reentrancy standing.
- **When a fix closes a use-after-free, name which allocation was freed early and ask what else that same scenario can free.** "The object was freed" and "the container's storage was reallocated" are different roots needing different guards, and a reference on the object does not cover the reallocation. A regression test covers only the variant its mutation triggers, so removing an element and appending one are not interchangeable: write one test per root, and distrust any note claiming a class of bug is fixed and covered without naming the interleaving it covers.

## Bounds and string handling

- A buffer and its length travel together, buffer first, and the length is a count of bytes with no implied terminator.
- `snprintf` returns the length it *would* have written. `if (n >= sizeof buf)` is the truncation check; ignoring the return silently truncates.
- Never hand `snprintf`'s return value to a length-taking call against the same buffer. Because it is the length that would have been written, an overshoot makes the copy read past the array into adjacent stack memory and emit it, which is a disclosure rather than a formatting defect, and an exact fill copies the embedded terminator into the middle of the output. Clamp to `sizeof buf - 1`, or drop the fixed buffer and append the pieces directly. The shape only fires when a format field can grow, so audit the numeric and string fields fed by parsed values first.
- `strncpy` does not NUL-terminate when the source fills the buffer, and it zero-pads the remainder when it does not. Prefer explicit `memcpy` plus an explicit terminator, or the platform's `strlcpy`.
- Never compute a bound from the data being bounded. Validate the length field against the actual remaining buffer *before* using it to index.
- Classifying a multibyte lead byte proves what the encoder intended, not what is present. A decoder that reads and consumes N continuation bytes on the strength of the lead byte over-reads at a truncated sequence, and because it also advances past bytes that were never there, it steps over the terminator and keeps walking adjacent memory into its own output: a one-byte over-read becomes an unbounded disclosure. Gate each branch on the continuation bytes themselves, left to right, so short-circuit evaluation stops at the terminator, and treat a malformed lead as a single byte rather than emitting a replacement and advancing anyway. Codec bugs cluster, so audit the inverse conversion in the same translation unit before closing the finding.
- When an API carries both a buffer start and a validated-region start, every backward walk floors at the validated one. The bytes below it are exactly the ones the validator refused to certify, so an unbounded back-up that trusts them to be well-formed units runs off the front of the allocation. Compare with `<=` against the floor rather than `==`, because a back-up that decrements before the guard re-evaluates overshoots by one. Fix every backward-walking site and every caller passing a floor in one pass; fixing one re-discovers the rest on the next audit. Where the API has no validated-region concept, the buffer start is the validated floor and the check is already correct.
- **Bounding one writer of a shared fixed buffer relocates the overflow.** Where several sequential writers fill one buffer, it overflows at whichever is still unguarded, and truncating the reported writer fills the buffer to the brim and hands the overflow to the next one. Bound every writer and abandon the whole operation at the first one that would exceed, or size the buffer dynamically. Place each check *before* its write and size it for the worst case (`assert(p + 5 <= end)` ahead of a varint loop), not for the bytes typically written.

## Untrusted input

Every parser reading a length-prefixed or nested format needs all five:

1. Validate the length prefix against the bytes actually remaining, before any read.
2. Validate any integer used as a size, index, or count against its real domain, before use.
3. Bound total allocation for one message, not just each field. A million valid 1KB fields is still a memory exhaustion.
4. Bound nesting depth explicitly, with a named constant.
5. Carry every partial token across a chunk boundary in parser state. A lookahead guarded by `i + 1 < len` that falls through to the default branch at the end of a chunk silently reinterprets the next chunk's first byte as ordinary content, and no whole-buffer test reaches it: the input has to be sized so the token straddles the read size exactly.

## Recursion to bounded worklist

Recursion over externally-supplied structure is a stack-exhaustion CVE waiting to be filed: a deeply nested document crashes the process before any length check fires. Convert it.

Two bounds are needed, and conflating them is the usual bug. **Nesting depth** is a property of the path from the root to the current node. **Worklist occupancy** is how many nodes are pending at once, which grows with *breadth*, not depth. A worklist of bare node pointers tracks only occupancy, so using its index as a depth limit rejects a flat tree with many children while letting a deep narrow one through. Carry the depth in the frame:

```c
enum {
	WALK_MAX_DEPTH   = 128,   /* longest root-to-node path */
	WALK_MAX_PENDING = 1024   /* nodes queued at once; bounds stack use */
};

typedef struct {
	node_t *node;
	size_t  depth;            /* 1 at the root */
} walk_frame_t;

/* Visits every node reachable from root. Fails with ERR_DEPTH when the
 * structure nests deeper than WALK_MAX_DEPTH, and ERR_CAPACITY when more
 * than WALK_MAX_PENDING nodes are pending at once. A NULL root visits
 * nothing and succeeds. */
static status_t walk_tree(node_t *root, visitor_fn visit, void *ctx)
{
	walk_frame_t stack[WALK_MAX_PENDING];
	size_t       pending = 0;

	if (root == NULL)
		return OK;

	stack[pending++] = (walk_frame_t){ .node = root, .depth = 1 };
	while (pending > 0) {
		const walk_frame_t frame = stack[--pending];

		status_t s = visit(frame.node, ctx);
		if (s != OK)
			return s;

		if (frame.node->child_count > 0 && frame.depth >= WALK_MAX_DEPTH)
			return ERR_DEPTH;

		for (size_t i = 0; i < frame.node->child_count; i++) {
			if (pending >= WALK_MAX_PENDING)
				return ERR_CAPACITY;
			stack[pending++] = (walk_frame_t){
				.node  = frame.node->children[i],
				.depth = frame.depth + 1,
			};
		}
	}
	return OK;
}
```

The conversion buys three things: stack use is a visible constant rather than a function of input, each limit is a named error instead of a crash, and the loop bound is statically evident. Where either cap must be large, allocate the worklist on the heap and keep both explicit bounds; dropping the occupancy bound just moves the exhaustion from the stack to the heap.

Bounding the walker does not bound the teardown. A depth guard on a recursive dump, encode, or validate says nothing about freeing the structure afterwards, and releasing a deeply nested one recurses on the same stack, so the process dies after every assertion has already passed and the failure looks like a harness fault rather than a missing guard. Free iteratively by unlinking from the head, because dropping the last reference on a linked node can route it to a deferred collector that then recurses anyway. Reproduce it on an ordinary machine with `ulimit -s 1024` instead of waiting for the small-stack platform to report it.

## Verify

- Suite passes under `-fsanitize=address,undefined` with `halt_on_error=1`, zero reports
- Valgrind `--leak-check=full --error-exitcode=1` clean where the suite links under it
- Every allocation size derived from input goes through an overflow-checked computation
- Every length field from untrusted input validated against remaining bytes before use
- No recursion reachable from external input without an explicit depth bound
- `-Wconversion` clean

# PHP extension C

The dialect rules for C written against the Zend Engine. These **override** the corresponding SKILL.md sections. Load before applying the layout, macro, or error-model rules to any file containing `PHP_FUNCTION`, `zend_`, `php_*.h`, or a `config.m4`.

## How the base rules resolve here

Most base rules already defer to project convention; this is what that convention turns out to be. Only formatting and the type choice override outright.

| Base rule | How it resolves in an extension |
|---|---|
| Repo formatting (`.clang-format`) | **Tabs.** php-src `CODING_STANDARDS.md` mandates them; extensions follow, regardless of any local preference. |
| No macro containing `return`, unless the project sanctions one | The project sanctions several. `RETURN_*`, `RETURN_THROWS()`, and `ZEND_PARSE_PARAMETERS_END()` return; they are mandatory idiom, not violations. `RETVAL_*` is the one that does **not** return: it assigns `return_value` and deliberately continues, which is the whole reason both spellings exist. |
| `goto` only where the repo sanctions it | The repo sanctions it. `goto cleanup` is the standard multi-resource release idiom throughout php-src. |
| Adopt the project's status type, else one enum per module | `zend_result` (`SUCCESS`/`FAILURE`) already exists. Do not invent a parallel enum beside it. |
| Exact-width types only where the representation is externally fixed | Here the project types win outright: `zend_long`, `zend_ulong`, `size_t`, `zend_string *`. `zend_long` is 32 or 64 bit by build, so never assume `int64_t`. |

## Build and test loop

```bash
phpize && ./configure --enable-<ext> && make -j$(nproc)
make test                                    # runs .phpt files under tests/
TESTS=tests/foo.phpt make test               # single test
php -d extension=modules/<ext>.so -r '...'   # ad-hoc probe
```

On a host carrying more than one PHP install, run `phpize` from the exact install the extension will load into, never whichever one is first on `PATH`. It bakes the thread-safety model, the module API number, and the include paths into the generated build system, and handing `--with-php-config` to `./configure` afterwards does not undo any of that. The build then succeeds and only `dlopen` fails, reporting an undefined TSRM symbol for a ZTS mismatch or a module API message for a version mismatch. Recover with `make distclean`, then re-run `phpize` and `php-config` from the same prefix.

Tests are `.phpt`: `--TEST--`, optional `--SKIPIF--`, `--FILE--`, then `--EXPECT--` or `--EXPECTF--`. Prefer `--EXPECTF--` with `%d`/`%s` wherever output carries addresses, paths, or floats.

## Arginfo is generated, never hand-written

Signatures live in `<ext>.stub.php`. Regenerate with php-src's `build/gen_stub.php`, which writes `<ext>_arginfo.h`.

Editing `*_arginfo.h` by hand is always a bug: the next regeneration silently discards it, and the stub and the header disagree in the meantime. Change the stub, regenerate, commit both.

## Argument parsing

`ZEND_PARSE_PARAMETERS_START` must run **before any allocation or resource acquisition** in the function. It returns on failure, so anything acquired above it leaks on the error path.

```c
PHP_FUNCTION(ext_encode)
{
	zend_string *input = NULL;
	zend_long flags = 0;

	ZEND_PARSE_PARAMETERS_START(1, 2)
		Z_PARAM_STR(input)
		Z_PARAM_OPTIONAL
		Z_PARAM_LONG(flags)
	ZEND_PARSE_PARAMETERS_END();

	/* allocations start only after this line */
}
```

`Z_PARAM_OBJ_OF_CLASS` yields a `zend_object *`, not a `zval *`. Dereference user-supplied array elements with `ZVAL_DEREF` before any type check, or a reference slips past the check as the wrong type.

## Errors and exceptions

- In a `PHP_FUNCTION`/`PHP_METHOD` handler: throw with `zend_throw_exception_ex(...)` or `zend_argument_*_error(...)`, then **`RETURN_THROWS()` immediately**. Falling through after a throw runs code in an exception state.
- `RETURN_THROWS()` expands to a valueless `return`, so it is valid **only** in a void Zend handler. A `zend_result` helper that throws returns `FAILURE` (or its declared failure value) instead, and the handler translates that into `RETURN_THROWS()` at the boundary.
- After calling into userland (a callback, a magic method, `zend_call_function`), check three things in order: the call's own `zend_result`, then `EG(exception)`, then that the result zval is not `IS_UNDEF`. `zend_call_function` can return `FAILURE` without setting an exception, and it initializes the result to `IS_UNDEF`, so an exception check alone lets an undefined value through to be consumed or destroyed.
- Internal helpers return `zend_result`. Reserve `bool` for genuine predicates.
- A failed allocation does not return in the extension model: `emalloc` bails out with a fatal error. Do not write a NULL check that cannot fire. `pemalloc(size, 1)` does **not** restore a NULL return either, since it forwards to `__zend_malloc`, which calls `zend_out_of_memory()` on failure. When a recoverable, checkable allocation failure is genuinely required, drop to plain `malloc`/`free`. Use `safe_emalloc(nmemb, size, offset)` for the multiply-then-add case, which is about overflow-checked sizing, not recoverable failure.

## Macros that declare locals

A `RETURN_*`-style macro that declares its own `zend_string *s` shadows a `PHP_FUNCTION` parameter named `s`, so the macro's *argument* expression resolves against the macro's freshly-allocated buffer instead of the caller's input. The output is uninitialised heap, often a recycled previous result, so it is nondeterministic and a fixed `--EXPECT--` cannot pin it. Sanitizers stay silent because nothing is out of bounds.

Prefix macro internals so they cannot collide (`_ext_s`), and evaluate arguments into locals at the top before declaring anything. A round-trip identity `.phpt` (`from_bin(to_bin($x)) === $x`) catches this class instantly where an output-matching test cannot.

## C++ vendored libraries

An exception must never unwind into the Zend engine. Wrap every call into a C++ vendor library so the handler is a C-compatible boundary:

```c
try {
    vendor_call();
} catch (const std::exception &e) {
    zend_throw_exception(NULL, e.what(), 0);
    RETURN_THROWS();
} catch (...) {
    zend_throw_exception(NULL, "unknown error", 0);
    RETURN_THROWS();
}
```

The bare `catch (...)` is required: `catch (const std::exception &)` alone still lets a thrown `int`, a string literal, or a foreign exception type escape. For the general rules on C++/C boundaries, ABI stability, and symbol visibility, see the `ia-cpp-systems` skill.

## Memory

- `emalloc`/`efree`/`erealloc`: request-scoped, freed wholesale at request end. Default choice.
- `pemalloc(size, persistent)`/`pefree(ptr, persistent)`: survives the request. Anything that must outlive the request, including anything reachable from a persistent resource, has to be persistent-allocated.
- Request-scoped (`emalloc`) data may live in module globals, which is a standard pattern across php-src's own extensions, provided it is released **and the pointer reset** in `RSHUTDOWN`. What corrupts is a request-scoped pointer left in a global across requests: the next request reads freed memory.
- Never cross the allocators. `emalloc` pairs only with `efree`; `malloc` only with `free`.
- `zend_string` is refcounted: `zend_string_copy` to take a reference, `zend_string_release` to drop one. Interned strings have refcount handling of their own, so never `efree` a `zend_string` directly.
- `zval` ownership: `ZVAL_COPY` takes a reference, `ZVAL_COPY_VALUE` does not. `zval_ptr_dtor` on anything owned.
- A limit enforced by the request allocator counts only allocations that went through it. Bytes a bundled C library takes from `malloc(3)` are invisible to `memory_limit`, to the debug allocator, and to `memory_get_usage`, and no in-tree call installs an allocator hook for the XML stack. So any claim that `memory_limit` bounds an input-driven allocation is wrong wherever the bytes came from libxml, libxslt, GD, libzip, or ICU; the cap has to sit at the trust boundary, with an operating-system limit behind it. Parse and transform stages bypass it entirely, and only the copy back into a PHP string is ever counted.
- `RSHUTDOWN` is too early when the executor's own value teardown is what re-populates the container. A request-scoped table drained there can be refilled by a hook that runs afterwards, and `MSHUTDOWN` then reads memory the request pool has already released, which surfaces only under the tracked allocator because ordinary fast shutdown skips the per-value teardown entirely. `post_deactivate` is the hook that runs after the last producer and before the pool is freed. Destroy and reinitialize each table there rather than only draining it, so a re-entrant hook on a bailout path always meets a valid empty table instead of a half-freed one, and free the payloads first where the table carries no element destructor.

**Sanitizers need the Zend allocator disabled.** The pooling MM hides leaks and overflows from both ASan and Valgrind:

```bash
USE_ZEND_ALLOC=0 valgrind --leak-check=full php -d extension=modules/<ext>.so test.php
USE_ZEND_ALLOC=0 php ...          # with an -fsanitize=address build
```

This cuts one way only. A **clean** run with the pooling allocator enabled proves nothing, because the pool hides request leaks and allocator-local heap errors. A **positive** report is still a real finding: stack overflows, plain `malloc` misuse, invalid accesses that escape the pool, and unrelated UB all report accurately either way. Never dismiss a hit on the grounds that Zend MM was on.

## Custom objects

An object struct embeds `zend_object` **last**, and the handler offset is `offsetof(struct, std)`:

```c
typedef struct {
	/* fields first */
	zend_object std;
} ext_obj_t;
```

- Register handlers once in `MINIT`, copying `std_object_handlers` and overriding what changes.
- `free_obj` must call `zend_object_std_dtor` after releasing owned fields.
- A custom `create_object` without a matching `clone_obj` handler, or with `clone_obj` left pointing at the default, corrupts the heap on `clone`. Set it explicitly, including to NULL when cloning must be rejected.
- An abstract internal base is worth giving sentinel handlers that fail loudly, rather than leaving inherited ones that assume a concrete layout. Confirm the reachable paths against the engine version in use before relying on any specific one.

## Module globals

`ZEND_DECLARE_MODULE_GLOBALS(ext)` plus an accessor macro. Under ZTS the globals are per-thread, so never cache a raw pointer *to* a globals struct across requests or hand one to another thread; go through the accessor each time. Request-scoped contents are fine under the `RSHUTDOWN` discipline above.

## Assertions

`ZEND_ASSERT(cond)` compiles out unless the build is `--enable-debug`. It is the extension equivalent of the base skill's assert-density rule and carries the same zero release cost.

## Version compatibility

Guard on `PHP_VERSION_ID`, never on a runtime version string:

```c
#if PHP_VERSION_ID >= 80400
	/* 8.4+ path */
#endif
```

Keep the guard around the smallest region that differs. A guard wrapping a whole function duplicates the body and the two copies drift.

## Verify

- `make test` passes with zero `FAIL`, and any new `.phpt` fails without the change
- `USE_ZEND_ALLOC=0` run under Valgrind or ASan is clean
- `*_arginfo.h` matches a fresh `gen_stub.php` run
- No `emalloc`/`free` or `malloc`/`efree` crossing, and every request-scoped pointer in module globals is reset in `RSHUTDOWN`
- Every throw in a Zend handler is followed by `RETURN_THROWS()`; every throw in a `zend_result` helper returns `FAILURE` instead
- Every userland call checks its `zend_result`, then `EG(exception)`, then `IS_UNDEF`
- Every allocation sits below `ZEND_PARSE_PARAMETERS_END()`

# Runtime safety and interfaces

## Boundaries and assertions

Public entry points validate arguments and return the argument-error status. Internal statics do not re-validate; they `assert` their invariants instead. Every state-mutating leaf asserts at least one invariant.

An assert is a machine-checked comment: it states what must stay true and sits exactly where an editor is about to change something. Standard `assert` costs nothing in builds that define `NDEBUG` before including `<assert.h>`, which is a project decision rather than an automatic property of a release build. Where assertions stay enabled in production, assert meaningful invariants and stop chasing density. Validation duplicated at every level is noise that hides logic.

Check what a **project's own** assert macro degrades to before assuming it is free. A macro that becomes an *assume* rather than a no-op still evaluates its condition on some toolchains: clang's `__builtin_assume` and MSVC's `__assume` do not evaluate, but the GCC `__builtin_expect` plus `__builtin_unreachable` form does. So an assert whose condition calls a function in another translation unit emits a real call in a release build, silently paying back the check an optimization just removed, and it measures perfectly on clang while regressing on GCC. Wrap those in the project's debug-only conditional instead. Do not answer that by marking the called predicate `pure` so the optimizer can drop it. The attribute is a promise to every caller, not a local hint, and it licenses common-subexpression elimination across exactly the state changes a context-dependent predicate exists to observe.

Never discover a foreign container's end by incrementing an index until the accessor returns null. An accessor documented as a plain index may throw or terminate on an out-of-range argument instead of reporting one, so bound every traversal by the API's own count accessor. Bounds-check any index that came from the data as well: a tag byte lifted out of a payload and used to select a child is where untrusted bytes become a structural index.

Constructing a library's objects directly bypasses the validation its high-level path performed. Raw `create_*` constructors skip the range and encoding checks the convenience call made on the way in, so a narrowing constructor wraps silently and a text constructor forces binary through the string encoder. Re-implement those checks when hand-building objects for a bulk path, or keep the convenience path.


## Memory and lifetime

State ownership at the interface, in the name (`_create` vs `_init`) and in the contract comment. Treat allocation failure as a status, never an abort, outside `main`.

For sanitizer invocation, the integer overflow and truncation rules, allocation and lifetime patterns, the recursion-to-worklist conversion, and untrusted-input parsing discipline, load [memory-safety.md](./memory-safety.md).


## Correctness traps

Four shapes compile clean, pass review, and fail in production. Check for them by name:

| The code does this | The trap |
|---|---|
| Formats a number another program parses | The `printf` float family follows process-global `LC_NUMERIC`; one `setlocale` anywhere emits `12,5` into SVG or JSON |
| Reads from a stream | Short reads are normal, and `&buf[n]` on a typed pointer advances `n * sizeof(*buf)` |
| Derives a range from user input | `end = start + count - 1` overflows before the validation that would reject it |
| Passes an integer to a foreign API | A value that passes a sign check still narrows to something else |

Load [correctness-traps.md](./correctness-traps.md) for detection greps, fix patterns, macro shadowing, and the portability checklist.


## Discipline

- Preserve behavior and ABI unless a semantic change was requested. Use an adapter when a foreign API conflicts with a local rule.
- Consume a new flag bit after the existing bitfield members, never ahead of one. Inserting ahead of a published field shifts every following field's position for consumers built against the old header, while `sizeof` and the struct's member offsets stay unchanged, so the usual ABI evidence stays green. Append, and reserve spare bits when publishing a bitfield.
- On a released ABI, new state never goes into a public struct, not even into existing padding: consumers keep the old offsets, and maintainers reject the change regardless of how the layout happens to work out. Take the ladder instead, in order: a file-scope `static` in the using translation unit, a thread-local where the state is per-thread, an encoding into an existing field that the owner sanctions, or the next ABI-breaking branch. Audit the diff against the public headers of a stable branch before proposing it.
- When a required constraint forces a deviation, comment at the deviation site and state the constraint. A note in the delivery message does not replace a comment in the source.
- A frozen public signature that cannot return a status excuses the status rule and nothing else: internal asserts and every other locally satisfiable rule still apply.
- Relocating a fix from a call site into a shared helper widens the set of struct fields that helper reads, and every caller that satisfied the old contract by accident, by leaving a now-read field uninitialized, becomes a fresh bug. Audit all callers when a shared function starts reading a new field, not only the one that motivated the change; an initialization assert on the aggregate is a debug check, not a guarantee that callers zero every member.
- A fork that diverged on its data model cannot be merged from upstream, only cherry-picked into. Once the two trees disagree on field types, a pointer and length against an owned string or an enum against a narrow int, a three-way merge has no conflict to report: it resolves by picking one side and invalidates the other side's assumptions in every consumer. The dangerous part is an accessor that still compiles because both sides declare a member of that name. Port fixes by hand into the fork's shapes, and record each as a local patch naming its upstream origin.
- Do not claim compliance for checks that could not run. Name the command that did not execute.