buffer: make methods work on Uint8Array instances #56578

nbbeeken · 2025-01-13T02:23:26Z

Removes the reliance on prototype bound methods internally
so that Uint8Arrays can be set as the bound this value when calling
the various Buffer methods. Introduces some additional tamper protection
by removing internal reliance on writable properties.

Fixes: #56577

codecov · 2025-01-13T03:51:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.22%. Comparing base (2e45656) to head (61c5588).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56578   +/-   ##
=======================================
  Coverage   89.21%   89.22%           
=======================================
  Files         662      662           
  Lines      191883   191902   +19     
  Branches    36941    36932    -9     
=======================================
+ Hits       171196   171218   +22     
- Misses      13536    13538    +2     
+ Partials     7151     7146    -5

Files with missing lines	Coverage Δ
lib/buffer.js	`100.00% <100.00%> (ø)`
lib/internal/buffer.js	`98.57% <100.00%> (+0.09%)`	⬆️

... and 23 files with indirect coverage changes

addaleax · 2025-01-13T17:04:16Z

doc/api/buffer.md

@@ -415,6 +415,21 @@ function:
 * [`Buffer.from(arrayBuffer[, byteOffset[, length]])`][`Buffer.from(arrayBuf)`]
 * [`Buffer.from(string[, encoding])`][`Buffer.from(string)`]

+### Buffer methods are callable with `Uint8Array` instances


I'd add a changes: YAML entry here that lists all the methods for which behavior actually changed, as well as for the individual methods where this behavior has been modified, so that it's easier for users to track which Node.js versions support this behavior and which don't

addaleax · 2025-01-13T17:08:02Z

lib/internal/buffer.js

  if (byteLength === 1)
-    return this.readInt8(offset);
+    return FunctionPrototypeCall(readInt8, this, offset);


This is already (correctly imo) marked as needing a benchmark CI run, but just as a heads up, if this does turn out to be an issue, it shouldn't be a big deal to create non-prototype versions of these functions that don't use this as the target

Yea I was thinking of that as an alternative implementation but took the route of least blast radius to start, happy to go through and change them to match the other int parsers that take the argument. (Should we go ahead and do that regardless of the benchmarks?)

Should we go ahead and do that regardless of the benchmarks?

No strong preferences from my side, maybe somebody else has one. But for now, I'd say we can fix the failures reported by GHA, then run regular CI + benchmark CI and see what happens

buffers/buffer-bytelength-string.js n=4000000 repeat=2 encoding='utf8' type='two_bytes' ** -1.37 % ±0.99% ±1.32% ±1.71% buffers/buffer-bytelength-string.js n=4000000 repeat=256 encoding='base64' type='latin1' ** 2.49 % ±1.83% ±2.45% ±3.24% buffers/buffer-creation.js n=600000 len=1024 type='slow-allocUnsafe' *** -3.01 % ±1.32% ±1.76% ±2.29% buffers/buffer-creation.js n=600000 len=4096 type='fast-allocUnsafe' *** -4.03 % ±2.22% ±2.96% ±3.85% buffers/buffer-creation.js n=600000 len=8192 type='fast-allocUnsafe' ** -2.50 % ±1.64% ±2.19% ±2.86% buffers/buffer-creation.js n=600000 len=8192 type='slow-allocUnsafe' ** -2.49 % ±1.44% ±1.92% ±2.50% buffers/buffer-fill.js n=20000 size=8192 type='fill("t", 0)' ** -4.09 % ±3.06% ±4.09% ±5.37% buffers/buffer-from.js n=800000 len=100 source='uint16array' *** -2.03 % ±0.98% ±1.31% ±1.71% buffers/buffer-from.js n=800000 len=2048 source='arraybuffer-middle' ** 1.65 % ±1.21% ±1.61% ±2.09% buffers/buffer-from.js n=800000 len=2048 source='arraybuffer' ** 1.82 % ±1.10% ±1.46% ±1.91% buffers/buffer-swap.js n=1000000 len=256 method='swap32' aligned='false' ** 0.75 % ±0.48% ±0.64% ±0.84% buffers/buffer-swap.js n=1000000 len=768 method='swap16' aligned='false' ** -1.57 % ±1.08% ±1.45% ±1.90% buffers/dataview-set.js n=1000000 type='Uint16BE' *** -24.92 % ±1.98% ±2.65% ±3.50% buffers/dataview-set.js n=1000000 type='Uint16LE' *** -25.11 % ±0.99% ±1.32% ±1.72% 23.55 false positives, when considering a 5% risk acceptance (*, **, ***), 4.71 false positives, when considering a 1% risk acceptance (**, ***), 0.47 false positives, when considering a 0.1% risk acceptance (***)

