From cd53d32ccf7bd0badd95e5d8fc9a80169acf939e Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 26 Jan 2025 23:16:35 -0800 Subject: [PATCH] Fix memory leak in certain cases when long URLs are passed to req.url (#16787) --- src/bun.js/webcore/request.zig | 1 + test/js/bun/http/req-url-leak-fixture.js | 26 +++++++++++++ test/js/bun/http/req-url-leak.test.ts | 47 ++++++++++++++++++++++++ 3 files changed, 74 insertions(+) create mode 100644 test/js/bun/http/req-url-leak-fixture.js create mode 100644 test/js/bun/http/req-url-leak.test.ts diff --git a/src/bun.js/webcore/request.zig b/src/bun.js/webcore/request.zig index 7274373be0bfb1..a3a63b8e22e8f9 100644 --- a/src/bun.js/webcore/request.zig +++ b/src/bun.js/webcore/request.zig @@ -490,6 +490,7 @@ pub const Request = struct { const href = bun.JSC.URL.hrefFromString(this.url); // TODO: what is the right thing to do for invalid URLS? if (!href.isEmpty()) { + this.url.deref(); this.url = href; } diff --git a/test/js/bun/http/req-url-leak-fixture.js b/test/js/bun/http/req-url-leak-fixture.js new file mode 100644 index 00000000000000..25dbbdf06173a3 --- /dev/null +++ b/test/js/bun/http/req-url-leak-fixture.js @@ -0,0 +1,26 @@ +let existingPromise = null; +const server = Bun.serve({ + port: 0, + async fetch(req) { + if (!existingPromise) { + existingPromise = Bun.sleep(0); + } + let waitedUpon = existingPromise; + await existingPromise; + if (existingPromise === waitedUpon) { + existingPromise = null; + } + return new Response(process.memoryUsage.rss().toString()); + }, +}); + +setInterval(() => { + console.log("RSS", (process.memoryUsage.rss() / 1024 / 1024) | 0); +}, 1000); +console.log("Server started on", server.url.href); + +if (process.channel) { + process.send({ + url: server.url.href, + }); +} diff --git a/test/js/bun/http/req-url-leak.test.ts b/test/js/bun/http/req-url-leak.test.ts new file mode 100644 index 00000000000000..2479c91b367eaa --- /dev/null +++ b/test/js/bun/http/req-url-leak.test.ts @@ -0,0 +1,47 @@ +import { test, expect } from "bun:test"; +import { bunExe, bunEnv } from "harness"; +import path from "path"; +test("req.url doesn't leak memory", async () => { + const { promise, resolve } = Promise.withResolvers(); + await using process = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "req-url-leak-fixture.js")], + env: bunEnv, + stdout: "inherit", + stderr: "inherit", + stdin: "inherit", + ipc(message, child) { + if (message.url) { + resolve(message.url); + } + }, + }); + + const baseURL = await promise; + + const url = new URL(Buffer.alloc(1024 * 15, "Z").toString(), baseURL); + + let maxRSS = 0; + + for (let i = 0; i < 512; i++) { + const batchSize = 64; + const promises = []; + for (let j = 0; j < batchSize; j++) { + promises.push( + fetch(url) + .then(r => r.text()) + .then(rssText => { + const rss = parseFloat(rssText); + if (Number.isSafeInteger(rss)) { + maxRSS = Math.max(maxRSS, rss); + } + }), + ); + } + await Promise.all(promises); + } + + console.log("RSS", maxRSS); + + // 557 MB on Bun 1.2 + expect(maxRSS).toBeLessThan(1024 * 1024 * 256); +}, 10_000);