Bun.serve: keep a split request head alive when its socket closes during the dispatch

packages/bun-uws/src/HttpParser.h

@@ -1531,13 +1531,21 @@ struct HttpResponseData;
 
             size_t maxCopyDistance = std::min<size_t>(maxFallbackSize - fallback.length(), (size_t) length);
 
+            /* This frame owns the buffer while the head is parsed out of it. The
+             * dispatch below can destruct this parser (the socket closes or is
+             * upgraded inside the handler) while the HttpRequest it was given
+             * still holds string_views into these bytes. */
+            std::string head = std::move(fallback);
+            fallback.clear();
+
             /* We don't want fallback to be short string optimized, since we want to move it */
-            fallback.reserve(fallback.length() + maxCopyDistance + std::max<unsigned int>(MINIMUM_HTTP_POST_PADDING, sizeof(std::string)));
-            fallback.append(data, maxCopyDistance);
+            head.reserve(head.length() + maxCopyDistance + std::max<unsigned int>(MINIMUM_HTTP_POST_PADDING, sizeof(std::string)));
+            head.append(data, maxCopyDistance);
 
             // break here on break
-            HttpParserResult consumed = fenceAndConsumePostPadded<true, IsNodeHttp>(maxHeaderSize, isConnectRequest, requireHostHeader, useStrictMethodValidation, useInsecureHTTPParser, useLenientTransferEncoding, nodeHttpRequestTrailers, chunkedExtensionsByteCount, fallback.data(), (unsigned int) fallback.length(), user, &req, requestHandler, dataHandler);
-            /* Return data will be different than user if we are upgraded to WebSocket or have an error */
+            HttpParserResult consumed = fenceAndConsumePostPadded<true, IsNodeHttp>(maxHeaderSize, isConnectRequest, requireHostHeader, useStrictMethodValidation, useInsecureHTTPParser, useLenientTransferEncoding, nodeHttpRequestTrailers, chunkedExtensionsByteCount, head.data(), (unsigned int) head.length(), user, &req, requestHandler, dataHandler);
+            /* Return data will be different than user if we are upgraded to WebSocket or have an error.
+             * The parser can be gone by now: do not touch a member before this return. */
             if (consumed.returnedData != user) {
                 /* The count is in fallback bytes, and the first `had` of them came from
                  * earlier reads. The head ends past them: those reads did not complete it. */
@@ -1553,7 +1561,6 @@ struct HttpResponseData;
                 /* This logic assumes that we consumed everything in fallback buffer.
                 * This is critically important, as we will get an integer overflow in case
                 * of "had" being larger than what we consumed, and that we would drop data */
-                fallback.clear();
                 data += consumedBytes - had;
                 length -= consumedBytes - had;
 
@@ -1616,6 +1623,8 @@ struct HttpResponseData;
                 }
 
             } else {
+                /* Short read: nothing was dispatched, keep accumulating. */
+                fallback = std::move(head);
                 if (fallback.length() == maxFallbackSize) {
                     return HttpParserResult::error(HTTP_ERROR_431_REQUEST_HEADER_FIELDS_TOO_LARGE, HTTP_PARSER_ERROR_REQUEST_HEADER_FIELDS_TOO_LARGE);
                 }

test/js/bun/http/serve-pending-promise-abort-leak.test.ts

@@ -1,5 +1,5 @@
 import { expect, test } from "bun:test";
-import { bunEnv, bunExe } from "harness";
+import { bunEnv, bunExe, bunRun } from "harness";
 import { connect } from "node:net";
 import { join } from "node:path";
 
@@ -12,6 +12,10 @@ async function stopAndAssertDrained(server: ReturnType<typeof Bun.serve>) {
   expect(server.pendingRequests).toBe(0);
 }
 
+// symbolize=0 so an unfixed build's ASAN abort exits promptly instead of
+// spending seconds in llvm-symbolizer.
+const asanOptionsWithoutSymbolizer = [bunEnv.ASAN_OPTIONS, "symbolize=0"].filter(Boolean).join(":");
+
 test.each([false, true])(
   "RequestContext is freed when client aborts before Promise<Response> settles (http2: %p)",
   async http2 => {
@@ -737,6 +741,40 @@ test.each(stoppedRequests)("server.stop(true) inside the handler of %s aborts it
   await stopped!;
 });
 
+// A request head that arrives split over two reads is parsed out of the HTTP
+// parser's per-socket fallback buffer, and the uWS request the dispatch holds
+// views into that buffer. server.stop(true) inside the handler closes the
+// request's own socket right there, and the close destructed the parser with
+// its buffer. Everything that materialises the headers after that read freed
+// memory: `req.headers` inside the handler, and the snapshot the server takes
+// itself when an async handler ends the dispatch. Under a sanitizer it is a
+// heap-use-after-free; without one the headers come back as the bytes of
+// whatever allocation took the block over.
+test.concurrent.each(["lazy", "async"])(
+  "a request head split over two reads survives server.stop(true) in the handler (%s headers)",
+  async mode => {
+    expect(
+      await bunRun(join(import.meta.dir, "serve-split-head-stop-fixture.ts"), {
+        SPLIT_HEAD_MODE: mode,
+        ASAN_OPTIONS: asanOptionsWithoutSymbolizer,
+      }),
+    ).toSpawn(`${Buffer.alloc(40, 0x4d).toString()}|300|host,x-mark,x-pad`);
+  },
+);
+
+// The same buffer, reached by the client alone: no server API call and no
+// nested event loop. The head is split, the request declares a body it never
+// sends, and `Connection: close` makes the completed response close the socket
+// inside the dispatch. The pending `req.text()` then rejects, and its handler
+// reads a url and headers that the close already freed.
+test.concurrent("a split request head survives a Connection: close response on an unfinished body", async () => {
+  expect(
+    await bunRun(join(import.meta.dir, "serve-split-head-close-fixture.ts"), {
+      ASAN_OPTIONS: asanOptionsWithoutSymbolizer,
+    }),
+  ).toSpawn(`AbortError|"http://x/a"|300|connection,content-length,host,x-pad`);
+});
+
 // A Response the server will never render still owns a body stream that
 // somebody produces into. The server has to cancel it, like a client abort
 // after the stream was attached does, or the producer waits for a pull that