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

src: fix memory leaks and refactor ByteSource #43202

Merged

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented May 24, 2022

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

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++. 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 tniessen added the crypto Issues and PRs related to the crypto subsystem. label May 24, 2022
@tniessen 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 tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

tniessen commented Jun 1, 2022

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

@targos
Copy link
Member

targos commented Jun 2, 2022

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

@tniessen
Copy link
Member Author

tniessen commented Jun 2, 2022

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

tniessen commented Jun 9, 2022

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

src/crypto/crypto_util.h Show resolved Hide resolved
@panva 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
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 12, 2022
@nodejs-github-bot 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
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jun 12, 2022
@aduh95 aduh95 merged commit 167ea80 into nodejs:master Jun 12, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jun 12, 2022

Landed in 167ea80

italojs pushed a commit to italojs/node that referenced this pull request Jun 12, 2022
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>
danielleadams pushed a commit that referenced this pull request Jun 13, 2022
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>
@danielleadams danielleadams mentioned this pull request Jun 13, 2022
danielleadams pushed a commit that referenced this pull request Jun 13, 2022
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>
tniessen added a commit to tniessen/node that referenced this pull request Jun 18, 2022
tniessen added a commit to tniessen/node that referenced this pull request Jun 18, 2022
Avoid manual calls to MallocOpenSSL in ArrayBufferOrViewContents and
use the new ByteSource::Builder instead.

Refs: nodejs#43202
nodejs-github-bot pushed a commit that referenced this pull request Jun 25, 2022
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 pushed a commit that referenced this pull request Jul 12, 2022
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
Copy link
Member

targos commented Jul 21, 2022

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

tniessen added a commit to tniessen/node that referenced this pull request Jul 26, 2022
ByteSource::Allocated accepts a void pointer now, so we do not need to
cast the argument to a char pointer.

Refs: nodejs#43202
nodejs-github-bot pushed a commit that referenced this pull request Jul 27, 2022
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>
danielleadams pushed a commit that referenced this pull request Aug 16, 2022
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>
ruyadorno pushed a commit that referenced this pull request Aug 23, 2022
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>
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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++. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants