From c6f439ca37c5fa34acc54a6df79214ae029ddf9f Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 7 Aug 2023 14:48:30 -0700 Subject: [PATCH] Avoid infinite recursion bug in server.c++ coroutine (#987) --- src/workerd/server/server.c++ | 90 +++++++++++++++++------------------ 1 file changed, 45 insertions(+), 45 deletions(-) diff --git a/src/workerd/server/server.c++ b/src/workerd/server/server.c++ index 931158c51f2..18754ce85f7 100644 --- a/src/workerd/server/server.c++ +++ b/src/workerd/server/server.c++ @@ -2385,62 +2385,62 @@ public: rewriter(kj::mv(rewriter)) {} kj::Promise run() { - kj::AuthenticatedStream stream = co_await listener->acceptAuthenticated(); + for (;;) { + kj::AuthenticatedStream stream = co_await listener->acceptAuthenticated(); - kj::Maybe cfBlobJson; - if (!rewriter->hasCfBlobHeader()) { - // Construct a cf blob describing the client identity. - - kj::PeerIdentity* peerId; - - KJ_IF_MAYBE(tlsId, - kj::dynamicDowncastIfAvailable(*stream.peerIdentity)) { - peerId = &tlsId->getNetworkIdentity(); + kj::Maybe cfBlobJson; + if (!rewriter->hasCfBlobHeader()) { + // Construct a cf blob describing the client identity. - // TODO(someday): Add client certificate info to the cf blob? At present, KJ only - // supplies the common name, but that doesn't even seem to be one of the fields that - // Cloudflare-hosted Workers receive. We should probably try to match those. - } else { - peerId = stream.peerIdentity; - } + kj::PeerIdentity* peerId; - KJ_IF_MAYBE(remote, - kj::dynamicDowncastIfAvailable(*peerId)) { - cfBlobJson = kj::str("{\"clientIp\": \"", escapeJsonString(remote->toString()), "\"}"); - } else KJ_IF_MAYBE(local, - kj::dynamicDowncastIfAvailable(*peerId)) { - auto creds = local->getCredentials(); + KJ_IF_MAYBE(tlsId, + kj::dynamicDowncastIfAvailable(*stream.peerIdentity)) { + peerId = &tlsId->getNetworkIdentity(); - kj::Vector parts; - KJ_IF_MAYBE(p, creds.pid) { - parts.add(kj::str("\"clientPid\":", *p)); - } - KJ_IF_MAYBE(u, creds.uid) { - parts.add(kj::str("\"clientUid\":", *u)); + // TODO(someday): Add client certificate info to the cf blob? At present, KJ only + // supplies the common name, but that doesn't even seem to be one of the fields that + // Cloudflare-hosted Workers receive. We should probably try to match those. + } else { + peerId = stream.peerIdentity; } - cfBlobJson = kj::str("{", kj::strArray(parts, ","), "}"); - } - } + KJ_IF_MAYBE(remote, + kj::dynamicDowncastIfAvailable(*peerId)) { + cfBlobJson = kj::str("{\"clientIp\": \"", escapeJsonString(remote->toString()), "\"}"); + } else KJ_IF_MAYBE(local, + kj::dynamicDowncastIfAvailable(*peerId)) { + auto creds = local->getCredentials(); - auto conn = kj::heap(*this, kj::mv(cfBlobJson)); + kj::Vector parts; + KJ_IF_MAYBE(p, creds.pid) { + parts.add(kj::str("\"clientPid\":", *p)); + } + KJ_IF_MAYBE(u, creds.uid) { + parts.add(kj::str("\"clientUid\":", *u)); + } - static auto constexpr listen = [](kj::Own self, - kj::Own conn, - kj::Own stream) -> kj::Promise { - try { - co_await conn->listedHttp.httpServer.listenHttp(kj::mv(stream)); - } catch (...) { - KJ_LOG(ERROR, kj::getCaughtExceptionAsKj()); + cfBlobJson = kj::str("{", kj::strArray(parts, ","), "}"); + } } - }; - // Run the connection handler loop in the global task set, so that run() waits for open - // connections to finish before returning, even if the listener loop is canceled. However, - // do not consider exceptions from a specific connection to be fatal. - owner.tasks.add(listen(kj::addRef(*this), kj::mv(conn), kj::mv(stream.stream))); + auto conn = kj::heap(*this, kj::mv(cfBlobJson)); - co_await run(); + static auto constexpr listen = [](kj::Own self, + kj::Own conn, + kj::Own stream) -> kj::Promise { + try { + co_await conn->listedHttp.httpServer.listenHttp(kj::mv(stream)); + } catch (...) { + KJ_LOG(ERROR, kj::getCaughtExceptionAsKj()); + } + }; + + // Run the connection handler loop in the global task set, so that run() waits for open + // connections to finish before returning, even if the listener loop is canceled. However, + // do not consider exceptions from a specific connection to be fatal. + owner.tasks.add(listen(kj::addRef(*this), kj::mv(conn), kj::mv(stream.stream))); + } } private: