src: fix memory leaks and refactor ByteSource by tniessen · Pull Request #43202 · nodejs/node · GitHub

Add ByteSource::Builder to replace the common MallocOpenSSL() + ByteSource::Allocated() pattern.

Remove ByteSource::reset() that is unused.

Remove ByteSource::Resize() to make ByteSource truly immutable (until moved away). Instead, ByteSource::Builder::release() takes an optional size argument that truncates the resulting ByteSource.

Fix occurrences of MallocOpenSSL() that do not always free the allocated memory by using the new ByteSource::Builder class instead.

Remove ByteSource::get() and replace uses with ByteSource::data().

Remove ReallocOpenSSL() because it likely only saves us a few bytes whenever we use it.

This does save a few dozen lines of code, too. (Much of the diff is just to make the linter happy, which apparently requires changed lines to be formatted, and that increases the diff by about 50 % in this case.)

Review requested:

@nodejs/crypto

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

This has been open for more than a week with no comments... ping @nodejs/crypto.

Do we have a team to ping for C++ reviews?

Do we have a team to ping for C++ reviews?

I don't think we do. I suggested this in #43286 (comment) (but I guess a fast-tracked PR wasn't a good place to mention it).

Edit: opened nodejs/TSC#1237.

Second edit: created @nodejs/cpp-reviewers.

This has been open for more than two weeks with no comments... ping @nodejs/cpp-reviewers @nodejs/crypto.

Commit Queue failed