Just filtered for the two and three star confidence ones and the changes are small, except for oddly the dataview tests, which just happen to be in the buffers folder, not sure how I could've changed that value but maybe this won't reproduce in CI? 🤞🏻

addaleax · 2025-01-13T17:13:21Z

lib/buffer.js

@@ -1026,7 +1044,7 @@ Buffer.prototype.lastIndexOf = function lastIndexOf(val, byteOffset, encoding) {
 };

 Buffer.prototype.includes = function includes(val, byteOffset, encoding) {
-  return this.indexOf(val, byteOffset, encoding) !== -1;
+  return bidirectionalIndexOf(this, val, byteOffset, encoding) !== -1;


Suggested change

return bidirectionalIndexOf(this, val, byteOffset, encoding) !== -1;

return bidirectionalIndexOf(this, val, byteOffset, encoding, true) !== -1;

This is a test failure that GHA mentions that seemed fairly easy to identify, I'm not sure if the others are related

nbbeeken · 2025-01-16T00:15:29Z

test/fixtures/permission/fs-traversal.js

-  // Sanity check (remove if the internals of Buffer.from change):
-  // The custom implementation of utf8Write should cause Buffer.from() to encode
-  // traversalPath instead of the sanitized output of resolve().
-  assert.strictEqual(Buffer.from(resolve(traversalPathWithExtraChars)).toString(), traversalPath);


If I understood correctly this can be deleted now that Buffer.prototype.utf8Write tampering doesn't impact from/toString. However, the test still has value (along with the tampering code above) if that were to be reversed at any point.

nbbeeken

toString()
write()
[Symbol.for('nodejs.util.inspect.custom')]()
includes()
readIntBE() (byteLength: 4, 2, 1)
readIntLE() (byteLength: 4, 2, 1)
readUIntBE() (byteLength: 4, 2, 1)
readUIntLE() (byteLength: 4, 2, 1)

still need to update the docs, just noting the functions changed here

Removes the reliance on prototype bound methods internally so that Uint8Arrays can be set as the bound `this` value when calling the various Buffer methods. Introduces some additional tamper protection by removing internal reliance on writable properties. Fixes: nodejs#56577

nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Jan 13, 2025

nbbeeken force-pushed the buffer-generic-methods branch from c35a917 to c73de47 Compare January 13, 2025 02:26

BridgeAR added the needs-benchmark-ci PR that need a benchmark CI run. label Jan 13, 2025

addaleax reviewed Jan 13, 2025

View reviewed changes

nbbeeken force-pushed the buffer-generic-methods branch from c73de47 to d2841d6 Compare January 15, 2025 23:59

nbbeeken commented Jan 16, 2025

View reviewed changes

nbbeeken commented Jan 18, 2025

View reviewed changes

nbbeeken force-pushed the buffer-generic-methods branch from fde97b3 to 01c4725 Compare January 18, 2025 03:34

docs: lint

61c5588

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: make methods work on Uint8Array instances #56578

buffer: make methods work on Uint8Array instances #56578

nbbeeken commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

addaleax Jan 13, 2025

addaleax Jan 13, 2025

nbbeeken Jan 13, 2025

addaleax Jan 14, 2025

nbbeeken Jan 16, 2025

addaleax Jan 13, 2025

nbbeeken Jan 16, 2025

nbbeeken left a comment

	return bidirectionalIndexOf(this, val, byteOffset, encoding) !== -1;
	return bidirectionalIndexOf(this, val, byteOffset, encoding, true) !== -1;

buffer: make methods work on Uint8Array instances #56578

Are you sure you want to change the base?

buffer: make methods work on Uint8Array instances #56578

Conversation

nbbeeken commented Jan 13, 2025 • edited Loading

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

addaleax Jan 13, 2025

Choose a reason for hiding this comment

addaleax Jan 13, 2025

Choose a reason for hiding this comment

nbbeeken Jan 13, 2025

Choose a reason for hiding this comment

addaleax Jan 14, 2025

Choose a reason for hiding this comment

nbbeeken Jan 16, 2025

Choose a reason for hiding this comment

addaleax Jan 13, 2025

Choose a reason for hiding this comment

nbbeeken Jan 16, 2025

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

nbbeeken commented Jan 13, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading