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: more automatic memory management in node_crypto.cc #20238

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of gotos.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@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. labels Apr 23, 2018
@addaleax
Copy link
Member Author

@addaleax addaleax added crypto Issues and PRs related to the crypto subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 23, 2018
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Thanks Anna, this looks great! I will have a closer look soon.

@@ -586,24 +595,26 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
// by SSL_CTX_use_certificate).

// Find issuer
if (*issuer != nullptr || X509_check_issued(ca, x) != X509_V_OK)
if (!*issuer || X509_check_issued(ca, x.get()) != X509_V_OK)
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something here, but doesn't this negate the check?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tniessen Yes, good catch. I don’t think we have tests for this at the moment. :/

@addaleax
Copy link
Member Author

addaleax commented Apr 23, 2018

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Left some comments, didn't review fully. I like the overall direction.

int iter,
int keylen)
MallocedBuffer<char>&& pass,
MallocedBuffer<char>&& salt,
Copy link
Member

Choose a reason for hiding this comment

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

Coincidence: I was working yesterday on simplifying this to something that uses std::vector<char>.

(Working on adding scrypt support and DRYing the PBKDF2 and RandomBytes code in the process.)

@@ -73,6 +73,32 @@ struct MarkPopErrorOnReturn {
~MarkPopErrorOnReturn() { ERR_pop_to_mark(); }
};

template<typename T, void (*function)(T*)>
Copy link
Member

Choose a reason for hiding this comment

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

Style: space before <.

