tls: fix convertALPNProtocols accepting ArrayBufferViews

[LiviaMedeiros]

Fix for ArrayBufferViews that are not Uint8Array.

I believe it should be eventually adjusted on buffer side, but not sure if it would be easy or worth potential breakage.

Audit through make test gave me two misuses, both test-only and safe:

node/test/parallel/test-buffer-alloc.js

Lines 46 to 47 in a346572

	const ui32 = new Uint32Array(4).fill(42);
	const e = Buffer.from(ui32);

node/test/parallel/test-webcrypto-random.js

Lines 50 to 51 in a346572

	const buf = new Uint16Array(10);
	const before = Buffer.from(buf).toString('hex');

However, there could still be similar bugs within crypto in places where Buffer.from(buffer) is used.

/cc @nodejs/crypto

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/net

[mcollina]

lgtm

[lpinca]

Is isUint8Array() faster than isArrayBufferView()? If not we can remove it as it would be handled by the isArrayBufferView() check below.

[LiviaMedeiros]

The question here is, is Buffer.from(buffer) faster or better than Buffer.from(arrayBuffer, byteOffset, length)? Having Buffer or Uint8Array here is expected in most cases, otherwise we would got bugreports for this.
I don't really know but my guess is "yes, it should be faster" because Buffer tends to use shared pools, and by doing buffer.slice() we would copy the whole pool.

[lpinca]

I don't really know but my guess is "yes, it should be faster" because Buffer tends to use shared pools, and by doing buffer.slice() we would copy the whole pool.

We should keep it as is when protocols is a Buffer but when it is an Uint8Array I'm not sure. AFAIK there is no pool for Uint8Arrays.

[lpinca]

Anyway it is not very important, it would only slightly improve readability by removing the OR condition. Feel free to ignore.

[LiviaMedeiros]

Hmm, tested it and

const { Buffer } = require('node:buffer');
console.log(Buffer.from(new Uint8Array([1,2,3])).buffer.byteLength);

Shows 8192 😕

[lpinca]

$ node
Welcome to Node.js v18.2.0.
Type ".help" for more information.
> const arr = new Uint8Array([1,2,3]);
undefined
> arr.buffer.byteLength
3
> const buf = Buffer.from(arr.slice(), arr.byteOffset, arr.byteLength);
undefined
> buf.buffer.byteLength
8192

[LiviaMedeiros]

$ ./node
Welcome to Node.js v19.0.0-pre.
Type ".help" for more information.
> const arr = new Uint8Array([1,2,3]);
undefined
> const buf = Buffer.from(arr.buffer.slice(), arr.byteOffset, arr.byteLength);
undefined
> buf.buffer.byteLength
3

The one above uses Uint8Array directly rather than ArrayBuffer :)

[lpinca]

Oh right, sorry.

[lpinca]

Isn't that the point of speedup? I mean, because of that in practice we should prefer doing Buffer.from(arr) directly so it will be able to reuse preallocated pool.

Yes, it makes sense.

[LiviaMedeiros]

Actually, it was the right direction, applicable to the next branch. I forgot that TypedArray.prototype.slice().buffer, unlike Buffer's version, does exactly what's needed there. Calling .buffer.slice() without arguments is in fact a huge mistake.
Unfortunately, DataView is not a TypedArray and supporting it requires slightly different approach.

Synthetic microbenchmarking shows ~50% speedup for TypedArray way over DataView, and ~700% for Buffer|Uint8Array way over DataView for me.
If readability suffered too much from that, last else if (isArrayBufferView) branch can be removed. Otherwise it seems to be the most correct workaround (at least until Buffer.from(anyArrayBufferView) is somehow supported 😄).

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44198/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/44204/

[LiviaMedeiros]

Landed in 15a682c

[mscdex]

The tls/convertprotocols.js benchmark is showing a ~7.5% performance regression after this commit. Looking at the changes, do we really need Buffer.isBuffer() since isUint8Array() will work in either case? I'm not sure if changing that alone will fix the regression or not...

[LiviaMedeiros]

The benchmark/tls/convertprotocols.js tests only array

node/benchmark/tls/convertprotocols.js

Line 11 in d72dcb0

const input = ['ABC', 'XYZ123', 'FOO'];

Which is covered by the very first condition; so I'm not sure as well.

As long as it doesn't affect readability, yes, it's worth a try to remove Buffer.isBuffer() check and merge DataView and ArrayBufferView together (safe to assume it to be a cold path). I'll open a PR for that.