Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: fix convertALPNProtocols accepting ArrayBufferViews #43211

Merged
merged 1 commit into from
May 29, 2022
Merged

tls: fix convertALPNProtocols accepting ArrayBufferViews #43211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions lib/tls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ const {
} = require('internal/errors').codes;
const internalUtil = require('internal/util');
internalUtil.assertCrypto();
const { isArrayBufferView } = require('internal/util/types');
const {
isArrayBufferView,
isDataView,
isUint8Array,
} = require('internal/util/types');

const net = require('net');
const { getOptionValue } = require('internal/options');
Expand Down Expand Up @@ -143,9 +147,16 @@ exports.convertALPNProtocols = function convertALPNProtocols(protocols, out) {
// If protocols is Array - translate it into buffer
if (ArrayIsArray(protocols)) {
out.ALPNProtocols = convertProtocols(protocols);
} else if (isArrayBufferView(protocols)) {
} else if (Buffer.isBuffer(protocols) || isUint8Array(protocols)) {
Copy link
Member

@lpinca lpinca May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@lpinca lpinca May 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, tested it and

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

Shows 8192 😕

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$ ./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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, sorry.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄).

// Copy new buffer not to be modified by user.
LiviaMedeiros marked this conversation as resolved.
Show resolved Hide resolved
out.ALPNProtocols = Buffer.from(protocols);
} else if (isDataView(protocols)) {
out.ALPNProtocols = Buffer.from(protocols.buffer.slice(
protocols.byteOffset,
protocols.byteOffset + protocols.byteLength
));
} else if (isArrayBufferView(protocols)) {
out.ALPNProtocols = Buffer.from(protocols.slice().buffer);
}
};

Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-tls-basic-validations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,11 @@ assert.throws(
const inputBuffer = Buffer.from(arrayBufferViewStr.repeat(8), 'utf8');
for (const expectView of common.getArrayBufferViews(inputBuffer)) {
const out = {};
const expected = Buffer.from(expectView.buffer.slice(),
expectView.byteOffset,
expectView.byteLength);
tls.convertALPNProtocols(expectView, out);
assert(out.ALPNProtocols.equals(Buffer.from(expectView)));
assert(out.ALPNProtocols.equals(expected));
}
}

Expand Down