|
ProtoCore v0.0.1
Deterministic, zero-heap network stack for embedded targets
|
Hard rules for library code. If a change violates one of these, it is wrong - no exceptions, no "it was simpler," no "just this once." This file is the checklist a reviewer hammers me with.
Scope
src/** - fully constrained. Every rule below applies.examples/** - Arduino sketches, so Arduino APIs (Serial, WiFi.begin, delay(), millis(), IPAddress, String) are fine except networking: the socket rules (#6) apply here too. Use the library's own transport, never WiFiClient / WiFiUDP.test/** - anything goes (any C++ / STL / stdlib). Do not apply these rules to tests.Enforcement. ci_tooling/check/check_src_banned.py is the mechanical gate for the machine-detectable bans (#1 strlen, #2 <stdlib.h> / heap / parse functions, #3 auto, #4 delay(), #8 the non-reentrant gmtime family, #7 em-dashes, #22 virtual dispatch / RTTI). The pre-commit hook runs it on the staged src/ sources and refuses the commit on any hit; the Banned-constructs guard CI step runs it over the whole tree (--all). Comments and string literals are exempt, so only real code trips it. Bans that need human or diff-aware judgment (#5 bare millis() "for new timing", #6 networking, #9-#16) stay review items with the rg recipes below. Run it yourself with python ci_tooling/check/check_src_banned.py --all.
| # | Banned | Why | Use instead | Catch it |
|---|---|---|---|---|
| 1 | strlen | unbounded read on a non-terminated buffer | strnlen(p, cap), or carry an explicit length | ‘rg -n ’\bstrlen\s*(' src/\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 2 \ilinebr </td> <td class="markdownTableBodyNone"><stdlib.h>/<cstdlib>and everything in it -malloc/free/calloc/realloc,atoi/atol/strtol/strtoll/strtod,qsort,rand,abs\ilinebr </td> <td class="markdownTableBodyNone"> no heap afterbegin(); hidden allocation / locale-dependent parsing \ilinebr </td> <td class="markdownTableBodyNone"> fixed BSS buffers; hand-rolled integer/float parse;<stdio.h>(snprintf) and<string.h>(memcpy/memcmp/memset/strnlen) are allowed \ilinebr </td> <td class="markdownTableBodyNone">rg -n '#include <c?stdlib' src/\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 3 \ilinebr </td> <td class="markdownTableBodyNone">autokeyword \ilinebr </td> <td class="markdownTableBodyNone"> hides the type; obscures conversions \ilinebr </td> <td class="markdownTableBodyNone"> spell the explicit type (captureless lambda ->staticfn; capturing[&]->staticfn taking state explicitly). Suppress Sonar S5827, do not apply it \ilinebr </td> <td class="markdownTableBodyNone">rg -n '\bauto' src/\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 4 \ilinebr </td> <td class="markdownTableBodyNone">delay(...)\ilinebr </td> <td class="markdownTableBodyNone"> blocks the worker; not clock-pluggable \ilinebr </td> <td class="markdownTableBodyNone">pcdelay(ms)fromservices/clock.h\ilinebr </td> <td class="markdownTableBodyNone">rg -n '\bdelay\s*(' src/\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 5 \ilinebr </td> <td class="markdownTableBodyNone"> baremillis()for new timing \ilinebr </td> <td class="markdownTableBodyNone"> bypasses the pluggable clock \ilinebr </td> <td class="markdownTableBodyNone">pc_millis()fromservices/clock.h\ilinebr </td> <td class="markdownTableBodyNone">rg -n '\bmillis\s*(' src/\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 6 \ilinebr </td> <td class="markdownTableBodyNone">WiFiClient,WiFiUDP,AsyncUDP,ETH.*, any outside networking lib, and raw lwIP (udp_*/tcp_*/pbuf) outsidetransport/+tls/\ilinebr </td> <td class="markdownTableBodyNone"> breaks the OSI layering; ties code to one platform stack \ilinebr </td> <td class="markdownTableBodyNone"> **TCP:**pc_client_*(network_drivers/transport/client.h) -pc_client_open/_send/_read/_available/_is_closed/_close. **UDP:**pc_udp_*(network_drivers/transport/udp.h) -pc_udp_listen/_send/_sendto/_listener_sendto. **Applies toexamples/too.** \ilinebr </td> <td class="markdownTableBodyNone">rg -n 'WiFiClient|WiFiUDP|AsyncUDP' src/ examples/\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 7 \ilinebr </td> <td class="markdownTableBodyNone"> em-dashes (-U+2014) \ilinebr </td> <td class="markdownTableBodyNone"> house style \ilinebr </td> <td class="markdownTableBodyNone"> commas, parentheses, or a linking word \ilinebr </td> <td class="markdownTableBodyNone">rg -n $'—' src/ docs/\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 8 \ilinebr </td> <td class="markdownTableBodyNone">gmtime/localtime/ctime/asctime\ilinebr </td> <td class="markdownTableBodyNone"> non-reentrant (shared static) \ilinebr </td> <td class="markdownTableBodyNone"> the_rforms:gmtime_r/localtime_r. Never put a time bug in a test seam either \ilinebr </td> <td class="markdownTableBodyNone">rg -n '(gmtime|localtime|ctime|asctime)\s*(' src/\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 9 \ilinebr </td> <td class="markdownTableBodyNone"> casting an enum member toint/byte/uint8_tto make it compile \ilinebr </td> <td class="markdownTableBodyNone"> silences the type check \ilinebr </td> <td class="markdownTableBodyNone"> propagate the enum type through the call chain; cast **only** at the literal wire byte read/written \ilinebr </td> <td class="markdownTableBodyNone"> review any(int)/(uint8_t)on an enum \ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 10 \ilinebr </td> <td class="markdownTableBodyNone"> signed-overflow UB in hand-rolled parsers (v = v*10 + don a signed int,neg << 8) \ilinebr </td> <td class="markdownTableBodyNone"> UB; UBSan-fno-sanitize-recover=alltraps it \ilinebr </td> <td class="markdownTableBodyNone"> accumulate in anunsignedtype, then apply sign; guard against overflow \ilinebr </td> <td class="markdownTableBodyNone">-fsanitize=undefinedin the pentest build \ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 11 \ilinebr </td> <td class="markdownTableBodyNone"> uninitialized variables / out-params \ilinebr </td> <td class="markdownTableBodyNone"> analyzers + real bugs; areturn falseguard is **not** enough \ilinebr </td> <td class="markdownTableBodyNone"> initialize at declaration; write every out-param on **every** path, including the false-return path \ilinebr </td> <td class="markdownTableBodyNone"> Sonar /-Wmaybe-uninitialized\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 12 \ilinebr </td> <td class="markdownTableBodyNone"> file-scope mutable state not inside one owned<Name>Ctxstruct \ilinebr </td> <td class="markdownTableBodyNone"> scattered ownership -> interlayer bugs \ilinebr </td> <td class="markdownTableBodyNone"> put every mutable file-scope variable in a single owned context struct \ilinebr </td> <td class="markdownTableBodyNone">python ci_tooling/check/check_owned_context.py\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 13 \ilinebr </td> <td class="markdownTableBodyNone"> back-compat shims / compat aliases \ilinebr </td> <td class="markdownTableBodyNone"> this library prefers industry best practice over internal back-compat \ilinebr </td> <td class="markdownTableBodyNone"> make the breaking change; bump major \ilinebr </td> <td class="markdownTableBodyNone"> review \ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 14 \ilinebr </td> <td class="markdownTableBodyNone"> British spelling in identifiers / comments (color,initialize,behavior) \ilinebr </td> <td class="markdownTableBodyNone"> house style \ilinebr </td> <td class="markdownTableBodyNone"> American spelling everywhere \ilinebr </td> <td class="markdownTableBodyNone">cspell\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 15 \ilinebr </td> <td class="markdownTableBodyNone"> duplicated string literals; config/default strings inline \ilinebr </td> <td class="markdownTableBodyNone"> drift; unconfigurable \ilinebr </td> <td class="markdownTableBodyNone"> dedup to a namedconst; put config/default strings inprotocore_config.hunder aPC_ENABLE_*guard \ilinebr </td> <td class="markdownTableBodyNone"> review \ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 16 \ilinebr </td> <td class="markdownTableBodyNone"> a newsrc/file whose mutable state is not in a Ctx from the start \ilinebr </td> <td class="markdownTableBodyNone"> see #12 \ilinebr </td> <td class="markdownTableBodyNone"> design with the owned context up front \ilinebr </td> <td class="markdownTableBodyNone">check_owned_context.py\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 17 \ilinebr </td> <td class="markdownTableBodyNone"> a mid-file#include(an#includeafter any code) \ilinebr </td> <td class="markdownTableBodyNone"> makes the dependency graph read-order-dependent; hides layering \ilinebr </td> <td class="markdownTableBodyNone"> hoist every#includeto the top of the file. A vendor header belongs inboard_drivers/, never behind a conditional in the core. A genuinely ordered include that derives from earlier macros is exempt only with a justified// PC_ALLOW_LATE_INCLUDE: <reason>on the include line \ilinebr </td> <td class="markdownTableBodyNone">python ci_tooling/check/check_src_banned.py –all\ilinebr </td> </tr> <tr class="markdownTableRowEven"> <td class="markdownTableBodyNone"> 18 \ilinebr </td> <td class="markdownTableBodyNone">constexprfor a value other code sizes itself against, unless the standard being implemented dictates it \ilinebr </td> <td class="markdownTableBodyNone"> **symbol-kind mismatch.** A#defineis an untyped preprocessor token; aconstexpris a typed object the preprocessor cannot see. A capacity declaredconstexprcannot appear in an#if, cannot set another knob's#definedefault, and cannot fail at config time - its check degrades to astatic_assertfiring mid-compile. That is exactly whyPC_ENABLE_EDGE_MESHcould not be compiled at all. Separately, aconstexpr**function** is a permission rather than a mandate: it is only _allowed_ to fold, and may emit runtime evaluation with no diagnostic, unless the call site forces constant evaluation (initializing aconstexprvariable, an array bound, acaselabel) \ilinebr </td> <td class="markdownTableBodyNone">#defineit inprotocore_config.h<tt>(or the module header) under the usual#ifndefguard, so it participates in preprocessor arithmetic like every other knob and its value is fixed before the compiler runs \ilinebr </td> <td class="markdownTableBodyNone">check_src_banned.py\ilinebr </td> </tr> <tr class="markdownTableRowOdd"> <td class="markdownTableBodyNone"> 19 \ilinebr </td> <td class="markdownTableBodyNone"> a function-local array (a stack buffer) of any size \ilinebr </td> <td class="markdownTableBodyNone"> **stack is the one allocation the footprint cannot see.** Every other byte here is a fixed BSS pool sized at config time, so peak DRAM is a number you can compute before flashing. A local array is outside that accounting, and worst-case stack depth silently becomes whatever the deepest call chain happens to allocate - which is why the size of the array is irrelevant to the ban. There is no cost to complying, because the work buffers are already reentrant by construction \ilinebr </td> <td class="markdownTableBodyNone"> borrow it.ScratchScope+scratch_alloc()(session/scratch.h) for handler and I/O buffers, failing closed when the borrow returns null; acrypto_workregion at a fixed offset (crypto/crypto_scratch.h, region map inprotocore_config.h) for crypto leaf math, where an allocator call in an inner loop would cost more than the buffer.staticlocals are BSS and belong to #16. Justify a true exception with// PC_ALLOW_STACK_ARRAY: <reason>on the declaration line \ilinebr </td> <td class="markdownTableBodyNone">check_src_banned.py` (ratcheted baseline) |
| 20 | snprintf / vsnprintf (runtime format-string formatting) | a format string makes the CPU re-parse at runtime, on every call, what the code already knew at compile time. Measured on an S3 at 240 MHz: 2.4x on a real HTTP response header, 2.8x on a JSON object, 6.6x on a bare u, 8.0x on %.2f (see FEATURE_PERFORMANCE.md 2b for the table and method). It also drags the libc float formatter into the image whether or not a f is ever used. Note the one case that goes the other way: a pure-literal frame is faster through snprintf than through the frame engine, so do not wrap one in a spec | declare the frame: a static const pc_field[] spec in shared_primitives/frame.h plus one pc_frame_build / pc_frame_append call. The spec is pre-decoded data in rodata, so nothing is parsed at runtime and the engine that walks it is the only place a conversion can be wrong. For a short frame built by hand, the appenders it is built on are pc_sb in shared_primitives/strbuf.h - pc_sb_put / pc_sb_u32 / pc_sb_json / pc_sb_xml bump-append into a caller-owned buffer and latch ok = false the first time something would not fit, so overflow is one flag test at the end (pc_sb_finish) instead of a truncation nobody notices. Justify a true exception with // PC_ALLOW_SNPRINTF: <reason> | check_src_banned.py (ratcheted baseline) |
| 21 | a braceless body after if / else / for / while (if (x) return;, or the body on the next line unbraced) | one statement silently becomes two. Adding a line to an unbraced body puts it OUTSIDE the condition and the code still compiles, still looks right, and is wrong at runtime - the goto-fail class of bug. It also breaks every mechanical rewrite: converting one snprintf into a pc_sb frame build turned unbraced bodies into code that ran unconditionally, and only a declaration-in-a-braceless-body happening to be a hard C++ error made it visible at all | always brace the body. InsertBraces: true in .clang-format does this automatically and without changing semantics, so the fix is a reformat rather than an edit - it braced 7 553 of the 7 564 sites in one pass. What it cannot reach is a condition split across #if / #else, because the brace and its if land in different preprocessor branches | check_src_banned.py + clang-format |
| 22 | virtual functions, class hierarchies (: public), RTTI (dynamic_cast / typeid), and std::function | the call target is not in the binary. A virtual call reads a vtable pointer out of the object and jumps through it, so which function runs is decided by a value that does not exist until runtime. Nothing downstream can be established from the image: the worst-case path is unknown, the call cannot be inlined or devirtualized, and a corrupted object turns every later call through it into an arbitrary jump. That is the same nondeterminism this library refuses everywhere else, where the pools are fixed, the sizes are compile-time and the worst case is a number. RTTI adds an unbounded runtime type walk on top; std::function type-erases through an allocation | dispatch through a function pointer held in an owned context (ProtoHandler is the model: one table of {on_accept, on_data, on_close, on_poll} per protocol), or walk a spec table (pc_field). Both are visible to the linker whole, so the set of reachable targets is a closed list | check_src_banned.py |
The two bans meet on the same line, and the order is load-bearing. snprintf(buf, sizeof(buf), ...) is correct only while buf is an array: the moment #19 turns it into a borrowed pointer, sizeof(buf) silently becomes 4 and the call truncates to three characters, with no diagnostic from any compiler or checker. Converting to pc_sb first states the capacity explicitly at the builder's init, so the later pointer conversion has nothing left to silently change. Doing it the other way round means 592 chances to lose a buffer size without noticing.
Nothing gets allocated on the heap after begin() is the older and simpler of the two: every pool, arena and table is reserved during setup, and the steady-state server never calls an allocator. That is what makes the footprint a fixed number.
Ban #19 closes the other half. The heap rule alone still leaves the stack free to grow without anyone accounting for it, so "fixed footprint" would mean fixed except for whatever the deepest call chain happens to put in a frame. Reserving everything up front and then allocating working buffers per call is only half a guarantee.
Setup-time code is not the target of #19: a stack array inside begin() or a one-shot init path runs before steady state, and is exempt with a // PC_ALLOW_STACK_ARRAY: begin()-time only justification. The request path is where the rule bites, because that is the path whose worst case has to be knowable.
The obvious objection to hoisting a local buffer into shared storage is reentrancy: two tasks in the same function would then scribble on one buffer. Neither destination has that problem, and neither solves it with a lock:
scratch_alloc() is per-worker.** PC_SCRATCH_SLOTS equals PC_WORKER_COUNT, so a borrow comes from the arena belonging to the calling worker. Concurrent callers touch different memory. The arena is reset per dispatch, so a borrow cannot outlive the handler and a forgotten release cannot accumulate.crypto_work regions are disjoint by construction.** The region map hands each sub-primitive a fixed offset via a running total, so a caller and its callee never overlap, and the total is a compile-time number rather than a runtime allocation.src/services/<name>/<name>.{h,cpp} behind a PC_ENABLE_<NAME> flag (default 0), no sockets/crypto in the codec, with a native_<name> Unity suite added to test/test_matrix.json (regen with test/gen_test_envs.py).const char * (pointer binds to the type), minimal WHY-only comments that read like the author wrote them.