template<typename T, void (*function)(T*)>
struct FunctionDeleter {
void operator()(T* pointer) const { function(pointer); }
typedef std::unique_ptr<T, FunctionDeleter> ptr;
Copy link
Member

Choose a reason for hiding this comment

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

Can you name this Ptr, Pointer, UniquePointer or whatever to make it clearer it's a type?

return ret;
DeleteFnPtr<X509_STORE_CTX, X509_STORE_CTX_free> store_ctx(
X509_STORE_CTX_new());
return store_ctx != nullptr &&
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this works because of boolean coercion but it might be clearer to write it as !!store_ctx or store_ctx == true or store_ctx.get() != nullptr.

@addaleax
Copy link
Member Author

Fixed tests (not sure why I didn’t see the failures before) and addressed @bnoordhuis’ first round of review nits.

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

Error Init(const char* sign_type);
Error Update(const char* data, int len);

protected:
void CheckThrow(Error error);

EVP_MD_CTX* mdctx_;
DeleteFnPtr<EVP_MD_CTX, EVP_MD_CTX_free> mdctx_;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to reuse EVPMDPointer?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tniessen thanks for catching, updated!

@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/18082/

Would anybody in particular like me to wait with landing until they have time to review? I know it’s a large commit, and not urgent, so I really don’t want to push this to land early.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Rubber stamp LGTM

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Wrong button..

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Needs a rebase (I'm guessing it conflicts with the EVP_CTRL_AEAD_SET_IVLEN change) but LGTM modulo comments. This PR makes the crypto code a lot nicer!

@@ -1125,6 +1110,9 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {


#ifndef OPENSSL_NO_ENGINE
// Helper for the smart pointer.
void ENGINE_free_fn(ENGINE* engine) { ENGINE_free_fn(engine); }
Copy link
Member

Choose a reason for hiding this comment

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

s/ENGINE_free_fn/ENGINE_free/ in the function body.

const unsigned char* p = reinterpret_cast<const unsigned char*>(sbuf);
SSL_SESSION* sess = d2i_SSL_SESSION(nullptr, &p, slen);
std::vector<char> sbuf(slen);
memcpy(sbuf.data(), Buffer::Data(args[0]), slen);
Copy link
Member

Choose a reason for hiding this comment

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

Just a suggestion since this isn't a functional change but it's correcter to use assignment here, like this:

std::vector<char> sbuf;
if (char* p = Buffer::Data(args[0])) sbuf.assign(p, p + slen);

The reason is that Buffer::Data() can return nullptr when slen == 0 and passing a nullptr to memcpy() is technically UB, even when the pointer isn't dereferenced.

ctx_ = nullptr;
ctx_.reset(HMAC_CTX_new());
if (!ctx_ ||
!HMAC_Init_ex(ctx_.get(), key, key_len, md, nullptr)) {
Copy link
Member

Choose a reason for hiding this comment

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

Fits on one line now. (Likewise on lines 3289 and 3378, I think.)

Buffer::Length(args[0]),
0));

int dataSize = DH_size(diffieHellman->dh_.get());
char* data = Malloc(dataSize);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use automatic memory management for this as well (and rename dataSize to data_size while you're here)? Just a suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea – I’ve switched this to MallocedBuffer, too (that takes care of dataSize anyway)

int r;

pub = EC_POINT_new(group);
ECPointPointer pub(EC_POINT_new(group));
if (pub == nullptr) {
Copy link
Member

Choose a reason for hiding this comment

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

!pub, also on line 4366.

@@ -5320,33 +5136,32 @@ void ConvertKey(const FunctionCallbackInfo<Value>& args) {
if (nid == NID_undef)
return env->ThrowTypeError("Invalid ECDH curve name");

EC_GROUP* group = EC_GROUP_new_by_curve_name(nid);
ECGroupPointer group(
EC_GROUP_new_by_curve_name(nid));
Copy link
Member

Choose a reason for hiding this comment

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

4 space indent.

src/util.h Outdated
}
MallocedBuffer& operator=(MallocedBuffer&& other) {
this->~MallocedBuffer();
return *new (this)MallocedBuffer(other);
Copy link
Member

Choose a reason for hiding this comment

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

Style: new(this)

Don't know if it matters here but use of placement new sometimes inhibits compiler optimizations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't know if it matters here but use of placement new sometimes inhibits compiler optimizations.

I don’t think we actually use it at this point, so this is just here for completeness

MallocedBuffer& operator=(const MallocedBuffer&) = delete;
};

} // namespace node
Copy link
Member

Choose a reason for hiding this comment

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

Is this class necessary? A std::unique_ptr with a custom deleter would be sufficient too, wouldn't it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Yes, the difference being that this also tracks the length of the buffer by itself. If you feel strongly about it I can switch to splitting up into two variables again

Copy link
Member

Choose a reason for hiding this comment

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

No strong feelings.

@addaleax
Copy link
Member Author

addaleax commented May 1, 2018

@tniessen
Copy link
Member

tniessen commented May 2, 2018

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 3, 2018
@addaleax
Copy link
Member Author

addaleax commented May 3, 2018

Rebased

CI before landing: https://ci.nodejs.org/job/node-test-commit/18218/

@addaleax
Copy link
Member Author

addaleax commented May 4, 2018

Another day, another merge conflict, another rebase. :)

CI: https://ci.nodejs.org/job/node-test-commit/18232/
Edit: AIX re-run: https://ci.nodejs.org/job/node-test-commit-aix/14729/
Edit²: Fixed compiler warnings, CI: https://ci.nodejs.org/job/node-test-commit/18234/

addaleax added a commit to addaleax/node that referenced this pull request May 8, 2018
Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of `goto`s.

PR-URL: nodejs#20238
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 8, 2018
Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of `goto`s.

Backport-PR-URL: #20609
PR-URL: #20238
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
tniessen pushed a commit to tniessen/node that referenced this pull request May 13, 2018
Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of `goto`s.

PR-URL: nodejs#20238
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request May 14, 2018
Prefer custom smart pointers fitted to the OpenSSL data structures
over more manual memory management and lots of `goto`s.

Backport-PR-URL: #20706
PR-URL: #20238
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request May 14, 2018
@tniessen tniessen mentioned this pull request May 15, 2018
4 tasks
@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants