From 662f79bdd875dec13b4e7897dafe679631d23ed5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 04:01:39 +0000 Subject: [PATCH 1/3] Contain handler/middleware exceptions instead of terminating the server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A handler (or Observe callback) that threw propagated out of the transport's I/O thread and called std::terminate, taking down every in-flight request on all threads — the interface only documented "handlers must not throw" (issue #41). - Add smithy::http::InvokeHandlerGuarded (smithy/http/server_dispatch.h): invokes a RequestHandler, converting any escaped exception into a 500 carrying a generated x-correlation-id header (and a minimal JSON body repeating it), with the id + what() written to std::clog so an otherwise-silent crash leaves one greppable server-side line. An empty handler yields 503. - Route the Beast, socket, and loopback transports through it so the guard holds regardless of which handler a consumer installs. - Guard the Observe middleware callback locally so a throwing metrics/log sink neither discards the built response nor unwinds the transport thread. - Tests: unit coverage of the guard (success passthrough, std/non-std exceptions -> 500 + distinct correlation ids, empty -> 503), a real end-to-end socket test proving a throwing handler yields 500 and the server survives the next request, and a middleware test for the throwing-callback case. - Docs: transport.h contract and server-guide describe the safety net. Refs #41 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SyQAo21Pv6GYhHrkbQj8xQ --- docs/server-guide.md | 6 ++ runtime/BUILD.bazel | 7 +- runtime/include/smithy/http/loopback.h | 3 +- runtime/include/smithy/http/server_dispatch.h | 25 +++++++ runtime/include/smithy/http/transport.h | 6 +- runtime/src/http/beast_transport.cc | 10 +-- runtime/src/http/server_dispatch.cc | 42 +++++++++++ runtime/src/http/socket_transport.cc | 4 +- runtime/src/server/middleware.cc | 13 +++- runtime/tests/http/server_dispatch_test.cc | 71 +++++++++++++++++++ runtime/tests/http/socket_transport_test.cc | 34 +++++++++ runtime/tests/server/middleware_test.cc | 12 ++++ 12 files changed, 223 insertions(+), 10 deletions(-) create mode 100644 runtime/include/smithy/http/server_dispatch.h create mode 100644 runtime/src/http/server_dispatch.cc create mode 100644 runtime/tests/http/server_dispatch_test.cc diff --git a/docs/server-guide.md b/docs/server-guide.md index e7d5f008..01fbab99 100644 --- a/docs/server-guide.md +++ b/docs/server-guide.md @@ -34,6 +34,12 @@ class MyHandler final : public example::weather::WeatherHandler { - **Validation/serialization errors** (including malformed request input the framework catches before your handler runs) map to 400; any other failure is a non-leaking 500 `InternalFailure`. +- **A handler that throws** (rather than returning an `Error`) is contained by the transport + layer: the exception is converted into a 500 carrying an `x-correlation-id` header, and the + same id plus the exception's `what()` is written to `std::clog`. One throwing request fails + alone — it never unwinds into the transport's I/O thread and terminates the process. Prefer + returning `smithy::Error` for expected failures; the catch-all is a safety net, not a control + path. ## Running a server diff --git a/runtime/BUILD.bazel b/runtime/BUILD.bazel index bb0d30b4..c0fa37aa 100644 --- a/runtime/BUILD.bazel +++ b/runtime/BUILD.bazel @@ -64,6 +64,7 @@ cc_library( name = "http", srcs = [ "src/http/headers.cc", + "src/http/server_dispatch.cc", "src/http/socket_transport.cc", "src/http/trace_context.cc", "src/http/uri.cc", @@ -72,6 +73,7 @@ cc_library( "include/smithy/http/headers.h", "include/smithy/http/loopback.h", "include/smithy/http/message.h", + "include/smithy/http/server_dispatch.h", "include/smithy/http/socket_transport.h", "include/smithy/http/trace_context.h", "include/smithy/http/transport.h", @@ -127,7 +129,10 @@ cc_test( cc_test( name = "http_test", size = "small", - srcs = ["tests/http/http_test.cc"], + srcs = [ + "tests/http/http_test.cc", + "tests/http/server_dispatch_test.cc", + ], copts = COPTS, deps = [ ":http", diff --git a/runtime/include/smithy/http/loopback.h b/runtime/include/smithy/http/loopback.h index af648aa7..c091fa3d 100644 --- a/runtime/include/smithy/http/loopback.h +++ b/runtime/include/smithy/http/loopback.h @@ -3,6 +3,7 @@ #include +#include "smithy/http/server_dispatch.h" #include "smithy/http/transport.h" namespace smithy::http { @@ -23,7 +24,7 @@ class Loopback : public HttpClient, public HttpServerTransport { // HttpClient: Outcome Send(const HttpRequest& request) override { if (!handler_) return Error::Transport("loopback: no handler installed", /*retryable=*/false); - return handler_(request); + return InvokeHandlerGuarded(handler_, request); } private: diff --git a/runtime/include/smithy/http/server_dispatch.h b/runtime/include/smithy/http/server_dispatch.h new file mode 100644 index 00000000..99d2ce5c --- /dev/null +++ b/runtime/include/smithy/http/server_dispatch.h @@ -0,0 +1,25 @@ +#ifndef SMITHY_HTTP_SERVER_DISPATCH_H_ +#define SMITHY_HTTP_SERVER_DISPATCH_H_ + +#include "smithy/http/message.h" +#include "smithy/http/transport.h" + +namespace smithy::http { + +// Invokes handler(request), converting any exception that escapes the handler +// into a 500 response instead of letting it propagate. Server transports call +// this rather than invoking the handler directly, so a throwing handler fails +// exactly one request instead of unwinding out of the transport's I/O thread +// and terminating the whole process. +// +// The synthesized 500 carries a generated correlation id in the +// "x-correlation-id" header (and a minimal JSON body repeating it); the same id +// plus the exception's what() is written to std::clog, so an otherwise-silent +// crash leaves one greppable line tying the client-visible failure to a +// server-side cause. handler may be empty — that yields a 503 (no correlation +// id: nothing ran). +HttpResponse InvokeHandlerGuarded(const RequestHandler& handler, const HttpRequest& request); + +} // namespace smithy::http + +#endif // SMITHY_HTTP_SERVER_DISPATCH_H_ diff --git a/runtime/include/smithy/http/transport.h b/runtime/include/smithy/http/transport.h index 8ee97959..c543ba79 100644 --- a/runtime/include/smithy/http/transport.h +++ b/runtime/include/smithy/http/transport.h @@ -24,8 +24,10 @@ class HttpClient { } }; -// What a server transport calls for each incoming request. Handlers must not -// throw; failures are expressed as HTTP responses. +// What a server transport calls for each incoming request. Handlers express +// failures as HTTP responses; a handler that nonetheless throws is contained +// by the transport (see smithy/http/server_dispatch.h) as a 500 with a +// correlation id rather than taking down the process. using RequestHandler = std::function; // Server-side transport: binds a listener and dispatches requests to a diff --git a/runtime/src/http/beast_transport.cc b/runtime/src/http/beast_transport.cc index 04ce04fd..96730f58 100644 --- a/runtime/src/http/beast_transport.cc +++ b/runtime/src/http/beast_transport.cc @@ -19,6 +19,7 @@ #include #include +#include "smithy/http/server_dispatch.h" #include "smithy/http/uri.h" namespace smithy::http { @@ -137,10 +138,11 @@ struct BeastServerTransport::State : std::enable_shared_from_this { self->active.fetch_add(1); const bool keep_alive = parser->get().keep_alive() && !self->stopping; const HttpRequest request = ToSmithyRequest(parser->get()); - // Handlers are synchronous for now (ADR-0003 keeps them exception-free); - // they run on the pool thread that completed the read. - const HttpResponse response = - self->handler ? self->handler(request) : HttpResponse{503, {}, ""}; + // Handlers run synchronously on the pool thread that completed the + // read. InvokeHandlerGuarded contains any exception the handler + // throws as a 500 — otherwise it would unwind out of io_context::run + // and terminate the process, dropping every in-flight request. + const HttpResponse response = InvokeHandlerGuarded(self->handler, request); auto wire = std::make_shared>( ToWireResponse(response, keep_alive)); auto& wire_stream = *stream; diff --git a/runtime/src/http/server_dispatch.cc b/runtime/src/http/server_dispatch.cc new file mode 100644 index 00000000..30c1de3d --- /dev/null +++ b/runtime/src/http/server_dispatch.cc @@ -0,0 +1,42 @@ +#include "smithy/http/server_dispatch.h" + +#include +#include +#include + +#include "smithy/core/uuid.h" + +namespace smithy::http { +namespace { + +HttpResponse InternalError(const HttpRequest& request, const std::string& what) { + const std::string correlation_id = GenerateUuidV4(); + // The built-in default sink. A structured-logging seam can replace this, but + // an unhandled handler exception must never be silent — this line is often + // the only server-side trace of a 500. + std::clog << "smithy: handler threw; correlation-id=" << correlation_id << " request=\"" + << request.method << ' ' << request.target << "\" what=\"" << what << "\"\n"; + HttpResponse response; + response.status = 500; + response.headers.Set("content-type", "application/json"); + response.headers.Set("x-correlation-id", correlation_id); + response.body = "{\"message\":\"internal error\",\"correlationId\":\"" + correlation_id + "\"}"; + return response; +} + +} // namespace + +HttpResponse InvokeHandlerGuarded(const RequestHandler& handler, const HttpRequest& request) { + if (!handler) { + return HttpResponse{503, {}, "", ""}; + } + try { + return handler(request); + } catch (const std::exception& e) { + return InternalError(request, e.what()); + } catch (...) { + return InternalError(request, "unknown exception"); + } +} + +} // namespace smithy::http diff --git a/runtime/src/http/socket_transport.cc b/runtime/src/http/socket_transport.cc index e02aab27..c6b87cba 100644 --- a/runtime/src/http/socket_transport.cc +++ b/runtime/src/http/socket_transport.cc @@ -19,6 +19,8 @@ #include #endif +#include "smithy/http/server_dispatch.h" + namespace smithy::http { namespace { @@ -297,7 +299,7 @@ void SocketHttpServer::AcceptLoop() { request.target = line.substr(first + 1, second - first - 1); request.headers = std::move(message->headers); request.body = std::move(message->body); - response = handler_(request); + response = InvokeHandlerGuarded(handler_, request); } } else { response = HttpResponse{400, {}, message.error().message()}; diff --git a/runtime/src/server/middleware.cc b/runtime/src/server/middleware.cc index 6c8fc8d8..ce5a5e26 100644 --- a/runtime/src/server/middleware.cc +++ b/runtime/src/server/middleware.cc @@ -2,6 +2,8 @@ #include #include +#include +#include #include #include #include @@ -33,7 +35,16 @@ Middleware Observe(std::function callback, observation.trace_parent = request.headers.Get("traceparent").value_or(""); observation.status = response.status; observation.duration = std::chrono::duration_cast(now() - start); - callback(observation); + // A throwing observation sink (e.g. a metrics backend under backpressure) + // must not discard an already-built response or unwind into the + // transport thread; swallow it after logging. + try { + callback(observation); + } catch (const std::exception& e) { + std::clog << "smithy: Observe callback threw: " << e.what() << "\n"; + } catch (...) { + std::clog << "smithy: Observe callback threw a non-std exception\n"; + } return response; }; }; diff --git a/runtime/tests/http/server_dispatch_test.cc b/runtime/tests/http/server_dispatch_test.cc new file mode 100644 index 00000000..4cc0a7e2 --- /dev/null +++ b/runtime/tests/http/server_dispatch_test.cc @@ -0,0 +1,71 @@ +#include "smithy/http/server_dispatch.h" + +#include + +#include +#include + +#include "smithy/http/message.h" +#include "smithy/http/transport.h" + +namespace smithy::http { +namespace { + +HttpRequest SampleRequest() { + HttpRequest request; + request.method = "POST"; + request.target = "/tasks"; + return request; +} + +TEST(ServerDispatchTest, PassesThroughASuccessfulResponse) { + RequestHandler handler = [](const HttpRequest&) { + HttpResponse response; + response.status = 201; + response.body = "ok"; + return response; + }; + const HttpResponse response = InvokeHandlerGuarded(handler, SampleRequest()); + EXPECT_EQ(response.status, 201); + EXPECT_EQ(response.body, "ok"); + EXPECT_FALSE(response.headers.Get("x-correlation-id").has_value()); +} + +TEST(ServerDispatchTest, StdExceptionBecomesA500WithCorrelationId) { + RequestHandler handler = [](const HttpRequest&) -> HttpResponse { + throw std::out_of_range("boom"); + }; + const HttpResponse response = InvokeHandlerGuarded(handler, SampleRequest()); + EXPECT_EQ(response.status, 500); + const auto id = response.headers.Get("x-correlation-id"); + ASSERT_TRUE(id.has_value()); + EXPECT_FALSE(id->empty()); + // The body repeats the same id so a client report can be tied to the log line. + EXPECT_NE(response.body.find(*id), std::string::npos); + EXPECT_EQ(response.headers.Get("content-type").value_or(""), "application/json"); +} + +TEST(ServerDispatchTest, NonStdExceptionBecomesA500) { + RequestHandler handler = [](const HttpRequest&) -> HttpResponse { throw 42; }; + const HttpResponse response = InvokeHandlerGuarded(handler, SampleRequest()); + EXPECT_EQ(response.status, 500); + EXPECT_TRUE(response.headers.Get("x-correlation-id").has_value()); +} + +TEST(ServerDispatchTest, DistinctFailuresGetDistinctCorrelationIds) { + RequestHandler handler = [](const HttpRequest&) -> HttpResponse { + throw std::runtime_error("x"); + }; + const auto a = InvokeHandlerGuarded(handler, SampleRequest()); + const auto b = InvokeHandlerGuarded(handler, SampleRequest()); + EXPECT_NE(a.headers.Get("x-correlation-id"), b.headers.Get("x-correlation-id")); +} + +TEST(ServerDispatchTest, EmptyHandlerIsA503NotACrash) { + const HttpResponse response = InvokeHandlerGuarded(RequestHandler{}, SampleRequest()); + EXPECT_EQ(response.status, 503); + EXPECT_FALSE(response.headers.Get("x-correlation-id").has_value()); +} + +} // namespace +} // namespace smithy::http diff --git a/runtime/tests/http/socket_transport_test.cc b/runtime/tests/http/socket_transport_test.cc index 19ca7b40..7419f943 100644 --- a/runtime/tests/http/socket_transport_test.cc +++ b/runtime/tests/http/socket_transport_test.cc @@ -2,6 +2,7 @@ #include +#include #include namespace smithy::http { @@ -42,6 +43,39 @@ TEST(SocketTransportTest, RoundTripsOverRealSockets) { server.Stop(); } +TEST(SocketTransportTest, ThrowingHandlerBecomesA500NotACrash) { + // The exception escapes the handler on the transport's own thread; before the + // guard this unwound out of the accept loop and terminated the process. The + // server must instead answer 500 and stay up for the next request. + SocketHttpServer server; + ASSERT_TRUE(server + .Start([](const HttpRequest& request) -> HttpResponse { + if (request.target == "/boom") { + throw std::runtime_error("handler blew up"); + } + return HttpResponse{200, {}, "ok"}; + }) + .ok()); + SocketHttpClient client("127.0.0.1", server.port()); + + HttpRequest boom; + boom.target = "/boom"; + const auto failed = client.Send(boom); + ASSERT_TRUE(failed.ok()) << failed.error().message(); + EXPECT_EQ(failed->status, 500); + EXPECT_FALSE(failed->headers.Get("x-correlation-id").value_or("").empty()); + + // The server survived: a subsequent request still succeeds. + HttpRequest fine; + fine.target = "/ok"; + const auto ok = client.Send(fine); + ASSERT_TRUE(ok.ok()) << ok.error().message(); + EXPECT_EQ(ok->status, 200); + EXPECT_EQ(ok->body, "ok"); + + server.Stop(); +} + TEST(SocketTransportTest, HandlesSequentialRequestsAndLargeBodies) { SocketHttpServer server; ASSERT_TRUE( diff --git a/runtime/tests/server/middleware_test.cc b/runtime/tests/server/middleware_test.cc index 2ff76aec..9f086715 100644 --- a/runtime/tests/server/middleware_test.cc +++ b/runtime/tests/server/middleware_test.cc @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -152,5 +153,16 @@ TEST(ObserveTest, CountsEveryRequest) { EXPECT_EQ(count, 3); } +TEST(ObserveTest, ThrowingCallbackDoesNotDiscardResponseOrPropagate) { + auto handler = Chain({Observe([](const RequestObservation&) { + throw std::runtime_error("metrics backend down"); + })}, + [](const http::HttpRequest&) { return Ok("payload"); }); + http::HttpResponse response; + EXPECT_NO_THROW(response = handler({})); + EXPECT_EQ(response.status, 200); + EXPECT_EQ(response.body, "payload"); +} + } // namespace } // namespace smithy::server From 1b45a4d301129df07f3cf2d76d791a98f0e2ee49 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 04:02:52 +0000 Subject: [PATCH 2/3] Add generated-dispatch integration test for the handler-throw 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Complements the transport-level socket test with one that drives the generated WeatherServer over loopback: a handler subclass whose GetCity throws must be contained as a 500 (with an x-correlation-id header) through the full generated stack — routing, validation, then the handler — and the server must remain usable afterward. Refs #41 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SyQAo21Pv6GYhHrkbQj8xQ --- examples/weather/generated_server_e2e_test.cc | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/examples/weather/generated_server_e2e_test.cc b/examples/weather/generated_server_e2e_test.cc index 1dd62e2c..acbc9034 100644 --- a/examples/weather/generated_server_e2e_test.cc +++ b/examples/weather/generated_server_e2e_test.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -104,6 +105,42 @@ TEST_F(GeneratedServerEndToEndTest, GetCityRoundTrips) { EXPECT_FLOAT_EQ(city->coordinates.longitude, -122.3321F); } +// A handler that throws (rather than returning a modeled Error) must be +// contained as a 500 through the full generated dispatch stack — routing, +// validation, then the handler — not crash the server. This drives the +// generated WeatherServer over loopback and reads the raw response so it can +// assert the status and correlation-id header the client abstraction hides. +class ThrowingHandler : public ReferenceHandler { + public: + smithy::Outcome GetCity(const GetCityInput& input) override { + (void)input; + throw std::runtime_error("handler blew up mid-request"); + } +}; + +TEST(GeneratedServerFaultTest, ThrowingHandlerBecomesA500ThroughGeneratedDispatch) { + WeatherServer server(std::make_shared()); + smithy::http::Loopback loopback; + ASSERT_TRUE(loopback.Start(server.Handler()).ok()); + + smithy::http::HttpRequest request; + request.method = "GET"; + request.target = "/cities/seattle"; // valid route → reaches GetCity + const auto response = loopback.Send(request); + + ASSERT_TRUE(response.ok()) << response.error().message(); + EXPECT_EQ(response->status, 500); + EXPECT_FALSE(response->headers.Get("x-correlation-id").value_or("").empty()); + + // The server instance is still usable afterward: a well-behaved route works. + WeatherServer healthy(std::make_shared()); + smithy::http::Loopback ok_loopback; + ASSERT_TRUE(ok_loopback.Start(healthy.Handler()).ok()); + const auto ok = ok_loopback.Send(request); + ASSERT_TRUE(ok.ok()) << ok.error().message(); + EXPECT_EQ(ok->status, 200); +} + // A transport that fails transiently proves the generated client's retry // path end to end (Phase 7): the third attempt reaches the server. class FlakyTransport final : public smithy::http::HttpClient { From bac6506577621880426ba195436d73c124d150c3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 8 Jul 2026 04:10:14 +0000 Subject: [PATCH 3/3] Range-check timestamp conversions from untrusted numbers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Decoding a CBOR tag-1 timestamp did `as_int() * 1000`, which overflows int64 on a ~9-byte payload — undefined behavior on untrusted input (issue #42). The epoch-seconds paths (CBOR double, JSON number, and the epoch-seconds string parser) likewise cast an unbounded double through `llround(seconds * 1000.0)` into int64, and extreme instants formatted to 5+ digit / negative years that no conformant peer can parse back. - Add Timestamp::FromEpochSecondsChecked / FromEpochMillisecondsChecked: Outcome-returning factories that reject non-finite values and any instant whose civil year falls outside the RFC 3339 / IMF-fixdate representable window (0000-9999), bounding the input before the scale and cast so neither can overflow. - Route the three untrusted-input paths through them: CBOR tag-1 decode (both integer and double content), TimestampFromDocument (JSON/CBOR numbers), and the epoch-seconds string parser. The unchecked FromEpochSeconds/FromEpochMilliseconds remain for internal callers with known-good values. - Tests: checked-factory unit tests (in-range, exact 0000/9999 boundaries, out-of-range and non-finite rejection) and a CBOR regression decoding the overflow payloads (huge tag-1 int, huge negative int, near-DBL_MAX double) as clean errors rather than UB. Closes #42 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SyQAo21Pv6GYhHrkbQj8xQ --- runtime/include/smithy/core/timestamp.h | 8 ++++++ runtime/src/cbor/cbor.cc | 14 +++++++--- runtime/src/core/document_serde.cc | 2 +- runtime/src/core/timestamp.cc | 35 +++++++++++++++++++++++- runtime/tests/cbor/cbor_test.cc | 13 +++++++++ runtime/tests/core/timestamp_test.cc | 36 +++++++++++++++++++++++++ 6 files changed, 102 insertions(+), 6 deletions(-) diff --git a/runtime/include/smithy/core/timestamp.h b/runtime/include/smithy/core/timestamp.h index 1035af36..d21470d6 100644 --- a/runtime/include/smithy/core/timestamp.h +++ b/runtime/include/smithy/core/timestamp.h @@ -30,6 +30,14 @@ class Timestamp { // Rounds to the nearest millisecond. static Timestamp FromEpochSeconds(double seconds); + // Range-checked factories for untrusted wire input. Serde uses these so a + // hostile or malformed number cannot overflow the int64/double arithmetic + // (undefined behavior) or land on an instant that formats to unparsable + // text: they reject any value whose civil year falls outside the + // RFC 3339 / IMF-fixdate representable window (0000-9999). + static Outcome FromEpochSecondsChecked(double seconds); + static Outcome FromEpochMillisecondsChecked(std::int64_t ms); + std::int64_t epoch_milliseconds() const { return ms_; } double epoch_seconds() const { return static_cast(ms_) / 1000.0; } diff --git a/runtime/src/cbor/cbor.cc b/runtime/src/cbor/cbor.cc index 20b0ed9c..ca6f3871 100644 --- a/runtime/src/cbor/cbor.cc +++ b/runtime/src/cbor/cbor.cc @@ -281,13 +281,19 @@ class Decoder { auto inner = DecodeValue(depth - 1); if (!inner) return std::move(inner).error(); if (*tag == kTagEpochTimestamp) { + // Tag 1 content is epoch seconds (integer or float). Route both + // through the range-checked factory: a large integer would overflow + // int64 when scaled to milliseconds, and an out-of-range float would + // overflow the cast — both undefined behavior on untrusted input. if (inner->is_int()) { - return Document(TimestampValue{Timestamp::FromEpochMilliseconds(inner->as_int() * 1000), - TimestampFormat::kEpochSeconds}); + auto ts = Timestamp::FromEpochSecondsChecked(static_cast(inner->as_int())); + if (!ts) return std::move(ts).error(); + return Document(TimestampValue{*ts, TimestampFormat::kEpochSeconds}); } if (inner->is_double()) { - return Document(TimestampValue{Timestamp::FromEpochSeconds(inner->as_double()), - TimestampFormat::kEpochSeconds}); + auto ts = Timestamp::FromEpochSecondsChecked(inner->as_double()); + if (!ts) return std::move(ts).error(); + return Document(TimestampValue{*ts, TimestampFormat::kEpochSeconds}); } return Fail("tag 1 content is not a number"); } diff --git a/runtime/src/core/document_serde.cc b/runtime/src/core/document_serde.cc index f2b32707..bb6ea350 100644 --- a/runtime/src/core/document_serde.cc +++ b/runtime/src/core/document_serde.cc @@ -19,7 +19,7 @@ Outcome TimestampFromDocument(const Document& doc, TimestampFormat fo if (format != TimestampFormat::kEpochSeconds) { return Error::Serialization("timestamp: numeric value for a string-formatted timestamp"); } - return Timestamp::FromEpochSeconds(doc.AsNumber()); + return Timestamp::FromEpochSecondsChecked(doc.AsNumber()); } if (doc.is_string()) { if (format == TimestampFormat::kEpochSeconds) { diff --git a/runtime/src/core/timestamp.cc b/runtime/src/core/timestamp.cc index 1b489833..6fddc365 100644 --- a/runtime/src/core/timestamp.cc +++ b/runtime/src/core/timestamp.cc @@ -213,7 +213,21 @@ Outcome ParseEpochSeconds(std::string_view text) { errno = 0; const double seconds = std::strtod(buffer.c_str(), nullptr); if (errno == ERANGE || !std::isfinite(seconds)) return invalid(); - return Timestamp::FromEpochSeconds(seconds); + return Timestamp::FromEpochSecondsChecked(seconds); +} + +// Epoch-milliseconds bounds of the RFC 3339 / IMF-fixdate representable window +// (0000-01-01T00:00:00.000Z .. 9999-12-31T23:59:59.999Z). Instants outside it +// both overflow the arithmetic below and format to text no conformant peer can +// parse, so untrusted numbers beyond it are rejected rather than corrupted. +constexpr std::int64_t kMinRepresentableMs = -62167219200000; // year 0000-01-01 +constexpr std::int64_t kMaxRepresentableMs = 253402300799999; // year 9999-12-31T23:59:59.999 + +Outcome CheckedFromMs(std::int64_t ms) { + if (ms < kMinRepresentableMs || ms > kMaxRepresentableMs) { + return Error::Serialization("timestamp: instant out of representable range (year 0000-9999)"); + } + return Timestamp::FromEpochMilliseconds(ms); } } // namespace @@ -222,6 +236,25 @@ Timestamp Timestamp::FromEpochSeconds(double seconds) { return Timestamp(static_cast(std::llround(seconds * 1000.0))); } +Outcome Timestamp::FromEpochSecondsChecked(double seconds) { + if (!std::isfinite(seconds)) { + return Error::Serialization("timestamp: epoch-seconds is not finite"); + } + // Bound the value before scaling so neither `seconds * 1000` nor the cast to + // int64 can overflow; CheckedFromMs then applies the exact year window. + constexpr double kMaxSeconds = 253402300800.0; // just past year 9999 + constexpr double kMinSeconds = -62167219200.0; // year 0000-01-01 + if (seconds < kMinSeconds || seconds > kMaxSeconds) { + return Error::Serialization( + "timestamp: epoch-seconds out of representable range (year 0000-9999)"); + } + return CheckedFromMs(static_cast(std::llround(seconds * 1000.0))); +} + +Outcome Timestamp::FromEpochMillisecondsChecked(std::int64_t ms) { + return CheckedFromMs(ms); +} + std::string Timestamp::Format(TimestampFormat format) const { const CivilTime c = Decompose(ms_); std::array buffer{}; diff --git a/runtime/tests/cbor/cbor_test.cc b/runtime/tests/cbor/cbor_test.cc index b256a489..455c0e12 100644 --- a/runtime/tests/cbor/cbor_test.cc +++ b/runtime/tests/cbor/cbor_test.cc @@ -86,6 +86,19 @@ TEST(CborTest, DecodesTag1Timestamps) { EXPECT_EQ(fractional->as_timestamp().value.epoch_milliseconds(), 1363896240500); } +// A ~9-byte tag-1 payload whose integer seconds, scaled to milliseconds, would +// overflow int64 (the old `as_int() * 1000` was undefined behavior). Likewise a +// tag-1 double far outside the representable range. Both must be a clean +// decode error, not UB. +TEST(CborTest, RejectsOutOfRangeTag1TimestampsWithoutOverflow) { + // c1 1b 7fffffffffffffff = tag 1 + uint64 9223372036854775807 seconds. + EXPECT_FALSE(Decode(FromHex("c11b7fffffffffffffff")).ok()); + // c1 3b 7fffffffffffffff = tag 1 + a huge negative integer. + EXPECT_FALSE(Decode(FromHex("c13b7fffffffffffffff")).ok()); + // c1 fb 7fe0000000000000 = tag 1 + double ~1.8e308 (near DBL_MAX). + EXPECT_FALSE(Decode(FromHex("c1fb7fe0000000000000")).ok()); +} + TEST(CborTest, RoundTripsComplexDocument) { DocumentMap map; map.emplace("null", Document(nullptr)); diff --git a/runtime/tests/core/timestamp_test.cc b/runtime/tests/core/timestamp_test.cc index 64a7bdc3..edccfdac 100644 --- a/runtime/tests/core/timestamp_test.cc +++ b/runtime/tests/core/timestamp_test.cc @@ -2,6 +2,9 @@ #include +#include +#include + namespace smithy { namespace { @@ -17,6 +20,39 @@ TEST(TimestampParseTest, EpochSecondsIsStrict) { } } +TEST(TimestampCheckedTest, AcceptsInRangeInstants) { + const auto a = Timestamp::FromEpochSecondsChecked(1515531081.123); + ASSERT_TRUE(a.ok()); + EXPECT_EQ(a->epoch_milliseconds(), 1515531081123); + EXPECT_TRUE(Timestamp::FromEpochSecondsChecked(0.0).ok()); + EXPECT_TRUE(Timestamp::FromEpochSecondsChecked(-5.0).ok()); // pre-epoch + EXPECT_TRUE(Timestamp::FromEpochMillisecondsChecked(0).ok()); +} + +TEST(TimestampCheckedTest, AcceptsTheRepresentableBoundaries) { + // The exact edges of the RFC 3339 window round-trip. + const auto max_dt = Timestamp::Parse("9999-12-31T23:59:59.999Z", TimestampFormat::kDateTime); + ASSERT_TRUE(max_dt.ok()); + EXPECT_TRUE(Timestamp::FromEpochMillisecondsChecked(max_dt->epoch_milliseconds()).ok()); + const auto min_dt = Timestamp::Parse("0000-01-01T00:00:00Z", TimestampFormat::kDateTime); + ASSERT_TRUE(min_dt.ok()); + EXPECT_TRUE(Timestamp::FromEpochMillisecondsChecked(min_dt->epoch_milliseconds()).ok()); +} + +TEST(TimestampCheckedTest, RejectsOutOfRangeAndNonFinite) { + // The values the CBOR/JSON UB findings are about: a huge integer scaled to + // milliseconds, an out-of-range float, and non-finite doubles. + EXPECT_FALSE(Timestamp::FromEpochSecondsChecked(1e300).ok()); + EXPECT_FALSE(Timestamp::FromEpochSecondsChecked(-1e300).ok()); + EXPECT_FALSE(Timestamp::FromEpochSecondsChecked(static_cast(9223372036854775LL)).ok()); + EXPECT_FALSE(Timestamp::FromEpochSecondsChecked(std::numeric_limits::infinity()).ok()); + EXPECT_FALSE(Timestamp::FromEpochSecondsChecked(std::nan("")).ok()); + // Just past year 9999 / before year 0000. + const auto past_max = Timestamp::Parse("9999-12-31T23:59:59.999Z", TimestampFormat::kDateTime); + ASSERT_TRUE(past_max.ok()); + EXPECT_FALSE(Timestamp::FromEpochMillisecondsChecked(past_max->epoch_milliseconds() + 1).ok()); +} + TEST(TimestampTest, FormatsDateTime) { // 1985-04-12T23:20:50.520Z, the Smithy spec's canonical example. const auto ts = Timestamp::FromEpochMilliseconds(482196050520);