- Loading data for nodejs/node/pull/43202
✔  Done loading data for nodejs/node/pull/43202
----------------------------------- PR info ------------------------------------
Title      src: fix memory leaks and refactor ByteSource (#43202)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     tniessen:src-memory-leaks-and-refactor-bytesource -> nodejs:master
Labels     crypto, c++, lib / src, author ready, needs-ci
Commits    1
 - src: fix memory leaks and refactor ByteSource
Committers 1
 - Tobias Nießen 
PR-URL: https://github.com/nodejs/node/pull/43202
Reviewed-By: Anna Henningsen 
Reviewed-By: Darshan Sen 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/43202
Reviewed-By: Anna Henningsen 
Reviewed-By: Darshan Sen 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 24 May 2022 23:01:55 GMT
   ✔  Approvals: 3
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/43202#pullrequestreview-1002220635
   ✔  - Darshan Sen (@RaisinTen) (TSC): https://github.com/nodejs/node/pull/43202#pullrequestreview-1002898142
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/43202#pullrequestreview-1003512265
   ✖  Last GitHub CI failed
   ℹ  Last Full PR CI on 2022-06-12T05:35:56Z: https://ci.nodejs.org/job/node-test-pull-request/44453/
- Querying data for job/node-test-pull-request/44453/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/2483034882

Landed in 167ea80

Add ByteSource::Builder to replace the common MallocOpenSSL() + ByteSource::Allocated() pattern. Remove ByteSource::reset() that is unused. Remove ByteSource::Resize() to make ByteSource truly immutable (until moved away). Instead, ByteSource::Builder::release() takes an optional size argument that truncates the resulting ByteSource. Fix occurrences of MallocOpenSSL() that do not always free the allocated memory by using the new ByteSource::Builder class instead. Remove ByteSource::get() and replace uses with ByteSource::data(). Remove ReallocOpenSSL() because it likely only saves us a few bytes whenever we use it. PR-URL: #43202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Avoid manual calls to MallocOpenSSL in ArrayBufferOrViewContents and use the new ByteSource::Builder instead. Refs: nodejs#43202

Avoid manual calls to MallocOpenSSL in ArrayBufferOrViewContents and use the new ByteSource::Builder instead. Refs: #43202 PR-URL: #43477 Reviewed-By: Darshan Sen <raisinten@gmail.com>

This would need a backport to land on v16.x because std::optional and std::nullopt are not available on that branch.

ByteSource::Allocated accepts a void pointer now, so we do not need to cast the argument to a char pointer. Refs: nodejs#43202

ByteSource::Allocated accepts a void pointer now, so we do not need to cast the argument to a char pointer. Refs: #43202 PR-URL: #44001 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Feng Yu <F3n67u@outlook.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 24, 2022

tniessen added the crypto Issues and PRs related to the crypto subsystem. label May 24, 2022

tniessen force-pushed the src-memory-leaks-and-refactor-bytesource branch 5 times, most recently from 45a1378 to 2b6e52e Compare May 25, 2022 22:00

tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2022

This comment was marked as outdated.

Sign in to view

tniessen added the review wanted PRs that need reviews. label May 27, 2022

tniessen mentioned this pull request Jun 2, 2022

Create GitHub team for reviewing src (C++) changes nodejs/TSC#1237

Closed

addaleax approved these changes Jun 10, 2022

View reviewed changes

Comment thread src/crypto/crypto_util.h Outdated

panva added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 10, 2022

RaisinTen approved these changes Jun 10, 2022

View reviewed changes

aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 12, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 12, 2022

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 12, 2022

aduh95 merged commit 167ea80 into nodejs:master Jun 12, 2022

danielleadams mentioned this pull request Jun 13, 2022

v18.4.0 proposal #43385

Merged

github-actions Bot mentioned this pull request Jun 13, 2022

CI Reliability 2022-06-13 nodejs/reliability#299

Open

15 tasks

github-actions Bot mentioned this pull request Jun 14, 2022

CI Reliability 2022-06-14 nodejs/reliability#300

Open

16 tasks

tniessen mentioned this pull request Jun 18, 2022

crypto: use ByteSource::Builder in To*Copy #43477

Merged

tniessen added a commit to tniessen/node that referenced this pull request Jun 18, 2022

crypto: use ByteSource::Builder in To*Copy

0e6123d

Avoid manual calls to MallocOpenSSL in ArrayBufferOrViewContents and use the new ByteSource::Builder instead. Refs: nodejs#43202

targos added the backport-requested-v16.x label Jul 21, 2022

tniessen added a commit to tniessen/node that referenced this pull request Jul 26, 2022

src: remove unnecessary cast in crypto_sig.cc

d9aaf23

ByteSource::Allocated accepts a void pointer now, so we do not need to cast the argument to a char pointer. Refs: nodejs#43202

tniessen mentioned this pull request Jul 26, 2022

src: remove unnecessary cast in crypto_sig.cc #44001

Merged

tniessen mentioned this pull request Sep 1, 2022

src: use automatic memory mgmt in SecretKeyGen #44479

Merged

targos added dont-land-on-v16.x and removed backport-requested-v16.x labels Sep 16, 2022

panva mentioned this pull request Apr 1, 2023

[v16.x backport] WebCryptoAPI fixes #47336

Closed

panva mentioned this pull request May 31, 2023

Perf regression on crypto.verify nodejs/performance#72

Closed

src: fix memory leaks and refactor ByteSource by tniessen · Pull Request #43202 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix memory leaks and refactor ByteSource - #43202

src: fix memory leaks and refactor ByteSource#43202
aduh95 merged 1 commit into
nodejs:masterfrom
tniessen:src-memory-leaks-and-refactor-bytesource

tniessen commented May 24, 2022 •
edited
Loading

Uh oh!

Uh oh!

nodejs-github-bot commented May 24, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 27, 2022

Uh oh!

tniessen commented Jun 1, 2022

Uh oh!

targos commented Jun 2, 2022

Uh oh!

tniessen commented Jun 2, 2022 •
edited
Loading

Uh oh!

Uh oh!

tniessen commented Jun 9, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 12, 2022

Uh oh!

aduh95 commented Jun 12, 2022

Uh oh!

targos commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: fix memory leaks and refactor ByteSource by tniessen · Pull Request #43202 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: fix memory leaks and refactor ByteSource - #43202

src: fix memory leaks and refactor ByteSource#43202aduh95 merged 1 commit intonodejs:masternodejs/node:masterfrom tniessen:src-memory-leaks-and-refactor-bytesourcetniessen/node:src-memory-leaks-and-refactor-bytesourceCopy head branch name to clipboard

Conversation

tniessen commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 24, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

nodejs-github-bot commented May 27, 2022

Uh oh!

tniessen commented Jun 1, 2022

Uh oh!

targos commented Jun 2, 2022

Uh oh!

tniessen commented Jun 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tniessen commented Jun 9, 2022

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 12, 2022

Uh oh!

aduh95 commented Jun 12, 2022

Uh oh!

targos commented Jul 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: fix memory leaks and refactor ByteSource#43202
aduh95 merged 1 commit into
nodejs:masterfrom
tniessen:src-memory-leaks-and-refactor-bytesource

tniessen commented May 24, 2022 •
edited
Loading

tniessen commented Jun 2, 2022 •
edited
Loading