Repository navigation
Web chunk(): copy the caller's bytes once, straight into the frame - #76
Conversation
The call-time copy made for the buffer-reuse fix was copied again into the frame when the chunk's turn came. The frame is now built at call time with a blank signature, the data is hashed where it sits in it, and the signature is written in when the chunk is signed.
bompus
added
author:anthropic
![bompus-ai-review[bot]](https://avatars.githubusercontent.com/in/5532859?s=60&v=4){kind=small}
|
|
||
| /** @param {unknown} data */ | ||
| const isBytes = (data) => | ||
| ArrayBuffer.isView(data) && /** @type {any} */ (data).BYTES_PER_ELEMENT === 1; // a Uint8Array of any realm or a Node Buffer; not a DataView or a wider array |
There was a problem hiding this comment.
medium: Validate the view's actual element type · ref 850dd2
The check accepts any ArrayBuffer.isView(data) whose ordinary data.BYTES_PER_ELEMENT property equals 1.
Failure: A DataView can have its own BYTES_PER_ELEMENT: 1 and length: 3 properties. Passing such a view over bytes [1, 2, 3] succeeds and produces a correctly signed frame containing [0, 0, 0], because frameChunk treats the DataView as an array-like object with no indexed elements. The previous copy operation rejected it, and the signing chain now advances.
Suggested fix: Use an intrinsic typed-array brand check that accepts the one-byte typed-array kinds and rejects DataView and wider views, without trusting an overridable property.
| throw new TypeError('data must be a Uint8Array'); | ||
| } | ||
| const frame = frameChunk(blankSignature, data); | ||
| const body = frame.length - 2 - data.length; // the data ends the frame, followed by CR LF; the signature ends the line before it |
There was a problem hiding this comment.
medium: Use one fixed length when framing a growable view · ref e8c2b2
frameChunk(blankSignature, data) reads data.length while building the frame, then body reads data.length again to locate the signature.
Failure: If another worker grows a SharedArrayBuffer while a length-tracking Uint8Array over it is being framed, those length reads can differ. The signature can then overwrite part of ;chunk-signature=, or the header can contain the wrong payload size, so the service rejects the upload. The previous implementation first copied into a fixed-length array.
Suggested fix: Capture the length once, create a fixed-length view for framing, and use that same length to calculate the body and signature offsets.
| const body = frame.length - 2 - data.length; // the data ends the frame, followed by CR LF; the signature ends the line before it | ||
| const at = body - 2 - blankSignature.length; | ||
| const signature = await sign(frame.subarray(body, frame.length - 2)); | ||
| utf8.encodeInto(signature, frame.subarray(at, at + signature.length)); |
There was a problem hiding this comment.
medium: Avoid requiring encodeInto after advancing the chain · ref 80d188
await sign(...) updates previous, after which the code calls utf8.encodeInto(...) to finish the frame.
Failure: In a WebCrypto environment that provides TextEncoder.encode but lacks encodeInto, every nonempty chunk rejects with a TypeError. The rejected chunk has already advanced the chain, so a subsequent end() is also signed against bytes that were never sent.
Suggested fix: Write the ASCII signature directly or use the already-required encode method, and ensure a framing failure cannot commit the signature chain.
Follow-up to #74. That change made the WebCrypto
chunk()copy the caller's buffer at call time, and thenframeChunkcopied the bytes again into the frame when the chunk's turn came. A benchmark of #74 showed the second copy costs about 10% when several 64 KiB chunks are queued at once.Change
chunk()now builds the frame at call time with a blank 64-digit signature. That is the only copy of the bytes, so the caller may still reuse its buffer straight away. The data is hashed where it sits in the frame, and the signature is written into the blank when the chunk's turn finishes. The bytes sent are unchanged (the corpus tests on the sync, dist and web entries pass), and the sync entries and the built files are untouched.The type check moved up front because
frameChunkno longer copies throughUint8Array.prototype.slice:chunk()throws aTypeErrorunless the data is a typed array with one byte per element (Uint8Arrayof any realm, NodeBuffer). ADataView, a wider typed array, a string and an object that only claims to be aUint8Arrayare refused before the chunk takes a turn, so the chain does not move.Measurement
Tinybench 6.2.2 on Bun 1.4.4, one fresh process per arm per run, 5 runs per arm with the order rotated, plus an A/A arm (this change twice). Median p50 latency over the 5 runs, and a preset rule: a difference counts only if it is over max(2%, the A/A gap) and the runs' ranges do not overlap. Arms: before #74 (822135d), #74 (396767a) and this change's
src/web.js(the commit was rebased onto a workflow-only commit afterwards; the file is byte-identical).chunk()64 KiB issued at oncechunk()64 KiB, awaitedchunk()1 KiB, awaitedsign()512 B event, awaitedMax RSS was 87–103 MiB in every arm, with no clear change. Output hashes of a fixed chunk and event sequence matched across all arms.
Limits: Bun only (the web entry targets Workers and browsers, which are V8 and were not measured); one shared host, with admissions refused for pressure bursts or an occupied host lock and retried; the awaited 64 KiB result is near the noise even at 5 runs, so read it as probably small, not zero.
Review
Reviewed once locally. Three low findings were fixed: the first type check could be spoofed with
Symbol.toStringTag; the layout string;chunk-signature=was written in two places (the offset now comes from the end of the frame); andInt8Arraywas refused while the sync entries accept it (a one-byte typed array is accepted again).bun run checkpasses in full, 112 tests with 2 skipped (the new refusal test runs only on the web entry, because the sync entries hash whatevernode:cryptoor Bun accept, such as a string).