Skip to content

Commit bb15ba4

Browse files
ronagclaude
andcommitted
fixup! buffer: support aligned allocations
Address Copilot review feedback: - The test asserted `buf.byteOffset % alignment === 0`, which does not hold: the padding an allocation needs depends on where its backing store lands, so an aligned buffer's `byteOffset` is congruent to `-address` modulo `alignment`, not to 0. Check alignment against `arrayBufferAlignedOffset()` instead, which makes the address alignment genuinely observable from JS rather than only asserted by the binding's CHECKs. - Correct the pre-existing `@returns {FastBuffer|undefined}` on `Buffer.allocUnsafeSlow()`; it never returns undefined. Assisted-by: Claude/Opus 5 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 9e9adee commit bb15ba4

2 files changed

Lines changed: 26 additions & 6 deletions

File tree

‎lib/buffer.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,7 @@ Buffer.allocUnsafe = function allocUnsafe(size, alignment) {
487487
* the returned buffer's `byteOffset` is therefore usually non-zero.
488488
* @param {number} size
489489
* @param {number} [alignment] A power of two, at most 2 ** 30
490-
* @returns {FastBuffer|undefined}
490+
* @returns {FastBuffer}
491491
*/
492492
Buffer.allocUnsafeSlow = function allocUnsafeSlow(size, alignment) {
493493
validateNumber(size, 'size', 0, kMaxLength);

‎test/parallel/test-buffer-alloc-alignment.js‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,28 @@
1+
// Flags: --expose-internals
12
'use strict';
23
require('../common');
34
const assert = require('assert');
45
const { Buffer, constants } = require('buffer');
6+
const { internalBinding } = require('internal/test/binding');
7+
const { arrayBufferAlignedOffset } = internalBinding('buffer');
58

69
// Buffer.allocUnsafe(size, alignment) and Buffer.allocUnsafeSlow(size,
710
// alignment) return a buffer whose memory starts at an address that is a
8-
// multiple of `alignment`. The address is not observable from JS, but the
9-
// binding asserts it internally, so exercising a range of sizes and alignments
10-
// here is what validates it.
11+
// multiple of `alignment`.
12+
13+
// Addresses are not observable from JS, so alignment cannot be checked against
14+
// `byteOffset` alone: the padding an allocation needs depends on where its
15+
// backing store happens to land. `arrayBufferAlignedOffset(ab, alignment)`
16+
// returns an offset into `ab` that is known to be aligned, so any other offset
17+
// is aligned exactly when it is congruent to that one.
18+
function assertAligned(buf, alignment) {
19+
const aligned = arrayBufferAlignedOffset(buf.buffer, alignment);
20+
// `aligned` is in [0, alignment), so adding `alignment` keeps this positive.
21+
const skew = (buf.byteOffset - aligned + alignment) % alignment;
22+
assert.strictEqual(skew, 0,
23+
`byteOffset ${buf.byteOffset} is not ${alignment} byte ` +
24+
`aligned (aligned offsets are ${aligned} mod ${alignment})`);
25+
}
1126

1227
const alignments = [1, 2, 4, 8, 16, 64, 512, 4096, 65536];
1328
const sizes = [0, 1, 7, 64, 65, 512, 4096, 100000];
@@ -17,7 +32,9 @@ for (const alloc of [Buffer.allocUnsafe, Buffer.allocUnsafeSlow]) {
1732
for (const size of sizes) {
1833
const buf = alloc(size, alignment);
1934
assert.strictEqual(buf.length, size);
20-
// Backing store address alignment is not observable from JS; the internal binding CHECKs it.
35+
if (size > 0) {
36+
assertAligned(buf, alignment);
37+
}
2138
// The view must fit inside the (over-allocated) ArrayBuffer.
2239
assert.ok(buf.byteOffset + size <= buf.buffer.byteLength);
2340
// The whole buffer must be writable through the aligned view.
@@ -111,6 +128,9 @@ for (const alloc of [Buffer.allocUnsafe, Buffer.allocUnsafeSlow]) {
111128
// Pooled, so consecutive allocations share an ArrayBuffer. A pool may be
112129
// exhausted in between, hence checking that any two neighbours share one.
113130
assert.ok(bufs.some((buf, i) => i > 0 && buf.buffer === bufs[i - 1].buffer));
131+
for (const buf of bufs) {
132+
assertAligned(buf, 64);
133+
}
114134

115135
// Stricter than the pool alignment, so this gets its own ArrayBuffer.
116136
const own = Buffer.allocUnsafe(64, 128);
@@ -126,7 +146,7 @@ for (const alloc of [Buffer.allocUnsafe, Buffer.allocUnsafeSlow]) {
126146
Buffer.allocUnsafe(24) :
127147
Buffer.allocUnsafe(24, 16);
128148
if (i % 2 === 1) {
129-
assert.strictEqual(buf.byteOffset % 16, 0);
149+
assertAligned(buf, 16);
130150
}
131151
buf.fill(i % 256);
132152
bufs.push(buf);

0 commit comments

Comments
 (0)