src: fix memory leaks and refactor ByteSource #43202

tniessen · 2022-05-24T23:01:55Z

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

nodejs-github-bot · 2022-05-24T23:01:59Z

Review requested:

@nodejs/crypto

nodejs-github-bot · 2022-05-27T13:24:46Z

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

tniessen · 2022-06-01T17:06:14Z

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

targos · 2022-06-02T08:05:39Z

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

tniessen · 2022-06-02T08:16:52Z

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.

tniessen · 2022-06-09T19:53:46Z

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

src/crypto/crypto_util.h

nodejs-github-bot · 2022-06-12T09:42:18Z

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

aduh95 · 2022-06-12T11:24:33Z

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: nodejs#43202 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

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>

Refs: nodejs#43202

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>

targos · 2022-07-21T05:30:26Z

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>

ByteSource::Allocated accepts a void pointer now, so we do not need to cast the argument to a char pointer. Refs: nodejs#43202 PR-URL: nodejs#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

src/crypto/crypto_util.h Show resolved Hide resolved

panva added the author ready PRs that have at least one approval, no pending requests for changes, 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 added a commit to tniessen/node that referenced this pull request Jun 18, 2022

crypto: use ByteSource::Builder in To*Copy

d14d479

Refs: nodejs#43202

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix memory leaks and refactor ByteSource #43202

src: fix memory leaks and refactor ByteSource #43202

tniessen commented May 24, 2022 •

edited

Loading

nodejs-github-bot commented May 24, 2022

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

tniessen commented Jun 1, 2022

targos commented Jun 2, 2022

tniessen commented Jun 2, 2022 •

edited

Loading

tniessen commented Jun 9, 2022

nodejs-github-bot commented Jun 12, 2022

aduh95 commented Jun 12, 2022

targos commented Jul 21, 2022

src: fix memory leaks and refactor ByteSource #43202

src: fix memory leaks and refactor ByteSource #43202

Conversation

tniessen commented May 24, 2022 • edited Loading

nodejs-github-bot commented May 24, 2022

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

tniessen commented Jun 1, 2022

targos commented Jun 2, 2022

tniessen commented Jun 2, 2022 • edited Loading

tniessen commented Jun 9, 2022

nodejs-github-bot commented Jun 12, 2022

aduh95 commented Jun 12, 2022

targos commented Jul 21, 2022

tniessen commented May 24, 2022 •

edited

Loading

tniessen commented Jun 2, 2022 •

edited

Loading