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.

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

Conversation

LiviaMedeiros
Copy link
Contributor

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:

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

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
Copy link
Collaborator

nodejs-github-bot commented May 26, 2022

Review requested:

  • @nodejs/crypto
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels May 26, 2022
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2022
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@LiviaMedeiros LiviaMedeiros force-pushed the tls-fix-convert-alpn-protocols branch from 403e1e3 to 15930c1 Compare May 26, 2022 16:05
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

lib/tls.js Outdated Show resolved Hide resolved
test/parallel/test-tls-basic-validations.js Outdated Show resolved Hide resolved
@@ -143,9 +146,13 @@ 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

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

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

lib/tls.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@LiviaMedeiros LiviaMedeiros force-pushed the tls-fix-convert-alpn-protocols branch from 220bc10 to 15a682c Compare May 29, 2022 08:14
@LiviaMedeiros LiviaMedeiros merged commit 15a682c into nodejs:master May 29, 2022
@LiviaMedeiros
Copy link
Contributor Author

Landed in 15a682c

@mscdex
Copy link
Contributor

mscdex commented May 30, 2022

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
Copy link
Contributor Author

The benchmark/tls/convertprotocols.js tests only array

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.

bengl pushed a commit that referenced this pull request May 30, 2022
PR-URL: #43211
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@bengl bengl mentioned this pull request May 31, 2022
F3n67u pushed a commit to F3n67u/node that referenced this pull request Jun 1, 2022
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #43211
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
PR-URL: #43211
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
PR-URL: #43211
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants