buffer: make methods work on Uint8Array instances

[nbbeeken]

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]

Codecov Report

Attention: Patch coverage is 91.12426% with 15 lines in your changes missing coverage. Please review.

Project coverage is 90.21%. Comparing base (db00f94) to head (6c65286).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_buffer.cc	10.00%	0 Missing and 9 partials ⚠️
lib/internal/buffer.js	95.00%	6 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #56578   +/-   ##
=======================================
  Coverage   90.20%   90.21%           
=======================================
  Files         630      630           
  Lines      185166   185188   +22     
  Branches    36227    36215   -12     
=======================================
+ Hits       167036   167070   +34     
+ Misses      11117    11114    -3     
+ Partials     7013     7004    -9     

Files with missing lines	Coverage Δ
lib/buffer.js	100.00% <100.00%> (ø)
lib/internal/http2/core.js	95.61% <100.00%> (ø)
lib/internal/buffer.js	98.75% <95.00%> (+<0.01%)	⬆️
src/node_buffer.cc	67.23% <10.00%> (ø)

... and 30 files with indirect coverage changes

[addaleax]

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

[nbbeeken]

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?)

[addaleax]

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

[nbbeeken]

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]

I think the up-to-minus-4% regressions on the others are also a bit concerning though 😕

To me this does feel like an impact we'd want to avoid if we can at all

[nbbeeken]

SGTM, I'll try switching over to direct invocations and see if we climb back up

[nbbeeken]

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