Tracking: C++ Core Guidelines conformance review

[aaylward]

Tracking issue from a project-wide review against the C++ Core Guidelines. Scope: all hand-written runtime C++ (runtime/include, runtime/src), the generated surface via the checked-in fixtures (examples/*/generated, each finding traced to the emitting generator class), test-support C++, and the enforcement tooling (.clang-tidy, CI, BUILD copts). Rules cited by guideline ID. Grouped by theme; each item can graduate to its own issue when picked up — #45 convention.

Consolidated 2026-08-25: #110 — a parallel pass of the same review, filed minutes after this one — is merged into this issue and closed as its duplicate. Its unique content now lives here: the §F enforcement posture section, the what-already-conforms-well inventory, and the suggested triage. Overlapping findings were already tracked below; nothing was dropped.

Accepted deviations, not findings: Outcome-based errors instead of exceptions (ADR-0003) and fail-fast contract violations (ADR-0009) are deliberate architecture, and Google style governs layout — the review treats these as the baseline and flags only places where those postures are applied inconsistently. Items already tracked in #45 are not repeated (the review independently re-confirmed several: unbounded Beast client response body beast_transport.cc:932, IPv6 endpoint splitting uri.cc:167-178, encode-side recursion depth in json.cc/cbor.cc, CRLF on the socket transport); two items below extend #45 entries and say so.

A. Silent data corruption (correctness first)

• [high] gzip feeds zlib a 32-bit-truncated input length (ES.46). gzip.cc:21 and :46 do stream.avail_in = static_cast<uInt>(data.size()) and never re-feed input in the loop. A 4 GiB + N byte body compresses only its first N bytes into a valid gzip stream — @requestCompression silently corrupts large uploads with no error anywhere. Reject oversized inputs up front or chunk next_in the way next_out already is.
• [high] Timestamp::Format(kEpochSeconds) mis-renders negative fractional instants (ES.*). timestamp.cc:264-266 floor-divides then appends the positive remainder: −500 ms formats as "-1.5", which the project's own ParseEpochSeconds reads back as −1500 ms. Every pre-1970 instant with a nonzero millisecond part is wire-corrupted and round-trip-broken (the Parse side is correct; only Format is wrong). Format the sign explicitly and decompose |ms_|.
• [medium] ParseEpochSeconds is locale-dependent via strtod, contradicting its documented contract (SL, I.2). timestamp.cc:212-215 pre-validates the grammar but converts with std::strtod, which honors LC_NUMERIC — under a comma-decimal locale the . stops the parse and fractional milliseconds are silently dropped (endptr is not checked). timestamp.h promises "no locale". Use std::from_chars (already relied on elsewhere) or convert from the digit scan the function already performs.
• [medium] ParseDateTime bypasses the checked range factory (I.5, ADR-0009 consistency). timestamp.cc:162 applies the UTC offset after civil-field validation and returns via unchecked FromEpochMilliseconds, so "0000-01-01T00:00:00+00:01" mints an instant outside the 0000–9999 window that the file's own comment says must be rejected — and which then Formats to unparsable text. Return CheckedFromMs(Compose(c) - offset_ms).
• [medium] SplitHeaderListValues splits inside quoted strings, contradicting its contract (I.7). headers.cc:91-105 splits on every comma; headers.h:45-48 promises quoted-string entries are returned verbatim. a="b,c", d yields a="b / c" / d — silent corruption of legally-quoted list-valued Smithy headers. Track quote/escape state or narrow the contract and audit callers.
• [medium] intEnum wire values truncate with no range or membership check (ES.46, Enum.1) — generator-systematic. byte/short/integer members get an explicit range rejection before their narrowing cast (SerdeCodeGen.java:202-217), but the INT_ENUM case (SerdeCodeGen.java:220-224) casts raw int64 straight into the enum class — e.g. serde.cc:361 in the jsonrpc fixture — and ValidationGenerator has no INT_ENUM case (string enums are validated). A wire value of 2^32+2 silently aliases onto a valid enumerator. Emit the same int32 bounds check, and optionally enum-set validation to match string enums. (PR Range-check intEnum and float wire values; validate intEnum membership (#109) #197)
• [medium] double→float narrowing in generated deserializers is UB on out-of-range input (ES.46) — generator-systematic. The FLOAT case (SerdeCodeGen.java:225-232, e.g. simplerestjson serde.cc:167) casts the parsed double to float unchecked; a request body containing 1e300 for a float member is undefined behavior per [conv.double] — UBSan's float-cast-overflow aborts, and this repo runs UBSan in CI. Reject finite overflow mirroring the int checks. (PR Range-check intEnum and float wire values; validate intEnum membership (#109) #197)
• [medium] ParseStatusLine uses std::atoi on untrusted bytes (ES.103). http1.cc:114 — UB on unrepresentable values (HTTP/1.1 99999999999999999999), and accepts +2/whitespace/0200 laxities the same file's Content-Length path deliberately forbids ten lines up. Use the existing digits-only + end-pointer recipe or std::from_chars.
• [low] JSON-RPC error code truncated before its range check (ES.46) — generator-systematic. JsonRpc2Protocol.java:97-100 emits static_cast<int>(code->as_int()) then tests >= 100 && < 600 (jsonrpc client.cc:47), so 2^32+404 classifies as 404. Range-check on int64 first. (PR Range-check intEnum and float wire values; validate intEnum membership (#109) #197)

B. HTTP transport robustness (beyond the #45 list)

• [high] No outbound CR/LF defense on any header write path (I.5) — extends the socket-transport item in Tracking: runtime robustness & numeric/security hardening #45. Beast serialization (beast_transport.cc:67-69, :898-899 — Beast's fields::insert does not validate) and Headers::Set/Add (headers.cc:75-82) accept raw CR/LF in names and values, so any handler/middleware echoing untrusted data into a header (a Location, a request-id echo) is full response splitting. The transports already strip framing headers — enforce control-byte rejection at the same authority point.
• [medium] Inbound parser stores field lines RFC 9112 requires rejecting (I.5). http1.cc:37-54: values keep embedded bare \r/\n, and Content-Length : 10 (space before colon) is stored as a different header name instead of rejected — a known smuggling primitive that sails past the framing checks at :60-65.
• [medium] Leftover buffered bytes become a phantom body (I.7). http1.cc:67 runs message.body = buffer.substr(header_end + 4) unconditionally; with no Content-Length and body_until_eof == false, trailing bytes in the same read are handed to the handler as a body, though http1.h:34-36 documents the message as ending with the headers.
• [medium] Write-timeout on a reused connection triggers a silent duplicate execution (I.7). The read path carefully excludes beast::error::timeout from stale ("a redial would only repeat them", beast_transport.cc:947), but the write path marks every write error stale (:924-928), and Send() then re-sends once on a fresh connection (:1006-1014) — a request already handed to the kernel whose write completion stalls executes twice, invisible to the retry layer's idempotency reasoning.
• [medium] Client write paths don't own the framing headers they document as transport authority (I.5). The socket client emits its own host/content-length/connection then appends all caller headers verbatim (socket_transport.cc:92-105) — a caller-set transfer-encoding rides beside the transport's content-length, the pair the server side's own comment calls "the classic smuggling pair" and strips (:197-199); the Beast client duplicates a caller-set host (beast_transport.cc:896-899). Strip/own the same set client-side.
• [medium] DNS resolution and connect are outside the documented per-request timeout (I.7). beast_transport.cc:831-836 resolves synchronously with no deadline (and each phase re-arms a full timeout, so worst case is several multiples of request_timeout_ms); the socket client's SO_RCVTIMEO/SO_SNDTIMEO never bound connect() (socket_transport.cc:74-86). A hung resolver blocks Send() for minutes regardless of configuration.
• [low] Double Start() on SocketHttpServer is an undiagnosed std::terminate (CP.23, CP.26). No already-started guard (socket_transport.cc:127-156); assigning to a joinable std::thread terminates — the sibling BeastServerTransport::Start returns Error::Validation("already started"), showing the intended contract. Also an fd leak and a handler_ race on the way down.
• [low] Socket server teardown can close the listener under a live accept thread (CP.23/24). In socket_transport.cc:218-237, a throw before join() (the nudge-client constructor allocates) is swallowed by catch (...) and control falls through to close(listener_) with the accept thread still running — fd-reuse race; close() also isn't guaranteed to wake a blocked accept(). Close only after a successful join; consider shutdown() over the loopback-connect nudge. Test-only transport, hence low.
• [low] MakeErrorResponse splices strings into JSON on a comment-only invariant (I.5). router.cc:14-17 — the header advertises it for generated-server framework failures, and ValidationFailure::message can embed request-derived text; one future call site forwarding user-influenced text is body injection. Escape the two fields or assert the invariant.
• [low] Guard/RequireApiKeyHeader/RequireBearerAuth skip the composition-time null-callback checks their siblings perform (I.5, I.8). middleware.h:33 says "neither callback may be null"; Observe/HealthEndpoint/PerClientRateLimit throw at composition, but these accept nulls and 500 on every request via std::bad_function_call (middleware.cc:24-34, :219-235) — the exact permanent-outage mode the other checks prevent.
• [low] Ports and status codes narrow unvalidated (ES.46). socket_transport.cc:138 / beast_transport.cc:613 truncate an out-of-range configured port and silently bind elsewhere; beast_transport.cc:65 feeds a handler-returned negative status to Beast as a huge unsigned.

C. Exception-safety posture vs ADR-0003 (E.*)

• [medium] Exceptions can cross the Outcome boundary and kill io threads (E.25, E.27). ADR-0003 says exceptions never cross the generated API and the runtime must build -fno-exceptions, but: io-thread bodies run unguarded (beast_transport.cc:640 — a bad_alloc in request conversion or a system_error from asio/SSL setup escapes io.run() and terminates the process, dropping all in-flight requests), and BeastHttpClient::Send/BeastServerTransport::Start can propagate system_error/bad_alloc to callers whose contract says Outcome-only. Meanwhile middleware/forwarded composition deliberately throws std::invalid_argument (middleware.cc:56,106,111,152, forwarded.cc:170,177,191) — fine per se, but incompatible with the ADR's -fno-exceptions buildability claim. Either contain at the boundaries or write the ADR amendment that reconciles the posture.
• [medium] Interceptor hooks documented "must not throw" but not noexcept (E.12). interceptor.h:20,28 state the contract in prose; retry.cc:48,52 calls them bare, so in today's exceptions-enabled build a throwing user hook propagates straight out of the generated client operation — exactly what ADR-0003 forbids. noexcept on the virtuals converts it to a diagnosable terminate at the offending hook, the ADR-0009-consistent posture.
• [low] z_stream teardown is manual on five early-return paths (R.1). gzip.cc calls deflateEnd/inflateEnd at five sites; every future early return (or a throwing out.append while exceptions remain enabled) is a latent ~256 KB leak. A local RAII wrapper collapses them to zero.

D. Interface and type-safety debt (I./C./T.)

• [medium] Document(bool) swallows every non-char pointer (C.46). With all constructors deliberately implicit (document.h:49,53), Document(&value) — a forgotten dereference — compiles and serializes true. Document(const void*) = delete; keeps const char* and bool working while making the trap a compile error.
• [low] Outcome states no requirements on T/E (T.10). outcome.h:42-46: Outcome<T, T> dies deep in the class body with duplicate constructors, and a T convertible from E silently picks the value side. A static_assert/requires names the contract at the instantiation point.
• [low] PageIterator template is unconstrained (T.10). pagination.h:25-26 — a slightly-wrong Next() signature on a hand-rolled paginator produces template spew instead of a one-line concept failure; C++20 is the floor, so a small PaginatorFor concept is free.
• [low] Weakly-typed quantities in ClientConfig (I.4). config.h:26 int request_timeout_ms and :49 signed int request_min_compression_size_bytes, in a struct whose own RetryPolicy uses std::chrono::milliseconds and whose pool knob uses size_t. -1 compiles silently. Breaking-change-sized, but the struct is young.
• [low] Null user-supplied hooks dereferenced unchecked, unlike their siblings (I.12). A nullptr in ClientConfig::interceptors is a null deref (retry.cc:48,52); ObserveAttempts(nullptr) is std::bad_function_call on the first request (observability.cc:25) — while RetryPolicy::sleep/jitter and TracePropagator in the same files are null-tolerant. Skip nulls or fail fast at registration.
• [low] Error factories take adjacent same-type code, message strings (I.24). error.h:27,39 — Modeled("Order not found", "OrderNotFound") compiles and corrupts retry/ErrorsAs matching. Only worth a strong typedef if it ever bites.

E. Smaller cleanups

• Compile-time tables (Con.2/Con.5): the CRC32 table (frame.cc:29-39) and base64 decode table (base64.cc:13-20) are runtime magic statics; both are constexpr-able in C++20, removing the init guard from the frame-decode and blob paths.
• ZLIB_CONST instead of const_cast (ES.50): gzip.cc:20,45 cast away const to satisfy next_in; zlib provides ZLIB_CONST for exactly this.
• CBOR decoder: std::span window and std::bit_cast (Bounds.1): cbor.cc:120 carries pointer+length by hand; :66-69/:316-323 memcpy-pun floats where bit_cast is typed and constexpr.
• Regex: 32-bit position stamps and compiler linkage (ES.46, SF.22): regex.cc:649,659 truncate positions to uint32_t (wrong-answer dedup past 4 GiB inputs — theoretical, one-type-change fix); RegexCompiler at regex.cc:461 has external linkage only to satisfy a friend declaration — an ODR hazard better spelled smithy::internal.
• string_view::data() into %s (SL.str): timestamp.cc:279-281 relies on literal-backed views being NUL-terminated; %.3s or const char* tables make it refactor-proof.
• Generated std::move on scalars (ES.56) — generator-systematic: one uniform move pattern moves bool/int/enum members (e.g. rest types.h:126); exactly what performance-move-const-arg flags once the lint gap in §F closes.
• Beast server copies the full response per request (F.15): Respond takes const HttpResponse& (beast_transport.cc:532-536) but both call sites pass rvalues — up-to-64 MiB body copy on the hot path; take by value and move.
• random_document.h relies on transitive includes (SF.10): random_document.h:60,71 use std::numeric_limits/std::vector without <limits>/<vector>; breaks on the libc++ matrix cell. Add the two includes. (PR Range-check intEnum and float wire values; validate intEnum membership (#109) #197)

F. Enforcement posture (P.5 — "prefer compile-time checking") — from #110

• Generated code sits outside every net the project built. CI clang-tidy excludes */generated/* (.github/workflows/ci.yml:241), HeaderFilterRegex: "runtime/include/.*" (.clang-tidy:38) skips generated headers, and generated BUILD.bazel emit no copts while //runtime gets -Wall -Wextra. The code every consumer inherits is the least-checked in the repo — the existing nets would already flag the two narrowing findings in §A and the dead store response.status = 201; immediately overwritten at examples/simplerestjson/generated/src/server.cc:159-160 (emitted unconditionally by HttpJsonServerGenerator.java). (Extends Enforce warning-clean compilation of generated C++ (the docs claim it; nothing checks it) #65.) Fix: run the existing clang-tidy config over one golden generated expansion in CI; have BuildFileGenerator emit the runtime COPTS.
• Warning-flag gaps for hand-written code. No -Wconversion anywhere (implicit-narrowing enforcement rests entirely on clang-tidy), no compiler -Werror (only clang-tidy/clang-format are warnings-as-errors), and fuzz/benchmark/example targets build at toolchain-default warning level. Note: bugprone-* already activates bugprone-narrowing-conversions (the cppcoreguidelines-narrowing-conversions alias) for runtime code, so the highest-value check is effectively on. Fix: add -Wconversion (or -Wfloat-conversion -Wshorten-64-to-32) + CI -Werror scoped off third_party/.
• cppcoreguidelines-* clang-tidy group is off; a few checks would add signal. The low-noise, high-value additions: cppcoreguidelines-slicing, -virtual-class-destructor, -special-member-functions, -missing-std-forward, -rvalue-reference-param-not-moved. The pro-bounds-*/owning-memory family would be pure noise given the project (reasonably) doesn't use GSL. Fix: cherry-pick those five rather than enabling the group.

What already conforms well (for fairness) — from #110

• R.1/R.11/R.3 — No naked new/delete anywhere; ownership is uniformly unique_ptr (regex tree, Boxed), shared_ptr (interceptors, transports), or values.
• C.20/C.21/C.35 — Rule of zero throughout the value types; Boxed is a textbook rule-of-five deep-copy wrapper with noexcept moves; interface classes have virtual destructors; Session deletes copies.
• CP.2/CP.32/R.34-36/F.53 — The Beast server's async discipline is exemplary: every completion captures State weakly, sessions own streams via aliasing shared_ptr (fds close exactly when the last handler releases), the connection pool holds its mutex only around vector ops and never under user code/IO, and RNG is thread_local.
• I.5/ES.103 (framing) — Duplicate Content-Length and any Transfer-Encoding rejected outright, Content-Length parsed digits-only with overflow handling, and servers strip handler-set framing headers — genuinely smuggling-aware where it counts.
• Enum.3 — All enums are scoped, with explicit underlying types where layout matters.
• ES.46 (modeled ints) — byte/short/integer members are range-rejected before every narrowing cast, and label/query/header numerics go through strict from_chars parsers — the intEnum and float gaps in §A are the only two holes in an otherwise disciplined serde posture.
• Tooling / P.5 (positive) — ASan/UBSan/libFuzzer configs with genuinely property-checking fuzz harnesses (byte-exact re-encode, prefix-monotonicity, trust-walk biconditionals) that also run as deterministic CI tests — a materially stronger fuzz posture than most projects this size.

Suggested triage — from #110, updated for progress

1. Land the three High items first — done: the gzip truncation, the Timestamp::Format(kEpochSeconds) sign bug, and the outbound-CRLF defense have all landed (checked above).
2. The remaining serde holes — intEnum truncation and double→float UB (§A) — are the highest-impact open items: attacker-reachable UB and silent aliasing in every generated deserializer, plus the JSON-RPC error-code truncation from the same narrowing family. (PR Range-check intEnum and float wire values; validate intEnum membership (#109) #197 addresses all of these.)
3. Close the generated-code enforcement gap (§F) early — the cheapest high-leverage move; it would have caught both narrowing findings and the dead store automatically. Coordinate with Enforce warning-clean compilation of generated C++ (the docs claim it; nothing checks it) #65.
4. The remaining "inconsistent posture" mediums (strtod, ParseDateTime, write-timeout retry) are each a localized fix that makes an already-deliberate strategy uniform.
5. Fold the (→ Tracking: runtime robustness & numeric/security hardening #45) items into Tracking: runtime robustness & numeric/security hardening #45 rather than duplicating; this issue keeps them only for the guidelines cross-reference.