Skip to content

src: remove usages of GetBackingStore in crypto#44079

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
kvakil:crypto-backingstores
Aug 4, 2022
Merged

src: remove usages of GetBackingStore in crypto#44079
nodejs-github-bot merged 2 commits intonodejs:mainfrom
kvakil:crypto-backingstores

Conversation

@kvakil
Copy link
Contributor

@kvakil kvakil commented Aug 1, 2022

This removes all usages of GetBackingStore in crypto. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
ArrayBufferOrViewContents -- I am pretty sure it is correct based on a
manual audit of the callsites & by analogy to ArrayBufferViewContents,
but please ensure that it is correct. If we are at all unsure of the
safety here, I would prefer to abandon this change.

Refs: #32226
Refs: #43921

@kvakil
src: remove usages of GetBackingStore in crypto
55bcf9a 
This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: nodejs#32226
Refs: nodejs#43921
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 1, 2022

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 1, 2022
@kvakil kvakil mentioned this pull request Aug 1, 2022
Merged
mcollina
mcollina approved these changes Aug 1, 2022
View reviewed changes
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

F3n67u
F3n67u approved these changes Aug 1, 2022
View reviewed changes
@F3n67u F3n67u added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 1, 2022
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 1, 2022

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

addaleax
addaleax requested changes Aug 1, 2022
View reviewed changes
@kvakil kvakil requested a review from addaleax August 1, 2022 20:53
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 1, 2022

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

@github-actions github-actions bot mentioned this pull request Aug 2, 2022
Open
11 tasks
kvakil added a commit to kvakil/node that referenced this pull request Aug 2, 2022
@kvakil
src: prevent copying ArrayBufferViewContents
d1f3368 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: nodejs#44079 (comment)
@kvakil kvakil mentioned this pull request Aug 2, 2022
Merged
@kvakil kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 2, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 2, 2022

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

@kvakil kvakil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 2, 2022
@kvakil kvakil self-assigned this Aug 2, 2022
tniessen
tniessen reviewed Aug 2, 2022
View reviewed changes
src/crypto/crypto_util.h
void operator=(const ArrayBufferOrViewContents&) = delete;

inline explicit ArrayBufferOrViewContents(v8::Local<v8::Value> buf) {
if (buf.IsEmpty()) {
Copy link
Member

@tniessen tniessen Aug 2, 2022

Choose a reason for hiding this comment

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

I understand why this exists, but it feels somewhat strange to explicitly allow passing in empty Locals.

Copy link
Contributor Author

@kvakil kvakil Aug 2, 2022

Choose a reason for hiding this comment

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

I agree. An alternative would be to allocate an empty ArrayBuffer and pass it into this function. None of the current callsites looked particularly hot, but I wasn't sure.

Do you think that would be acceptable wrt to performance?

(P.S. if your concern here is blocking, feel free to request changes on the PR.)

@github-actions github-actions bot mentioned this pull request Aug 3, 2022
Open
15 tasks
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 4, 2022
@nodejs-github-bot nodejs-github-bot merged commit 4c725f4 into nodejs:main Aug 4, 2022
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 4, 2022

Landed in 4c725f4

This was referenced Aug 5, 2022
Open
Open
nodejs-github-bot pushed a commit that referenced this pull request Aug 6, 2022
@kvakil
src: prevent copying ArrayBufferViewContents
760ecc9 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
@kvakil @danielleadams
src: remove usages of GetBackingStore in crypto
ebff06b 
This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
@kvakil @danielleadams
src: prevent copying ArrayBufferViewContents
8082c24 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@danielleadams danielleadams mentioned this pull request Aug 16, 2022
Closed
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
@kvakil @ruyadorno
src: remove usages of GetBackingStore in crypto
4755ad5 
This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: #32226
Refs: #43921
PR-URL: #44079
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
@kvakil @ruyadorno
src: prevent copying ArrayBufferViewContents
960a209 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@ruyadorno ruyadorno mentioned this pull request Aug 23, 2022
Merged
targos pushed a commit that referenced this pull request Sep 5, 2022
@kvakil @targos
src: prevent copying ArrayBufferViewContents
985ffc0 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@kvakil @Fyko
src: remove usages of GetBackingStore in crypto
76876ab 
This removes all usages of GetBackingStore in `crypto`. See the
linked issue for an explanation.

Note: I am not sure of the lifetime semantics intended by
`ArrayBufferOrViewContents` -- I am pretty sure it is correct based on
a manual audit of the callsites, but please ensure that it is correct.

Refs: nodejs#32226
Refs: nodejs#43921
PR-URL: nodejs#44079
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
@kvakil @Fyko
src: prevent copying ArrayBufferViewContents
983a57d 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: nodejs#44079 (comment)
PR-URL: nodejs#44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
juanarbol pushed a commit that referenced this pull request Oct 10, 2022
@kvakil @juanarbol
src: prevent copying ArrayBufferViewContents
b79fc57 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
juanarbol pushed a commit that referenced this pull request Oct 11, 2022
@kvakil @juanarbol
src: prevent copying ArrayBufferViewContents
4ee8ac3 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: #44079 (comment)
PR-URL: #44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@kvakil @guangwong
src: prevent copying ArrayBufferViewContents
21e03ac 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: nodejs/node#44079 (comment)
PR-URL: nodejs/node#44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
guangwong pushed a commit to noslate-project/node that referenced this pull request Jan 3, 2023
@kvakil @guangwong
src: prevent copying ArrayBufferViewContents
3ab13b1 
It is error-prone to copy or heap-allocate `ArrayBufferViewContents`,
because you might accidentally cause it to exceed the lifetime of its
argument. Let's make it impossible to do so. Fortunately we were not
doing so anywhere already, so this diff is purely defensive.

Refs: nodejs/node#44079 (comment)
PR-URL: nodejs/node#44091
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Feng Yu <F3n67u@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tniessen tniessen tniessen left review comments

@mcollina mcollina mcollina approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@F3n67u F3n67u F3n67u approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kvakil kvakil

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@kvakil @nodejs-github-bot @mcollina @addaleax @tniessen @F3n67u @targos @aduh95