-
Notifications
You must be signed in to change notification settings - Fork 36
fix: avoid Blob upload body to prevent native ArrayBuffer leak #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import { randomBytes } from 'node:crypto'; | ||
|
|
||
| const CRLF = '\r\n'; | ||
|
|
||
| export interface MultipartRequestBody { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend dropping multipart altogether, we can just ingest with
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! Much simpler: #316 |
||
| body: Uint8Array<ArrayBuffer>; | ||
| contentType: string; | ||
| } | ||
|
|
||
| // Serializes a single `profile` part into a multipart/form-data request body | ||
| // as a plain, fully-materialized Uint8Array instead of a `FormData` + `Blob`. | ||
| // | ||
| // WHY this is a hand-rolled buffer rather than `FormData`: | ||
| // | ||
| // When the request body is a `FormData` containing a `Blob`, undici's `fetch` | ||
| // serializes each blob part by calling `blob.stream()`. On Node 24.16.0 - | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of v26.0.0–v26.5.x is affected too, not just 24.16–24.18. The upstream fix (nodejs/node#63577) is still not released or backported as of today. |
||
| // 24.18.0 `Blob.prototype.stream()` pins the source `ArrayBuffer` in an | ||
| // eternal (never-released) handle, so every flush leaks one payload-sized | ||
| // ArrayBuffer. With two profilers flushing on the default interval this is a | ||
| // steady, monotonic native-memory leak (~45-80 MB/day in production) that a | ||
| // long-lived process never recovers. | ||
| // - node core bug: https://github.com/nodejs/node/issues/63574 | ||
| // - core fix (main, not yet on 24.x): https://github.com/nodejs/node/pull/63577 | ||
| // - 24.x backport tracking: https://github.com/nodejs/node/issues/64105 | ||
| // | ||
| // Passing a typed-array body sidesteps `Blob.stream()` entirely, so the leak | ||
| // cannot occur on any Node version. It also avoids a wasteful | ||
| // Buffer -> Blob -> stream round-trip on every flush on all versions. The bytes | ||
| // produced here are wire-identical to what undici emits for the equivalent | ||
| // `FormData`/`Blob` (same part framing, headers, and CRLF placement); only the | ||
| // boundary token differs, which is arbitrary by design. | ||
| export function buildProfileMultipartBody( | ||
| content: Uint8Array<ArrayBuffer> | ||
| ): MultipartRequestBody { | ||
| const boundary = `----pyroscope${randomBytes(16).toString('hex')}`; | ||
|
|
||
| const header: Buffer = Buffer.from( | ||
| `--${boundary}${CRLF}` + | ||
| `Content-Disposition: form-data; name="profile"; filename="profile"${CRLF}` + | ||
| `Content-Type: application/octet-stream${CRLF}${CRLF}`, | ||
| 'utf8' | ||
| ); | ||
| const footer: Buffer = Buffer.from(`${CRLF}--${boundary}--${CRLF}`, 'utf8'); | ||
|
|
||
| const body: Buffer<ArrayBuffer> = Buffer.concat([header, content, footer]); | ||
|
|
||
| return { | ||
| body: new Uint8Array(body.buffer, body.byteOffset, body.byteLength), | ||
| contentType: `multipart/form-data; boundary=${boundary}`, | ||
| }; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import { describe, it } from 'node:test'; | ||
| import { strict as assert } from 'node:assert'; | ||
| import { randomBytes } from 'node:crypto'; | ||
| import { Readable } from 'node:stream'; | ||
|
|
||
| import busboy from 'busboy'; | ||
|
|
||
| import { buildProfileMultipartBody } from '../src/utils/build-profile-multipart-body.js'; | ||
|
|
||
| interface ParsedPart { | ||
| name: string; | ||
| filename: string | undefined; | ||
| mimeType: string; | ||
| data: Buffer; | ||
| } | ||
|
|
||
| // Parse a multipart/form-data payload with busboy, the same parser the | ||
| // profiler integration tests use to decode uploads. | ||
| const parseMultipart = ( | ||
| body: Uint8Array, | ||
| contentType: string | ||
| ): Promise<ParsedPart[]> => | ||
| new Promise<ParsedPart[]>((resolve, reject) => { | ||
| const bb = busboy({ headers: { 'content-type': contentType } }); | ||
| const partPromises: Promise<ParsedPart>[] = []; | ||
|
|
||
| bb.on('file', (name, stream, info) => { | ||
| partPromises.push( | ||
| stream.toArray().then((chunks: Buffer[]) => ({ | ||
| name, | ||
| filename: info.filename, | ||
| mimeType: info.mimeType, | ||
| data: Buffer.concat(chunks), | ||
| })) | ||
| ); | ||
| }); | ||
| // 'close' can fire before the per-file toArray() promises settle, so wait | ||
| // on them before resolving. | ||
| bb.on('close', () => { | ||
| Promise.all(partPromises).then(resolve).catch(reject); | ||
| }); | ||
| bb.on('error', reject); | ||
|
|
||
| Readable.from(Buffer.from(body)).pipe(bb); | ||
| }); | ||
|
|
||
| describe('buildProfileMultipartBody', () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest another test to protect against underlying undici changes in the future it('is byte-identical to the FormData/Blob serialization it replaces, modulo the boundary', async () => {
// Payload deliberately contains CRLFs and a leading "--" sequence so the
// comparison also covers framing-adjacent content, not just plain bytes.
const content = Buffer.from('some profile bytes\r\n--tricky\r\n');
// What fetch used to send: a FormData holding one Blob field.
// Response(formData) runs undici's real multipart serializer — the same
// code path fetch uses — without any network I/O. (The one-off
// blob.stream() call this triggers in-test is harmless.)
const formData = new FormData();
formData.append('profile', new Blob([content]), 'profile');
const response = new Response(formData);
const undiciContentType = response.headers.get('content-type') as string;
const undiciBoundary = undiciContentType.split('boundary=')[1];
const undiciBody = Buffer.from(await response.arrayBuffer());
// TODO change to `buildProfileMultipartBody(content)` when buffer is removed
const { body, contentType } = buildProfileMultipartBody(
new Uint8Array(content)
);
const boundary = contentType.split('boundary=')[1];
// Substitute each side's (random) boundary token, then require the full
// byte strings — headers, casing, CRLF placement, epilogue — to match.
// latin1 keeps the comparison byte-exact for non-UTF-8 content.
const normalize = (buffer: Buffer, from: string): string =>
buffer.toString('latin1').replaceAll(from, '<boundary>');
// If undici ever changes its serialization in a future Node release,
// this failing is a feature: it flags that the "wire-identical to the
// old FormData path" guarantee needs to be re-verified against servers.
assert.equal(
normalize(Buffer.from(body), boundary),
normalize(undiciBody, undiciBoundary)
);
}); |
||
| it('sets a multipart/form-data content type with a pyroscope boundary', () => { | ||
| const { contentType } = buildProfileMultipartBody( | ||
| new Uint8Array([1, 2, 3]) | ||
| ); | ||
|
|
||
| assert.match( | ||
| contentType, | ||
| /^multipart\/form-data; boundary=----pyroscope[0-9a-f]{32}$/ | ||
| ); | ||
| }); | ||
|
|
||
| it('serializes the profile part with bytes intact (byte-equal to input)', async () => { | ||
| // Random bytes exercise binary content, including sequences that could | ||
| // resemble CRLF/boundary framing. | ||
| const content = new Uint8Array(randomBytes(4096).buffer); | ||
|
|
||
|
|
||
| const { body, contentType } = buildProfileMultipartBody(content); | ||
| const parts = await parseMultipart(body, contentType); | ||
|
|
||
| assert.equal(parts.length, 1); | ||
| const [part] = parts; | ||
| assert.equal(part.name, 'profile'); | ||
| assert.equal(part.filename, 'profile'); | ||
| assert.equal(part.mimeType, 'application/octet-stream'); | ||
| // Byte-for-byte equality with the buffer handed to the serializer. | ||
| assert.deepEqual(new Uint8Array(part.data), content); | ||
| }); | ||
|
|
||
| it('produces a body that is not a Blob (avoids Blob.stream leak)', () => { | ||
| const { body } = buildProfileMultipartBody( | ||
| new Uint8Array([0, 255, 10, 13]) | ||
| ); | ||
|
|
||
| assert.ok(body instanceof Uint8Array); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Buffer.concatalways copies into a fresh ArrayBuffer-backed Buffer, so the SharedArrayBuffer narrowing no longer serves a purpose soarrayBuffercan be removed andprofileBuffercan be passed directly tobuildProfileMultipartBody