From 785a100caee6537d34a165892353ffb670cb5cf4 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:03:46 +0200 Subject: [PATCH 1/8] 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 12219b026d63b7..5eafaee23f73cf 100644 --- a/src/node.cc +++ b/src/node.cc @@ -3858,9 +3858,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 6877f1977dae8b..f2650af93a51d6 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 c8e8f79e3c07bd413a223567dfceda7aac1f8a53 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:17:03 +0200 Subject: [PATCH 2/8] 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 a9f96389121c9e..cd6dc417ba2460 100644 --- a/src/fs_event_wrap.cc +++ b/src/fs_event_wrap.cc @@ -85,6 +85,7 @@ void FSEventWrap::Start(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); FSEventWrap* wrap = Unwrap(args.Holder()); + CHECK_EQ(wrap->initialized_, false); if (args.Length() < 1 || !args[0]->IsString()) { return env->ThrowTypeError("filename must be a valid string"); From a3ba357a94a8098fa15cf6881c4051e089d93317 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:35:01 +0200 Subject: [PATCH 3/8] 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 471a92056dd848..8346613b199253 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -148,7 +148,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 31a1016d1f3366a3c1399bf7ce3c3fb1186d9c4f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 13:56:59 +0200 Subject: [PATCH 4/8] 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 21e4d33cab9316..762913ba731821 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3324,17 +3324,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()); @@ -3465,12 +3465,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; @@ -3603,13 +3603,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; @@ -3803,13 +3803,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 cb94650e0735d0..d9105fd11a5721 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -495,14 +495,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_; }; @@ -526,14 +524,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_; }; @@ -551,7 +547,6 @@ class SignBase : public BaseObject { SignBase(Environment* env, v8::Local wrap) : BaseObject(env, wrap), - md_(nullptr), initialised_(false) { } @@ -565,7 +560,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 d378b8fbe89141b644248c8c7db39f4902ae12d1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:07:56 +0200 Subject: [PATCH 5/8] 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 762913ba731821..5a9ffa16c4f0ed 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3330,13 +3330,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 2ec930790e0803357f8c21e755e9c0bc64c46ce5 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:08:25 +0200 Subject: [PATCH 6/8] 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 5a9ffa16c4f0ed..42f023b386d4c1 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -3333,7 +3333,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 b9c7d59e8f7d202af0f4a2b7e50bef702bbac9b0 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 14:32:56 +0200 Subject: [PATCH 7/8] 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 29649e32c7dbbc..15f58843983c8f 100644 --- a/src/node_zlib.cc +++ b/src/node_zlib.cc @@ -145,8 +145,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 ea053f6460fdfeede95eaae6fe6161c38f2861ad Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 22 Jun 2016 21:57:13 +0200 Subject: [PATCH 8/8] 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 420c71d7ea4052..3b98b4a3771b3b 100644 --- a/src/process_wrap.cc +++ b/src/process_wrap.cc @@ -120,12 +120,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"); } @@ -133,12 +130,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 62fadb4396ce19..4ff70b48ca9618 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 fab2f09eefea69..1f9fc68809a9dc 100644 --- a/src/spawn_sync.h +++ b/src/spawn_sync.h @@ -173,7 +173,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);