Skip to content

Range-check intEnum and float wire values; validate intEnum membership (#109) - #197

Merged
aaylward merged 10 commits into
mainfrom
claude/smithy-cpp-dedup-issues-hy5key
Aug 25, 2026
Merged

Range-check intEnum and float wire values; validate intEnum membership (#109)#197
aaylward merged 10 commits into
mainfrom
claude/smithy-cpp-dedup-issues-hy5key

Conversation

@aaylward

Copy link
Copy Markdown
Collaborator

What

Closes the two highest-impact open items on the #109 C++ Core Guidelines tracker (the remaining serde narrowing holes, per the tracker's triage), plus the related smaller findings from the same narrowing family. #110 was consolidated into #109 and closed as its duplicate before this PR.

  • intEnum wire values were cast straight into the int32-backed enum class — a wire int64 of 2^32+2 silently aliased onto a valid enumerator. intEnum now merges into the bounded-integer serde case and shares byte/short/integer's "value out of range" rejection, on document bodies and text bindings alike (SerdeCodeGen.java).
  • float members cast the parsed double unchecked — a finite wire value beyond float range (1e300) was undefined behavior per [conv.double], which UBSan's float-cast-overflow aborts on. Both the document-body path (SerdeCodeGen.java) and the query/label/header text path (HttpBindingCodeGen.java) now narrow through a new runtime helper, smithy::FloatFromDouble, which rejects finite overflow while NaN/±Infinity still pass (they're legal Smithy float values and narrow losslessly).
  • Servers now validate intEnum membership (ValidationGenerator.java), matching string enums: same suite-exact ValidationException message with the value set spelled in ints (the smithy-rs convention), same @internal policy (wire-valid but unadvertised). Clients deliberately keep unknown-but-in-range values for forward compatibility — the asymmetry is pinned by tests. The generated smoke/response suites immediately caught that a default-constructed intEnum (0) usually isn't a member, so minimal test values now pick the first modeled member, as string enums always did (NodeLiteralGenerator.java).
  • Folded-in minor findings from the same family (Tracking: C++ Core Guidelines conformance review #109 §A/§E): the jsonRpc2 client truncated error.code to int before its 100–599 range test, so 2^32+404 classified as HTTP 404 and 5xx aliases came back retryable — codes are now classified on the full int64 (JsonRpc2Protocol.java); and random_document.h gets the <limits>/<vector> includes it was borrowing transitively (SF.10).

Checked-in goldens are regenerated; regeneration is byte-stable. CHANGELOG, docs/server-guide.md, and docs/generated-types.md updated for the behavior changes.

Testing

Beyoncé Rule throughout — everything touched now has something pinning it:

  • Runtime unit: FloatFromDouble in document_serde_test.cc — in-range, exact FLT_MAX edge, finite overflow rejection (including one double-ULP past the edge), NaN/±Inf pass-through, subnormal rounding.
  • Codegen unit (Java): SerdeGeneratorTest pins the intEnum range check preceding the cast and the float narrowing (double stays unchecked); HttpJsonBindingProtocolTest pins the text-binding float path (no fixture binds a float to a text position, so goldens can't); ValidationGeneratorTest pins intEnum membership incl. the @internal policy.
  • Integration, across module boundaries: new examples/roundtrip/rest/numeric_bounds_wire_test.cc drives the roundtrip fixture at three depths — generated serde directly with hostile Documents, generated server over the wire (SerializationException before the handler; ValidationException with the suite-exact message and fieldList path), and generated client parsing hostile responses — plus the client/server asymmetry pin. examples/jsonrpc2/interop_wire_test.cc adds a hand-rolled peer sending error code 21474837103 (must not classify as a retryable 503). The hand-written malformed suite (protocol-tests/simplerestjson/malformed) pins the alloy fixture's intEnum label: out-of-int32 → SerializationException, in-range-unknown → ValidationException.
  • Full matrix locally: bazel test //... --config=werror — 126/126 pass; (cd codegen && gradle build spotlessCheck) green; golden regeneration byte-identical; UBSan pass (gcc, -fsanitize=undefined -fno-sanitize-recover=all) green over the affected suites (this environment's clang lacks the sanitizer runtime libs, so the clang asan/ubsan cells are left to CI).

Checklist

  • Tests added/updated for the change
  • bazel test //... and (cd codegen && gradle build spotlessCheck) pass locally
  • Formatting clean (clang-format, buildifier, spotless)
  • Architectural decisions recorded as an ADR (not applicable — makes the existing ADR-0009 range-check posture uniform)

🤖 Generated with Claude Code

https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU


Generated by Claude Code

claude added 8 commits August 25, 2026 00:35
A finite double beyond float range is UB to static_cast ([conv.double]);
generated deserializers need the check in one tested place. NaN and
±Infinity pass through — they are legal Smithy float values on every wire.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
intEnum merges into the bounded-integer serde case: its underlying type is
int32, so a wire int64 of 2^32+2 silently aliased onto a valid enumerator.
float rejects finite doubles beyond float range via smithy::FloatFromDouble
on both the document-body path and the query/label/header text path — the
raw cast was UB that UBSan's float-cast-overflow aborts on. Unknown
in-range intEnum values still parse, matching string enums' tolerant reads.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
String enums already fail request validation outside the modeled value set;
intEnum members were accepted silently. Same suite-exact ValidationException
message with the value set spelled in ints (the smithy-rs convention), same
@internal policy: wire-valid but unadvertised. The generated smoke and
response suites promptly caught that a default-constructed intEnum (0) is
not usually a member — minimal test values now pick the first modeled
member, exactly as string enums always did.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
ParseError truncated error.code to int before its 100-599 range test, so a
peer sending 2^32+404 classified as HTTP 404 — and 5xx aliases came back
retryable. The interop suite now pins a hand-rolled peer sending
21474837103: not a 503, lands in the 400 class, not retryable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
)

One suite driving the roundtrip REST fixture at three depths: the generated
serde directly (hostile Documents), the generated server over the wire
(SerializationException before the handler, ValidationException with the
suite-exact message), and the generated client parsing hostile responses —
plus the deliberate asymmetry: clients keep unknown-but-in-range intEnum
values for forward compatibility, servers reject them.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
std::numeric_limits and std::vector without <limits>/<vector> — breaks on
the libc++ matrix cell (SF.10).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
FloatFromDouble rejected at FLT_MAX, but [conv.double] only goes undefined
once round-to-nearest overflows — at 2^128*(1-2^-25). The gap is not
theoretical: shortest-round-trip float text, including FormatFloat's own
output for FLT_MAX, parses to a double slightly above FLT_MAX that must
keep narrowing to it. The boundary is now the true overflow bound, pinned
by a FormatFloat->strtod->FloatFromDouble round-trip test and a wire-text
serde case.

Also from the panel: the jsonRpc2 truncation test used 21474837103, which
truncates to 623 and classified as 400 under old and new code alike — a
vacuous pin; 21474836983 truncates to 503 and discriminates. Generated
jsonRpc2 clients now include <cstdint> for their std::int64_t local instead
of borrowing it transitively (SF.10). The Describe test helper fails
cleanly instead of dereferencing a failed Outcome.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU

Copy link
Copy Markdown
Collaborator Author

CI status on 832296c: the bazel (asan + ubsan, ubuntu-24.04) job is red, but the failure is unrelated to this PR — its TSan sweep caught a data race in //runtime:session_registry_test:

SUMMARY: ThreadSanitizer: data race runtime/include/smithy/server/session_registry.h:303:25
  in SessionRegistry<...>::Resume(...)   (racing with WriterLoop, session_registry.h:540:38)

during SessionRegistryGraceTest.DetachResumeChurnUnderBroadcastAndCloseStaysSafe (the test's assertions pass; TSan's one warning fails the run). Why it isn't this PR's failure:

  • The diff touches serde/codegen/tests only — session_registry.h and its test are byte-identical to main on this branch, so the race exists on main too.
  • The identical job was green on this PR's previous head 375abd5; the churn test races only intermittently (it didn't reproduce in local TSan runs either).

I don't have permission to re-run the failed job (403 on rerun-failed-jobs) — a maintainer re-run should clear it. The race itself looks worth a look independent of this PR; I've noted it on the #109 tracker.


Generated by Claude Code

Copy link
Copy Markdown
Collaborator Author

Resolved — no maintainer re-run needed: updating the branch onto current main (the spotless 8.10.0 bump) retriggered CI, and the full matrix is green on 3a803cb, TSan sweep included. That confirms the earlier session_registry race is an intermittent pre-existing flake, tracked on #109. The PR is now green and mergeable, awaiting review.


Generated by Claude Code

@cursor

cursor Bot commented Aug 25, 2026

Copy link
Copy Markdown

Review

LGTM — this closes the real #109 narrowing holes cleanly, and the prior panel pass already fixed the sharp edges (true float overflow bound vs FLT_MAX, discriminatory jsonRpc2 code, Describe helper). CI is green across the matrix.

What looks right

  • FloatFromDouble rejects at 0x1.ffffffp+127 (IEEE midpoint / ties-to-even → out of range), not FLT_MAX. The FormatFloatstrtod → narrow round-trip pin is the important one; without it this library’s own float text would 400.
  • intEnum → int32 bounds on the document path (text path was already covered via ParseInt64Text + int64Bounds) stops the 2^32+2 → 2 alias; membership validation mirrors string-enum @internal policy; client/server asymmetry is explicitly pinned.
  • jsonRpc2 classifying on full int64 before the 100–599 window, with 21474836983 (truncates to 503) rather than a vacuous constant.
  • Test layering matches the claim: runtime unit, codegen unit, malformed suite, and cross-module wire pins.

Nits (non-blocking)

  1. docs/model-evolution.md — the “Add enum value” row still only describes string enums (kUnknown + original text). Worth a half-sentence that intEnum clients keep unknown in-range values while servers reject via ValidationException, so it stays aligned with generated-types.md / this PR’s posture.
  2. CHANGELOG wording — “on document bodies and text bindings alike” for intEnum describes the end state; on main the text path was already bounded. Harmless, but a reader bisecting the hole will look at SerdeCodeGen first.

No correctness issues found; happy to see this land.

Comment thread CHANGELOG.md Outdated
(#109). Three holes in the otherwise-uniform range-check posture: an
`intEnum` member cast the raw wire int64 straight into its `int32`-backed
`enum class`, so 2^32+2 silently aliased onto a valid enumerator (byte /
short / integer members already rejected out-of-range values — intEnum now

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit (non-blocking): “text bindings alike” describes the end state — on main the intEnum text path was already bounded via ParseInt64Text + int64Bounds(INT_ENUM). The hole this PR closes for intEnum is the document-body cast in SerdeCodeGen (plus server membership). Harmless, but a reader bisecting will look here first expecting a text-path change too.

Review-bot nits on #197, both verified: the intEnum text-binding path was
already bounded on main (ParseInt64Text + int64Bounds) — the hole this PR
closes is the document-body cast, so the changelog now says which path
changed; and model-evolution.md's add-enum-value row now covers intEnums
(clients keep unknown in-range values, servers reject outside the modeled
set) alongside string enums.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014C7WdBD99mUFWGxMvGSjSU
@aaylward
aaylward enabled auto-merge August 25, 2026 21:01
@aaylward
aaylward merged commit 9f84c9a into main Aug 25, 2026
16 checks passed
@aaylward
aaylward deleted the claude/smithy-cpp-dedup-issues-hy5key branch August 25, 2026 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants