From 5e60ded244fb1651a76070d541de12d5b96d7719 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:03:46 +0200 Subject: [PATCH 01/10] src: check uv_async_init() return value Pointed out by Coverity. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node.cc | 6 +++--- src/node_win32_etw_provider.cc | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/node.cc b/src/node.cc index cc15ac272aaa00..eaf642b76a9975 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4096,9 +4096,9 @@ void Init(int* argc, // init async debug messages dispatching // Main thread uses uv_default_loop - uv_async_init(uv_default_loop(), - &dispatch_debug_messages_async, - DispatchDebugMessagesAsyncCallback); + CHECK_EQ(0, uv_async_init(uv_default_loop(), + &dispatch_debug_messages_async, + DispatchDebugMessagesAsyncCallback)); uv_unref(reinterpret_cast(&dispatch_debug_messages_async)); #if defined(NODE_V8_OPTIONS) diff --git a/src/node_win32_etw_provider.cc b/src/node_win32_etw_provider.cc index 3aa8578db029d3..7b766bd2bb99b8 100644 --- a/src/node_win32_etw_provider.cc +++ b/src/node_win32_etw_provider.cc @@ -155,9 +155,9 @@ void init_etw() { event_write = (EventWriteFunc)GetProcAddress(advapi, "EventWrite"); // create async object used to invoke main thread from callback - uv_async_init(uv_default_loop(), - &dispatch_etw_events_change_async, - etw_events_change_async); + CHECK_EQ(0, uv_async_init(uv_default_loop(), + &dispatch_etw_events_change_async, + etw_events_change_async)); uv_unref(reinterpret_cast(&dispatch_etw_events_change_async)); if (event_register) { From 88a87482123c2e9047a247ccb3007d6b0f1900bc Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:13:58 +0200 Subject: [PATCH 02/10] src: initialize encoding_ data member Pointed out by Coverity. Not really a bug because it's assigned before use but explicit assignment in the constructor is more obviously correct. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/fs_event_wrap.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index 10af967def48fd..cf9559df9d0c5a 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -34,6 +34,8 @@ class FSEventWrap: public HandleWrap { size_t self_size() const override { return sizeof(*this); } private: + static const encoding kDefaultEncoding = UTF8; + FSEventWrap(Environment* env, Local object); ~FSEventWrap() override; @@ -41,8 +43,8 @@ class FSEventWrap: public HandleWrap { int status); uv_fs_event_t handle_; - bool initialized_; - enum encoding encoding_; + bool initialized_ = false; + enum encoding encoding_ = kDefaultEncoding; }; @@ -50,9 +52,7 @@ FSEventWrap::FSEventWrap(Environment* env, Local object) : HandleWrap(env, object, reinterpret_cast(&handle_), - AsyncWrap::PROVIDER_FSEVENTWRAP) { - initialized_ = false; -} + AsyncWrap::PROVIDER_FSEVENTWRAP) {} FSEventWrap::~FSEventWrap() { @@ -101,7 +101,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { if (args[2]->IsTrue()) flags |= UV_FS_EVENT_RECURSIVE; - wrap->encoding_ = ParseEncoding(env->isolate(), args[3], UTF8); + wrap->encoding_ = ParseEncoding(env->isolate(), args[3], kDefaultEncoding); int err = uv_fs_event_init(wrap->env()->event_loop(), &wrap->handle_); if (err == 0) { From 57387fa29f130b5fae28b50c935e61665a37f9ff Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:17:03 +0200 Subject: [PATCH 03/10] src: guard against starting fs watcher twice This commit adds a CHECK that verifies that the file event watcher is not started twice, which would be indicative of a bug in lib/fs.js. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/fs_event_wrap.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/fs_event_wrap.cc b/src/fs_event_wrap.cc index cf9559df9d0c5a..2354a1be0cd57b 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -88,6 +88,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { FSEventWrap* wrap; ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder()); + CHECK_EQ(wrap->initialized_, false); static const char kErrMsg[] = "filename must be a string or Buffer"; if (args.Length() < 1) From 664ba510fd0fbfb4ac32fa90c0f6d377210b40f9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:35:01 +0200 Subject: [PATCH 04/10] src: remove unused data member write_queue_size_ Remove TLSWrap::write_queue_size_, it's not used anywhere. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/tls_wrap.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/tls_wrap.h b/src/tls_wrap.h index d608ddfcb5b37c..f390c9fe9281f7 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -150,7 +150,6 @@ class TLSWrap : public AsyncWrap, BIO* enc_out_; NodeBIO* clear_in_; size_t write_size_; - size_t write_queue_size_; typedef ListHead WriteItemList; WriteItemList write_item_queue_; WriteItemList pending_write_items_; From 0e8206933e1cfbbab1e7831c0c58d088308aa910 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:56:59 +0200 Subject: [PATCH 05/10] src: remove unused md_ data members The code assigned the result of EVP_get_digestbyname() to data members called md_ that were not used outside the initialization functions. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_crypto.cc | 34 +++++++++++++++++----------------- src/node_crypto.h | 6 ------ 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index def23744ef1292..d7dcd6936c4352 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3585,17 +3585,17 @@ void Hmac::New(const FunctionCallbackInfo& args) { void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) { HandleScope scope(env()->isolate()); - CHECK_EQ(md_, nullptr); - md_ = EVP_get_digestbyname(hash_type); - if (md_ == nullptr) { + CHECK_EQ(initialised_, false); + const EVP_MD* md = EVP_get_digestbyname(hash_type); + if (md == nullptr) { return env()->ThrowError("Unknown message digest"); } HMAC_CTX_init(&ctx_); int result = 0; if (key_len == 0) { - result = HMAC_Init(&ctx_, "", 0, md_); + result = HMAC_Init(&ctx_, "", 0, md); } else { - result = HMAC_Init(&ctx_, key, key_len, md_); + result = HMAC_Init(&ctx_, key, key_len, md); } if (!result) { return ThrowCryptoError(env(), ERR_get_error()); @@ -3730,12 +3730,12 @@ void Hash::New(const FunctionCallbackInfo& args) { bool Hash::HashInit(const char* hash_type) { - CHECK_EQ(md_, nullptr); - md_ = EVP_get_digestbyname(hash_type); - if (md_ == nullptr) + CHECK_EQ(initialised_, false); + const EVP_MD* md = EVP_get_digestbyname(hash_type); + if (md == nullptr) return false; EVP_MD_CTX_init(&mdctx_); - if (EVP_DigestInit_ex(&mdctx_, md_, nullptr) <= 0) { + if (EVP_DigestInit_ex(&mdctx_, md, nullptr) <= 0) { return false; } initialised_ = true; @@ -3881,13 +3881,13 @@ void Sign::New(const FunctionCallbackInfo& args) { SignBase::Error Sign::SignInit(const char* sign_type) { - CHECK_EQ(md_, nullptr); - md_ = EVP_get_digestbyname(sign_type); - if (!md_) + CHECK_EQ(initialised_, false); + const EVP_MD* md = EVP_get_digestbyname(sign_type); + if (md == nullptr) return kSignUnknownDigest; EVP_MD_CTX_init(&mdctx_); - if (!EVP_SignInit_ex(&mdctx_, md_, nullptr)) + if (!EVP_SignInit_ex(&mdctx_, md, nullptr)) return kSignInit; initialised_ = true; @@ -4087,13 +4087,13 @@ void Verify::New(const FunctionCallbackInfo& args) { SignBase::Error Verify::VerifyInit(const char* verify_type) { - CHECK_EQ(md_, nullptr); - md_ = EVP_get_digestbyname(verify_type); - if (md_ == nullptr) + CHECK_EQ(initialised_, false); + const EVP_MD* md = EVP_get_digestbyname(verify_type); + if (md == nullptr) return kSignUnknownDigest; EVP_MD_CTX_init(&mdctx_); - if (!EVP_VerifyInit_ex(&mdctx_, md_, nullptr)) + if (!EVP_VerifyInit_ex(&mdctx_, md, nullptr)) return kSignInit; initialised_ = true; diff --git a/src/node_crypto.h b/src/node_crypto.h index e0d01887ac9db2..24ac77365cf455 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -500,14 +500,12 @@ class Hmac : public BaseObject { Hmac(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - md_(nullptr), initialised_(false) { MakeWeak(this); } private: HMAC_CTX ctx_; /* coverity[member_decl] */ - const EVP_MD* md_; /* coverity[member_decl] */ bool initialised_; }; @@ -531,14 +529,12 @@ class Hash : public BaseObject { Hash(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - md_(nullptr), initialised_(false) { MakeWeak(this); } private: EVP_MD_CTX mdctx_; /* coverity[member_decl] */ - const EVP_MD* md_; /* coverity[member_decl] */ bool initialised_; bool finalized_; }; @@ -557,7 +553,6 @@ class SignBase : public BaseObject { SignBase(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - md_(nullptr), initialised_(false) { } @@ -571,7 +566,6 @@ class SignBase : public BaseObject { void CheckThrow(Error error); EVP_MD_CTX mdctx_; /* coverity[member_decl] */ - const EVP_MD* md_; /* coverity[member_decl] */ bool initialised_; }; From d404566687ed107e0181af4599c8fbc35c55cf3d Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:07:56 +0200 Subject: [PATCH 06/10] src: remove duplicate HMAC_Init calls PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_crypto.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index d7dcd6936c4352..eda62489a76107 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3591,13 +3591,10 @@ void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) { return env()->ThrowError("Unknown message digest"); } HMAC_CTX_init(&ctx_); - int result = 0; if (key_len == 0) { - result = HMAC_Init(&ctx_, "", 0, md); - } else { - result = HMAC_Init(&ctx_, key, key_len, md); + key = ""; } - if (!result) { + if (!HMAC_Init(&ctx_, key, key_len, md)) { return ThrowCryptoError(env(), ERR_get_error()); } initialised_ = true; From 26a918f4f5bc56a246ab07db20f871c07a29a887 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:08:25 +0200 Subject: [PATCH 07/10] src: remove deprecated HMAC_Init, use HMAC_Init_ex PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_crypto.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index eda62489a76107..a7964cde94ac54 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3594,7 +3594,7 @@ void Hmac::HmacInit(const char* hash_type, const char* key, int key_len) { if (key_len == 0) { key = ""; } - if (!HMAC_Init(&ctx_, key, key_len, md)) { + if (!HMAC_Init_ex(&ctx_, key, key_len, md, nullptr)) { return ThrowCryptoError(env(), ERR_get_error()); } initialised_ = true; From da4c1c314d0b933810c03b368a9a3379b82481d3 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:32:56 +0200 Subject: [PATCH 08/10] src: fix use-after-return in zlib bindings Pointed out by Coverity. Introduced in commit 5b8e1dab from September 2011 ("Initial pass at zlib bindings".) The asynchronous version of Write() used a pointer to a stack-allocated buffer on flush. A mitigating factor is that zlib does not dereference the pointer for zero-sized writes but it's still technically UB. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_zlib.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/node_zlib.cc b/src/node_zlib.cc index 7ce4a1e361f7ae..2c583298d99045 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -149,8 +149,7 @@ class ZCtx : public AsyncWrap { if (args[1]->IsNull()) { // just a flush - Bytef nada[1] = { 0 }; - in = nada; + in = nullptr; in_len = 0; in_off = 0; } else { From c50e19220453039b08065f50eb2cd1713c402c4e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:56:18 +0200 Subject: [PATCH 09/10] src: fix memory leak in WriteBuffers() error path Pointed out by Coverity. Introduced in commit 05d30d53 from July 2015 ("fs: implemented WriteStream#writev"). WriteBuffers() leaked memory in the synchronous uv_fs_write() error path when trying to write > 1024 buffers. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_file.cc | 25 +++++-------------------- src/util.h | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/node_file.cc b/src/node_file.cc index 089ead82ea430c..968284788a4b72 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1079,38 +1079,23 @@ static void WriteBuffers(const FunctionCallbackInfo& args) { int64_t pos = GET_OFFSET(args[2]); Local req = args[3]; - uint32_t chunkCount = chunks->Length(); + MaybeStackBuffer iovs(chunks->Length()); - uv_buf_t s_iovs[1024]; // use stack allocation when possible - uv_buf_t* iovs; - - if (chunkCount > arraysize(s_iovs)) - iovs = new uv_buf_t[chunkCount]; - else - iovs = s_iovs; - - for (uint32_t i = 0; i < chunkCount; i++) { + for (uint32_t i = 0; i < iovs.length(); i++) { Local chunk = chunks->Get(i); - if (!Buffer::HasInstance(chunk)) { - if (iovs != s_iovs) - delete[] iovs; + if (!Buffer::HasInstance(chunk)) return env->ThrowTypeError("Array elements all need to be buffers"); - } iovs[i] = uv_buf_init(Buffer::Data(chunk), Buffer::Length(chunk)); } if (req->IsObject()) { - ASYNC_CALL(write, req, UTF8, fd, iovs, chunkCount, pos) - if (iovs != s_iovs) - delete[] iovs; + ASYNC_CALL(write, req, UTF8, fd, *iovs, iovs.length(), pos) return; } - SYNC_CALL(write, nullptr, fd, iovs, chunkCount, pos) - if (iovs != s_iovs) - delete[] iovs; + SYNC_CALL(write, nullptr, fd, *iovs, iovs.length(), pos) args.GetReturnValue().Set(SYNC_RESULT); } diff --git a/src/util.h b/src/util.h index 0a6b5e0dbef9c5..1570c57f195c66 100644 --- a/src/util.h +++ b/src/util.h @@ -231,6 +231,16 @@ class MaybeStackBuffer { return buf_; } + T& operator[](size_t index) { + CHECK_LT(index, length()); + return buf_[index]; + } + + const T& operator[](size_t index) const { + CHECK_LT(index, length()); + return buf_[index]; + } + size_t length() const { return length_; } @@ -282,6 +292,10 @@ class MaybeStackBuffer { buf_[0] = T(); } + explicit MaybeStackBuffer(size_t storage) : MaybeStackBuffer() { + AllocateSufficientStorage(storage); + } + ~MaybeStackBuffer() { if (buf_ != buf_st_) free(buf_); From ed3d8b13ee9a705d89f9e0397d9e96519e7e47ac Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 21:57:13 +0200 Subject: [PATCH 10/10] src: fix bad logic in uid/gid checks Pointed out by Coverity. Introduced in commits 3546383c ("process_wrap: avoid leaking memory when throwing due to invalid arguments") and fa4eb47c ("bindings: add spawn_sync bindings"). The return statements inside the if blocks were dead code because their guard conditions always evaluated to false. Remove them. PR-URL: https://github.com/nodejs/node/pull/7374 Reviewed-By: Anna Henningsen Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/process_wrap.cc | 14 ++++---------- src/spawn_sync.cc | 33 +++++++-------------------------- src/spawn_sync.h | 1 - 3 files changed, 11 insertions(+), 37 deletions(-) diff --git a/src/process_wrap.cc b/src/process_wrap.cc index 2b214d1d47a8bd..d574bf22965407 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -123,12 +123,9 @@ class ProcessWrap : public HandleWrap { // options.uid Local uid_v = js_options->Get(env->uid_string()); if (uid_v->IsInt32()) { - int32_t uid = uid_v->Int32Value(); - if (uid & ~((uv_uid_t) ~0)) { - return env->ThrowRangeError("options.uid is out of range"); - } + const int32_t uid = uid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETUID; - options.uid = (uv_uid_t) uid; + options.uid = static_cast(uid); } else if (!uid_v->IsUndefined() && !uid_v->IsNull()) { return env->ThrowTypeError("options.uid should be a number"); } @@ -136,12 +133,9 @@ class ProcessWrap : public HandleWrap { // options.gid Local gid_v = js_options->Get(env->gid_string()); if (gid_v->IsInt32()) { - int32_t gid = gid_v->Int32Value(); - if (gid & ~((uv_gid_t) ~0)) { - return env->ThrowRangeError("options.gid is out of range"); - } + const int32_t gid = gid_v->Int32Value(env->context()).FromJust(); options.flags |= UV_PROCESS_SETGID; - options.gid = (uv_gid_t) gid; + options.gid = static_cast(gid); } else if (!gid_v->IsUndefined() && !gid_v->IsNull()) { return env->ThrowTypeError("options.gid should be a number"); } diff --git a/src/spawn_sync.cc b/src/spawn_sync.cc index 2d6012761c8a97..79f10a0ea2594d 100644 --- a/src/spawn_sync.cc +++ b/src/spawn_sync.cc @@ -729,17 +729,19 @@ int SyncProcessRunner::ParseOptions(Local js_value) { } Local js_uid = js_options->Get(env()->uid_string()); if (IsSet(js_uid)) { - if (!CheckRange(js_uid)) + if (!js_uid->IsInt32()) return UV_EINVAL; - uv_process_options_.uid = static_cast(js_uid->Int32Value()); + const int32_t uid = js_uid->Int32Value(env()->context()).FromJust(); + uv_process_options_.uid = static_cast(uid); uv_process_options_.flags |= UV_PROCESS_SETUID; } Local js_gid = js_options->Get(env()->gid_string()); if (IsSet(js_gid)) { - if (!CheckRange(js_gid)) + if (!js_gid->IsInt32()) return UV_EINVAL; - uv_process_options_.gid = static_cast(js_gid->Int32Value()); + const int32_t gid = js_gid->Int32Value(env()->context()).FromJust(); + uv_process_options_.gid = static_cast(gid); uv_process_options_.flags |= UV_PROCESS_SETGID; } @@ -763,7 +765,7 @@ int SyncProcessRunner::ParseOptions(Local js_value) { Local js_max_buffer = js_options->Get(env()->max_buffer_string()); if (IsSet(js_max_buffer)) { - if (!CheckRange(js_max_buffer)) + if (!js_max_buffer->IsUint32()) return UV_EINVAL; max_buffer_ = js_max_buffer->Uint32Value(); } @@ -915,27 +917,6 @@ bool SyncProcessRunner::IsSet(Local value) { } -template -bool SyncProcessRunner::CheckRange(Local js_value) { - if ((t) -1 > 0) { - // Unsigned range check. - if (!js_value->IsUint32()) - return false; - if (js_value->Uint32Value() & ~((t) ~0)) - return false; - - } else { - // Signed range check. - if (!js_value->IsInt32()) - return false; - if (js_value->Int32Value() & ~((t) ~0)) - return false; - } - - return true; -} - - int SyncProcessRunner::CopyJsString(Local js_value, const char** target) { Isolate* isolate = env()->isolate(); diff --git a/src/spawn_sync.h b/src/spawn_sync.h index 8ddba479f31e1b..8f4c05aa5f4768 100644 --- a/src/spawn_sync.h +++ b/src/spawn_sync.h @@ -175,7 +175,6 @@ class SyncProcessRunner { inline int AddStdioInheritFD(uint32_t child_fd, int inherit_fd); static bool IsSet(Local value); - template static bool CheckRange(Local js_value); int CopyJsString(Local js_value, const char** target); int CopyJsStringArray(Local js_value, char** target);