From c0a92899a8b90271d92241118c1dd1fd65acf840 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 12 Feb 2021 20:57:53 +0100 Subject: [PATCH 1/4] src: avoid implicit type conversions (take 2) This fixes more C4244 MSVC warnings in the code base. Refs: https://github.com/nodejs/node/pull/37149 --- src/base64-inl.h | 34 ++++++++++++++----------------- src/crypto/crypto_dsa.cc | 20 ++++++++++-------- src/crypto/crypto_keygen.cc | 3 ++- src/crypto/crypto_keys.cc | 7 +++---- src/crypto/crypto_rsa.cc | 10 +++++---- src/inspector/worker_inspector.cc | 27 +++++++++++++----------- src/inspector/worker_inspector.h | 15 +++++++------- src/inspector_agent.cc | 4 ++-- src/inspector_agent.h | 2 +- src/node_serdes.cc | 2 +- src/node_url.cc | 9 ++++---- src/node_wasi.cc | 6 ++++-- src/signal_wrap.cc | 2 +- src/stream_base.cc | 2 +- src/string_bytes.cc | 4 ++-- src/udp_wrap.cc | 11 +++++----- 16 files changed, 83 insertions(+), 75 deletions(-) diff --git a/src/base64-inl.h b/src/base64-inl.h index 650218a0481085..1046fd83b9d417 100644 --- a/src/base64-inl.h +++ b/src/base64-inl.h @@ -30,21 +30,17 @@ bool base64_decode_group_slow(char* const dst, const size_t dstlen, size_t* const i, size_t* const k) { uint8_t hi; uint8_t lo; -#define V(expr) \ - for (;;) { \ - const uint8_t c = src[*i]; \ - lo = unbase64(c); \ - *i += 1; \ - if (lo < 64) \ - break; /* Legal character. */ \ - if (c == '=' || *i >= srclen) \ - return false; /* Stop decoding. */ \ - } \ - expr; \ - if (*i >= srclen) \ - return false; \ - if (*k >= dstlen) \ - return false; \ +#define V(expr) \ + for (;;) { \ + const uint8_t c = static_cast(src[*i]); \ + lo = unbase64(c); \ + *i += 1; \ + if (lo < 64) break; /* Legal character. */ \ + if (c == '=' || *i >= srclen) return false; /* Stop decoding. */ \ + } \ + expr; \ + if (*i >= srclen) return false; \ + if (*k >= dstlen) return false; \ hi = lo; V(/* Nothing. */); V(dst[(*k)++] = ((hi & 0x3F) << 2) | ((lo & 0x30) >> 4)); @@ -66,10 +62,10 @@ size_t base64_decode_fast(char* const dst, const size_t dstlen, size_t k = 0; while (i < max_i && k < max_k) { const unsigned char txt[] = { - static_cast(unbase64(src[i + 0])), - static_cast(unbase64(src[i + 1])), - static_cast(unbase64(src[i + 2])), - static_cast(unbase64(src[i + 3])), + static_cast(unbase64(static_cast(src[i + 0]))), + static_cast(unbase64(static_cast(src[i + 1]))), + static_cast(unbase64(static_cast(src[i + 2]))), + static_cast(unbase64(static_cast(src[i + 3]))), }; const uint32_t v = ReadUint32BE(txt); diff --git a/src/crypto/crypto_dsa.cc b/src/crypto/crypto_dsa.cc index c4016dc07c6743..d3446eb60a2789 100644 --- a/src/crypto/crypto_dsa.cc +++ b/src/crypto/crypto_dsa.cc @@ -146,14 +146,18 @@ Maybe GetDsaKeyDetail( size_t modulus_length = BN_num_bytes(p) * CHAR_BIT; size_t divisor_length = BN_num_bytes(q) * CHAR_BIT; - if (target->Set( - env->context(), - env->modulus_length_string(), - Number::New(env->isolate(), modulus_length)).IsNothing() || - target->Set( - env->context(), - env->divisor_length_string(), - Number::New(env->isolate(), divisor_length)).IsNothing()) { + if (target + ->Set( + env->context(), + env->modulus_length_string(), + Number::New(env->isolate(), static_cast(modulus_length))) + .IsNothing() || + target + ->Set( + env->context(), + env->divisor_length_string(), + Number::New(env->isolate(), static_cast(divisor_length))) + .IsNothing()) { return Nothing(); } diff --git a/src/crypto/crypto_keygen.cc b/src/crypto/crypto_keygen.cc index b37cd87da944dd..b08ca3b2f513d6 100644 --- a/src/crypto/crypto_keygen.cc +++ b/src/crypto/crypto_keygen.cc @@ -66,7 +66,8 @@ Maybe SecretKeyGenTraits::AdditionalConfig( SecretKeyGenConfig* params) { Environment* env = Environment::GetCurrent(args); CHECK(args[*offset]->IsUint32()); - params->length = std::trunc(args[*offset].As()->Value() / CHAR_BIT); + params->length = static_cast( + std::trunc(args[*offset].As()->Value() / CHAR_BIT)); if (params->length > INT_MAX) { const std::string msg{ SPrintF("length must be less than or equal to %s bits", diff --git a/src/crypto/crypto_keys.cc b/src/crypto/crypto_keys.cc index 07004b78bac954..4f0102a0eede41 100644 --- a/src/crypto/crypto_keys.cc +++ b/src/crypto/crypto_keys.cc @@ -529,10 +529,9 @@ Maybe GetSecretKeyDetail( // converted to bits. size_t length = key->GetSymmetricKeySize() * CHAR_BIT; - return target->Set( - env->context(), - env->length_string(), - Number::New(env->isolate(), length)); + return target->Set(env->context(), + env->length_string(), + Number::New(env->isolate(), static_cast(length))); } Maybe GetAsymmetricKeyDetail( diff --git a/src/crypto/crypto_rsa.cc b/src/crypto/crypto_rsa.cc index 741e1885e2659c..c3250ea2e42ac1 100644 --- a/src/crypto/crypto_rsa.cc +++ b/src/crypto/crypto_rsa.cc @@ -532,10 +532,12 @@ Maybe GetRsaKeyDetail( size_t modulus_length = BN_num_bytes(n) * CHAR_BIT; - if (target->Set( - env->context(), - env->modulus_length_string(), - Number::New(env->isolate(), modulus_length)).IsNothing()) { + if (target + ->Set( + env->context(), + env->modulus_length_string(), + Number::New(env->isolate(), static_cast(modulus_length))) + .IsNothing()) { return Nothing(); } diff --git a/src/inspector/worker_inspector.cc b/src/inspector/worker_inspector.cc index deeddcc0836dd1..ff292ea4422b19 100644 --- a/src/inspector/worker_inspector.cc +++ b/src/inspector/worker_inspector.cc @@ -11,7 +11,7 @@ namespace { class WorkerStartedRequest : public Request { public: WorkerStartedRequest( - int id, + uint64_t id, const std::string& url, std::shared_ptr worker_thread, bool waiting) @@ -28,7 +28,7 @@ class WorkerStartedRequest : public Request { return "Worker " + std::to_string(id); } - int id_; + uint64_t id_; WorkerInfo info_; bool waiting_; }; @@ -42,22 +42,25 @@ void Report(const std::unique_ptr& delegate, class WorkerFinishedRequest : public Request { public: - explicit WorkerFinishedRequest(int worker_id) : worker_id_(worker_id) {} + explicit WorkerFinishedRequest(uint64_t worker_id) : worker_id_(worker_id) {} void Call(MainThreadInterface* thread) override { thread->inspector_agent()->GetWorkerManager()->WorkerFinished(worker_id_); } private: - int worker_id_; + uint64_t worker_id_; }; } // namespace - ParentInspectorHandle::ParentInspectorHandle( - int id, const std::string& url, - std::shared_ptr parent_thread, bool wait_for_connect) - : id_(id), url_(url), parent_thread_(parent_thread), + uint64_t id, + const std::string& url, + std::shared_ptr parent_thread, + bool wait_for_connect) + : id_(id), + url_(url), + parent_thread_(parent_thread), wait_(wait_for_connect) {} ParentInspectorHandle::~ParentInspectorHandle() { @@ -78,11 +81,11 @@ std::unique_ptr ParentInspectorHandle::Connect( return parent_thread_->Connect(std::move(delegate), prevent_shutdown); } -void WorkerManager::WorkerFinished(int session_id) { +void WorkerManager::WorkerFinished(uint64_t session_id) { children_.erase(session_id); } -void WorkerManager::WorkerStarted(int session_id, +void WorkerManager::WorkerStarted(uint64_t session_id, const WorkerInfo& info, bool waiting) { if (info.worker_thread->Expired()) @@ -93,8 +96,8 @@ void WorkerManager::WorkerStarted(int session_id, } } -std::unique_ptr -WorkerManager::NewParentHandle(int thread_id, const std::string& url) { +std::unique_ptr WorkerManager::NewParentHandle( + uint64_t thread_id, const std::string& url) { bool wait = !delegates_waiting_on_start_.empty(); return std::make_unique(thread_id, url, thread_, wait); } diff --git a/src/inspector/worker_inspector.h b/src/inspector/worker_inspector.h index b01063e01fc890..540d98c742fe05 100644 --- a/src/inspector/worker_inspector.h +++ b/src/inspector/worker_inspector.h @@ -53,12 +53,13 @@ struct WorkerInfo { class ParentInspectorHandle { public: - ParentInspectorHandle(int id, const std::string& url, + ParentInspectorHandle(uint64_t id, + const std::string& url, std::shared_ptr parent_thread, bool wait_for_connect); ~ParentInspectorHandle(); std::unique_ptr NewParentInspectorHandle( - int thread_id, const std::string& url) { + uint64_t thread_id, const std::string& url) { return std::make_unique(thread_id, url, parent_thread_, @@ -75,7 +76,7 @@ class ParentInspectorHandle { bool prevent_shutdown); private: - int id_; + uint64_t id_; std::string url_; std::shared_ptr parent_thread_; bool wait_; @@ -87,9 +88,9 @@ class WorkerManager : public std::enable_shared_from_this { : thread_(thread) {} std::unique_ptr NewParentHandle( - int thread_id, const std::string& url); - void WorkerStarted(int session_id, const WorkerInfo& info, bool waiting); - void WorkerFinished(int session_id); + uint64_t thread_id, const std::string& url); + void WorkerStarted(uint64_t session_id, const WorkerInfo& info, bool waiting); + void WorkerFinished(uint64_t session_id); std::unique_ptr SetAutoAttach( std::unique_ptr attach_delegate); void SetWaitOnStartForDelegate(int id, bool wait); @@ -100,7 +101,7 @@ class WorkerManager : public std::enable_shared_from_this { private: std::shared_ptr thread_; - std::unordered_map children_; + std::unordered_map children_; std::unordered_map> delegates_; // If any one needs it, workers stop for all std::unordered_set delegates_waiting_on_start_; diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index c101bf80d2a6e9..9c3721f0689b0e 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -527,7 +527,7 @@ class NodeInspectorClient : public V8InspectorClient { timers_.emplace(std::piecewise_construct, std::make_tuple(data), std::make_tuple(env_, [=]() { callback(data); })); CHECK(result.second); - uint64_t interval = 1000 * interval_s; + uint64_t interval = static_cast(1000 * interval_s); result.first->second.Update(interval, interval); } @@ -917,7 +917,7 @@ void Agent::SetParentHandle( } std::unique_ptr Agent::GetParentHandle( - int thread_id, const std::string& url) { + uint64_t thread_id, const std::string& url) { if (!parent_handle_) { return client_->getWorkerManager()->NewParentHandle(thread_id, url); } else { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index efd090c49b4311..08b8817f436286 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -84,7 +84,7 @@ class Agent { void SetParentHandle(std::unique_ptr parent_handle); std::unique_ptr GetParentHandle( - int thread_id, const std::string& url); + uint64_t thread_id, const std::string& url); // Called to create inspector sessions that can be used from the same thread. // The inspector responds by using the delegate to send messages back. diff --git a/src/node_serdes.cc b/src/node_serdes.cc index 879253f9bc2ebc..835179d511417a 100644 --- a/src/node_serdes.cc +++ b/src/node_serdes.cc @@ -443,7 +443,7 @@ void DeserializerContext::ReadRawBytes( CHECK_GE(position, ctx->data_); CHECK_LE(position + length, ctx->data_ + ctx->length_); - const uint32_t offset = position - ctx->data_; + const uint32_t offset = static_cast(position - ctx->data_); CHECK_EQ(ctx->data_ + offset, position); args.GetReturnValue().Set(offset); diff --git a/src/node_url.cc b/src/node_url.cc index 82fe8078a8db0a..399c37638ad56c 100644 --- a/src/node_url.cc +++ b/src/node_url.cc @@ -896,7 +896,7 @@ void URLHost::ParseIPv6Host(const char* input, size_t length) { } if (compress_pointer != nullptr) { - unsigned swaps = piece_pointer - compress_pointer; + int64_t swaps = piece_pointer - compress_pointer; piece_pointer = buffer_end - 1; while (piece_pointer != &value_.ipv6[0] && swaps > 0) { uint16_t temp = *piece_pointer; @@ -963,7 +963,7 @@ void URLHost::ParseIPv4Host(const char* input, size_t length, bool* is_ipv4) { while (pointer <= end) { const char ch = pointer < end ? pointer[0] : kEOL; - int remaining = end - pointer - 1; + int64_t remaining = end - pointer - 1; if (ch == '.' || ch == kEOL) { if (++parts > static_cast(arraysize(numbers))) return; @@ -996,10 +996,11 @@ void URLHost::ParseIPv4Host(const char* input, size_t length, bool* is_ipv4) { } type_ = HostType::H_IPV4; - val = numbers[parts - 1]; + val = static_cast(numbers[parts - 1]); for (int n = 0; n < parts - 1; n++) { double b = 3 - n; - val += numbers[n] * pow(256, b); + val += + static_cast(numbers[n]) * static_cast(pow(256, b)); } value_.ipv4 = val; diff --git a/src/node_wasi.cc b/src/node_wasi.cc index 67d3966e2013a1..ffcc37c5274c98 100644 --- a/src/node_wasi.cc +++ b/src/node_wasi.cc @@ -279,7 +279,8 @@ void WASI::ArgsGet(const FunctionCallbackInfo& args) { if (err == UVWASI_ESUCCESS) { for (size_t i = 0; i < wasi->uvw_.argc; i++) { - uint32_t offset = argv_buf_offset + (argv[i] - argv[0]); + uint32_t offset = + static_cast(argv_buf_offset + (argv[i] - argv[0])); uvwasi_serdes_write_uint32_t(memory, argv_offset + (i * UVWASI_SERDES_SIZE_uint32_t), @@ -410,7 +411,8 @@ void WASI::EnvironGet(const FunctionCallbackInfo& args) { if (err == UVWASI_ESUCCESS) { for (size_t i = 0; i < wasi->uvw_.envc; i++) { - uint32_t offset = environ_buf_offset + (environment[i] - environment[0]); + uint32_t offset = static_cast( + environ_buf_offset + (environment[i] - environment[0])); uvwasi_serdes_write_uint32_t(memory, environ_offset + diff --git a/src/signal_wrap.cc b/src/signal_wrap.cc index 49ea849f637cf7..cdf063c035a5d3 100644 --- a/src/signal_wrap.cc +++ b/src/signal_wrap.cc @@ -153,7 +153,7 @@ class SignalWrap : public HandleWrap { void DecreaseSignalHandlerCount(int signum) { Mutex::ScopedLock lock(handled_signals_mutex); - int new_handler_count = --handled_signals[signum]; + int64_t new_handler_count = --handled_signals[signum]; CHECK_GE(new_handler_count, 0); if (new_handler_count == 0) handled_signals.erase(signum); diff --git a/src/stream_base.cc b/src/stream_base.cc index 87781efb0e8111..8e2585bf5d56eb 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -337,7 +337,7 @@ MaybeLocal StreamBase::CallJSOnreadMethod(ssize_t nread, } } - env->stream_base_state()[kReadBytesOrError] = nread; + env->stream_base_state()[kReadBytesOrError] = static_cast(nread); env->stream_base_state()[kArrayBufferOffset] = offset; Local argv[] = { diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 556dba97c093f3..daff1424d22e5a 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -250,8 +250,8 @@ static size_t hex_decode(char* buf, const size_t srcLen) { size_t i; for (i = 0; i < len && i * 2 + 1 < srcLen; ++i) { - unsigned a = unhex(src[i * 2 + 0]); - unsigned b = unhex(src[i * 2 + 1]); + unsigned a = unhex(static_cast(src[i * 2 + 0])); + unsigned b = unhex(static_cast(src[i * 2 + 1])); if (!~a || !~b) return i; buf[i] = (a << 4) | b; diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index e746f62c5edd81..10a5fd929f6037 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -531,7 +531,7 @@ void UDPWrap::DoSend(const FunctionCallbackInfo& args, int family) { wrap->current_send_has_callback_ = sendto ? args[5]->IsTrue() : args[3]->IsTrue(); - err = wrap->Send(*bufs, count, addr); + err = static_cast(wrap->Send(*bufs, count, addr)); wrap->current_send_req_wrap_.Clear(); wrap->current_send_has_callback_ = false; @@ -705,11 +705,10 @@ void UDPWrap::OnRecv(ssize_t nread, Context::Scope context_scope(env->context()); Local argv[] = { - Integer::New(env->isolate(), nread), - object(), - Undefined(env->isolate()), - Undefined(env->isolate()) - }; + Integer::New(env->isolate(), static_cast(nread)), + object(), + Undefined(env->isolate()), + Undefined(env->isolate())}; if (nread < 0) { MakeCallback(env->onmessage_string(), arraysize(argv), argv); From 6e89d7364c03f6398f2a3f8cdc9b49a17776ddf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 12 Feb 2021 20:58:42 +0100 Subject: [PATCH 2/4] src: disable unfixable MSVC warnings The following warnings are disabled: - C4065 in node_revert.h: there are no security reversions on the master branch. - C4003 in base64-inl.h: a macro is used four times, only once without parameters. --- src/base64-inl.h | 8 ++++++++ src/node_revert.h | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/base64-inl.h b/src/base64-inl.h index 1046fd83b9d417..1b6cdd93f002a4 100644 --- a/src/base64-inl.h +++ b/src/base64-inl.h @@ -23,6 +23,11 @@ inline uint32_t ReadUint32BE(const unsigned char* p) { static_cast(p[3]); } +#ifdef _MSC_VER +#pragma warning(push) +// MSVC C4003: not enough actual parameters for macro 'identifier' +#pragma warning(disable : 4003) +#endif template bool base64_decode_group_slow(char* const dst, const size_t dstlen, @@ -50,6 +55,9 @@ bool base64_decode_group_slow(char* const dst, const size_t dstlen, return true; // Continue decoding. } +#ifdef _MSC_VER +#pragma warning(pop) +#endif template size_t base64_decode_fast(char* const dst, const size_t dstlen, diff --git a/src/node_revert.h b/src/node_revert.h index 0f0c32412ef0b5..edf6ad772eecb0 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -28,6 +28,12 @@ namespace per_process { extern unsigned int reverted_cve; } +#ifdef _MSC_VER +#pragma warning(push) +// MSVC C4065: switch statement contains 'default' but no 'case' labels +#pragma warning(disable : 4065) +#endif + inline const char* RevertMessage(const reversion cve) { #define V(code, label, msg) case SECURITY_REVERT_##code: return label ": " msg; switch (cve) { @@ -38,6 +44,10 @@ inline const char* RevertMessage(const reversion cve) { #undef V } +#ifdef _MSC_VER +#pragma warning(pop) +#endif + inline void Revert(const reversion cve) { per_process::reverted_cve |= 1 << cve; printf("SECURITY WARNING: Reverting %s\n", RevertMessage(cve)); From 4f94dbc7db6b31ed14b76d0adf1e1e67ed632ede Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 12 Feb 2021 20:59:38 +0100 Subject: [PATCH 3/4] src: fix ETW_WRITE_EMPTY_EVENT macro A comment was written before the last line, hiding a check. --- src/node_win32_etw_provider-inl.h | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/node_win32_etw_provider-inl.h b/src/node_win32_etw_provider-inl.h index a7846f413a204c..c1b7b6b8de51ae 100644 --- a/src/node_win32_etw_provider-inl.h +++ b/src/node_win32_etw_provider-inl.h @@ -111,13 +111,14 @@ extern int events_enabled; dataDescriptors); \ CHECK_EQ(status, ERROR_SUCCESS); -#define ETW_WRITE_EMPTY_EVENT(eventDescriptor) \ - DWORD status = event_write(node_provider, \ - &eventDescriptor, \ - 0, \ - NULL); // NOLINT (readability/null_usage) \ +// NOLINTNEXTLINE (readability/null_usage) +#define NULL_NOLINT NULL + +#define ETW_WRITE_EMPTY_EVENT(eventDescriptor) \ + DWORD status = event_write(node_provider, &eventDescriptor, 0, NULL_NOLINT); \ CHECK_EQ(status, ERROR_SUCCESS); +#undef NULL_NOLINT void NODE_HTTP_SERVER_REQUEST(node_dtrace_http_server_request_t* req, node_dtrace_connection_t* conn, const char* remote, int port, From 8d9cac8abc7ee55e0456ac1dc730bbfb092fadd5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Fri, 12 Feb 2021 21:37:51 +0100 Subject: [PATCH 4/4] fixup! src: fix ETW_WRITE_EMPTY_EVENT macro --- src/node_win32_etw_provider-inl.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_win32_etw_provider-inl.h b/src/node_win32_etw_provider-inl.h index c1b7b6b8de51ae..9238ea22a3a179 100644 --- a/src/node_win32_etw_provider-inl.h +++ b/src/node_win32_etw_provider-inl.h @@ -118,8 +118,6 @@ extern int events_enabled; DWORD status = event_write(node_provider, &eventDescriptor, 0, NULL_NOLINT); \ CHECK_EQ(status, ERROR_SUCCESS); -#undef NULL_NOLINT - void NODE_HTTP_SERVER_REQUEST(node_dtrace_http_server_request_t* req, node_dtrace_connection_t* conn, const char* remote, int port, const char* method, const char* url, int fd) { @@ -272,6 +270,8 @@ void NODE_V8SYMBOL_ADD(LPCSTR symbol, } #undef SETSYMBUF +#undef NULL_NOLINT + bool NODE_HTTP_SERVER_REQUEST_ENABLED() { return events_enabled > 0; } bool NODE_HTTP_SERVER_RESPONSE_ENABLED() { return events_enabled > 0; }