Skip to content

src: use automatic memory mgmt in SecretKeyGen - #44479

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:src-avoid-manual-memory-management-secretkeygen
Sep 5, 2022
Merged

src: use automatic memory mgmt in SecretKeyGen#44479
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
tniessen:src-avoid-manual-memory-management-secretkeygen

Conversation

@tniessen

@tniessentniessen commented Sep 1, 2022

Copy link
Copy Markdown
Member

Avoid manual memory management (i.e., calling MallocOpenSSL). This leaves less room for memory leaks and other bugs.

The import bit here is using ByteSource::Builder (see #43202).

@tniessen
src: use automatic memory mgmt in SecretKeyGen
8d19d39
Avoid manual memory management (i.e., calling MallocOpenSSL). This
leaves less room for memory leaks and other bugs.
@tniessentniessen added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 1, 2022
@nodejs-github-bot

nodejs-github-bot commented Sep 1, 2022

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Sep 1, 2022
@tniessentniessen added review wanted PRs that need reviews. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 4, 2022
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2022
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

nodejs-github-bot commented Sep 4, 2022

Copy link
Copy Markdown
Collaborator

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

@github-actionsgithub-actionsBot mentioned this pull request Sep 5, 2022
Open
17 tasks
@tniessen

tniessen commented Sep 5, 2022

Copy link
Copy Markdown
MemberAuthor

cc @nodejs/cpp-reviewers

@tniessentniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2022
@tniessentniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2022
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 1f54fc2 into nodejs:mainSep 5, 2022
@nodejs-github-bot

nodejs-github-bot commented Sep 5, 2022

Copy link
Copy Markdown
Collaborator

Landed in 1f54fc2

This was referenced Sep 6, 2022
Open
Open
Open
Open
Open
Open
@RafaelGSS

RafaelGSS commented Sep 26, 2022

Copy link
Copy Markdown
Member

It didn't land cleanly on v18.x. This PR conflicts with a security change in the same method (

node/src/crypto/crypto_keygen.cc

Line 83 in 0c2a572

params->out = MallocOpenSSL<char>(params->length);
).

@RafaelGSSRafaelGSS mentioned this pull request Oct 14, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@addaleaxaddaleaxaddaleax approved these changes

Assignees

No one assigned

Labels

author readyPRs 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++.cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tniessen@nodejs-github-bot@RafaelGSS@bnoordhuis@addaleax@targos