From 16ef09fc491b7a3e9bc07a290567859ca6f90539 Mon Sep 17 00:00:00 2001 From: Myles Borins Date: Wed, 6 Jun 2018 16:20:30 +0200 Subject: [PATCH 01/17] Working on v10.4.1 PR-URL: https://github.com/nodejs/node/pull/21167 --- src/node_version.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/node_version.h b/src/node_version.h index 0e0f325d07a..47ff6d71793 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -24,12 +24,12 @@ #define NODE_MAJOR_VERSION 10 #define NODE_MINOR_VERSION 4 -#define NODE_PATCH_VERSION 0 +#define NODE_PATCH_VERSION 1 #define NODE_VERSION_IS_LTS 0 #define NODE_VERSION_LTS_CODENAME "" -#define NODE_VERSION_IS_RELEASE 1 +#define NODE_VERSION_IS_RELEASE 0 #ifndef NODE_STRINGIFY #define NODE_STRINGIFY(n) NODE_STRINGIFY_HELPER(n) From 53f85633538ea619bac8125696c94f25c2ba7b68 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 4 Jun 2018 19:20:54 -0400 Subject: [PATCH 02/17] n-api: back up env before async work finalize We must back up the value of `_env` before calling the async work complete callback, because the complete callback may delete the instance in which `_env` is stored by calling `napi_delete_async_work`, and because we need to use it after the complete callback has completed. Fixes: https://github.com/nodejs/node/issues/20966 PR-URL: https://github.com/nodejs/node/pull/21129 Reviewed-By: Anatoli Papirovski Reviewed-By: Refael Ackermann Reviewed-By: Michael Dawson --- src/node_api.cc | 12 +++++-- test/addons-napi/test_async/test-loop.js | 14 ++++++++ test/addons-napi/test_async/test_async.cc | 43 +++++++++++++++++++++++ 3 files changed, 66 insertions(+), 3 deletions(-) create mode 100644 test/addons-napi/test_async/test-loop.js diff --git a/src/node_api.cc b/src/node_api.cc index a83244131ff..fdd12afc220 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3393,13 +3393,19 @@ class Work : public node::AsyncResource, public node::ThreadPoolWork { CallbackScope callback_scope(this); - NAPI_CALL_INTO_MODULE(_env, + // We have to back up the env here because the `NAPI_CALL_INTO_MODULE` macro + // makes use of it after the call into the module completes, but the module + // may have deallocated **this**, and along with it the place where _env is + // stored. + napi_env env = _env; + + NAPI_CALL_INTO_MODULE(env, _complete(_env, ConvertUVErrorCode(status), _data), - [this] (v8::Local local_err) { + [env] (v8::Local local_err) { // If there was an unhandled exception in the complete callback, // report it as a fatal exception. (There is no JavaScript on the // callstack that can possibly handle it.) - v8impl::trigger_fatal_exception(_env, local_err); + v8impl::trigger_fatal_exception(env, local_err); }); // Note: Don't access `work` after this point because it was diff --git a/test/addons-napi/test_async/test-loop.js b/test/addons-napi/test_async/test-loop.js new file mode 100644 index 00000000000..efd15bcb2d0 --- /dev/null +++ b/test/addons-napi/test_async/test-loop.js @@ -0,0 +1,14 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const test_async = require(`./build/${common.buildType}/test_async`); +const iterations = 500; + +let x = 0; +const workDone = common.mustCall((status) => { + assert.strictEqual(status, 0, 'Work completed successfully'); + if (++x < iterations) { + setImmediate(() => test_async.DoRepeatedWork(workDone)); + } +}, iterations); +test_async.DoRepeatedWork(workDone); diff --git a/test/addons-napi/test_async/test_async.cc b/test/addons-napi/test_async/test_async.cc index 4d3e025097b..a7ea0eb64c0 100644 --- a/test/addons-napi/test_async/test_async.cc +++ b/test/addons-napi/test_async/test_async.cc @@ -1,3 +1,4 @@ +#include #include #include "../common.h" @@ -173,10 +174,52 @@ napi_value TestCancel(napi_env env, napi_callback_info info) { return nullptr; } +struct { + napi_ref ref; + napi_async_work work; +} repeated_work_info = { nullptr, nullptr }; + +static void RepeatedWorkerThread(napi_env env, void* data) {} + +static void RepeatedWorkComplete(napi_env env, napi_status status, void* data) { + napi_value cb, js_status; + NAPI_CALL_RETURN_VOID(env, + napi_get_reference_value(env, repeated_work_info.ref, &cb)); + NAPI_CALL_RETURN_VOID(env, + napi_delete_async_work(env, repeated_work_info.work)); + NAPI_CALL_RETURN_VOID(env, + napi_delete_reference(env, repeated_work_info.ref)); + repeated_work_info.work = nullptr; + repeated_work_info.ref = nullptr; + NAPI_CALL_RETURN_VOID(env, + napi_create_uint32(env, (uint32_t)status, &js_status)); + NAPI_CALL_RETURN_VOID(env, + napi_call_function(env, cb, cb, 1, &js_status, nullptr)); +} + +static napi_value DoRepeatedWork(napi_env env, napi_callback_info info) { + size_t argc = 1; + napi_value cb, name; + NAPI_ASSERT(env, repeated_work_info.ref == nullptr, + "Reference left over from previous work"); + NAPI_ASSERT(env, repeated_work_info.work == nullptr, + "Work pointer left over from previous work"); + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &cb, nullptr, nullptr)); + NAPI_CALL(env, napi_create_reference(env, cb, 1, &repeated_work_info.ref)); + NAPI_CALL(env, + napi_create_string_utf8(env, "Repeated Work", NAPI_AUTO_LENGTH, &name)); + NAPI_CALL(env, + napi_create_async_work(env, nullptr, name, RepeatedWorkerThread, + RepeatedWorkComplete, &repeated_work_info, &repeated_work_info.work)); + NAPI_CALL(env, napi_queue_async_work(env, repeated_work_info.work)); + return nullptr; +} + napi_value Init(napi_env env, napi_value exports) { napi_property_descriptor properties[] = { DECLARE_NAPI_PROPERTY("Test", Test), DECLARE_NAPI_PROPERTY("TestCancel", TestCancel), + DECLARE_NAPI_PROPERTY("DoRepeatedWork", DoRepeatedWork), }; NAPI_CALL(env, napi_define_properties( From 4de7e0c96cfe7df473bab8e73b1cfc60692b9e80 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sat, 9 Jun 2018 17:11:06 -0700 Subject: [PATCH 03/17] deps,npm: float node-gyp patch on npm This small change makes addon test build successfully with LLVM 10.0.0 and above. This will be fixed in npm source when node-gyp is updated to 3.6.3 or above. PR-URL: https://github.com/nodejs/node/pull/21239 Reviewed-By: Anna Henningsen Reviewed-By: Minwoo Jung Reviewed-By: Colin Ihrig Reviewed-By: Joyee Cheung --- deps/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py b/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py index b06bdc4e8b7..da29ebdf1ae 100644 --- a/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py +++ b/deps/npm/node_modules/node-gyp/gyp/pylib/gyp/xcode_emulation.py @@ -1262,7 +1262,7 @@ def XcodeVersion(): except: version = CLTVersion() if version: - version = re.match(r'(\d\.\d\.?\d*)', version).groups()[0] + version = re.match(r'(\d+\.\d+\.?\d*)', version).groups()[0] else: raise GypError("No Xcode or CLT version detected!") # The CLT has no build information, so we return an empty string. From 1e49eadd685d1ba626da04f0562ef833af12fefa Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 8 Jun 2018 10:23:18 -0700 Subject: [PATCH 04/17] tools,gyp: fix regex for version matching Tool versions can be 10 and higher. Float patch from node-gyp to accommodate this fact of life. PR-URL: https://github.com/nodejs/node/pull/21216 Refs: https://github.com/nodejs/node-gyp/commit/293092c362febffe19f72712467565045e08e8f1 Reviewed-By: Refael Ackermann Reviewed-By: Ben Noordhuis Reviewed-By: Richard Lau Reviewed-By: Anna Henningsen --- tools/gyp/pylib/gyp/xcode_emulation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/gyp/pylib/gyp/xcode_emulation.py b/tools/gyp/pylib/gyp/xcode_emulation.py index 667c53695a1..9082b9da358 100644 --- a/tools/gyp/pylib/gyp/xcode_emulation.py +++ b/tools/gyp/pylib/gyp/xcode_emulation.py @@ -1403,7 +1403,7 @@ def XcodeVersion(): except: version = CLTVersion() if version: - version = re.match(r'(\d\.\d\.?\d*)', version).groups()[0] + version = re.match(r'(\d+\.\d+\.?\d*)', version).groups()[0] else: raise GypError("No Xcode or CLT version detected!") # The CLT has no build information, so we return an empty string. From 1bbfe9a72b6466ec11619d726d4c4047a78e1056 Mon Sep 17 00:00:00 2001 From: Misty De Meo Date: Wed, 6 Jun 2018 12:28:42 -0700 Subject: [PATCH 05/17] build: fix configure script for double-digits Compare versions using tuples instead of strings so that it is future-proofed against versions that contain a number that is more than one digit. PR-URL: https://github.com/nodejs/node/pull/21183 Reviewed-By: Richard Lau Reviewed-By: Rich Trott Reviewed-By: Joyee Cheung Reviewed-By: Jeremiah Senkpiel Reviewed-By: Trivikram Kamat --- configure | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/configure b/configure index c4dda999fd2..dcc7f594c8c 100755 --- a/configure +++ b/configure @@ -612,8 +612,8 @@ def try_check_compiler(cc, lang): values = (proc.communicate()[0].split() + ['0'] * 7)[0:7] is_clang = values[0] == '1' - gcc_version = '%s.%s.%s' % tuple(values[1:1+3]) - clang_version = '%s.%s.%s' % tuple(values[4:4+3]) + gcc_version = tuple(values[1:1+3]) + clang_version = tuple(values[4:4+3]) return (True, is_clang, clang_version, gcc_version) @@ -707,13 +707,13 @@ def check_compiler(o): ok, is_clang, clang_version, gcc_version = try_check_compiler(CXX, 'c++') if not ok: warn('failed to autodetect C++ compiler version (CXX=%s)' % CXX) - elif clang_version < '3.4.2' if is_clang else gcc_version < '4.9.4': + elif clang_version < (3, 4, 2) if is_clang else gcc_version < (4, 9, 4): warn('C++ compiler too old, need g++ 4.9.4 or clang++ 3.4.2 (CXX=%s)' % CXX) ok, is_clang, clang_version, gcc_version = try_check_compiler(CC, 'c') if not ok: warn('failed to autodetect C compiler version (CC=%s)' % CC) - elif not is_clang and gcc_version < '4.2.0': + elif not is_clang and gcc_version < (4, 2, 0): # clang 3.2 is a little white lie because any clang version will probably # do for the C bits. However, we might as well encourage people to upgrade # to a version that is not completely ancient. From 03ded94ffe272c6e919d912d2bec3ef9036c37ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Tue, 5 Jun 2018 11:08:12 +0200 Subject: [PATCH 06/17] deps: patch V8 to 6.7.288.44 Refs: https://github.com/v8/v8/compare/6.7.288.43...6.7.288.44 PR-URL: https://github.com/nodejs/node/pull/21146 Reviewed-By: Ben Noordhuis Reviewed-By: Colin Ihrig --- deps/v8/include/v8-version.h | 2 +- deps/v8/src/builtins/builtins-date.cc | 2 ++ deps/v8/src/intl.cc | 14 +++++++------- deps/v8/src/intl.h | 7 ++++--- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/deps/v8/include/v8-version.h b/deps/v8/include/v8-version.h index f871c8fa676..ed513b5b80d 100644 --- a/deps/v8/include/v8-version.h +++ b/deps/v8/include/v8-version.h @@ -11,7 +11,7 @@ #define V8_MAJOR_VERSION 6 #define V8_MINOR_VERSION 7 #define V8_BUILD_NUMBER 288 -#define V8_PATCH_LEVEL 43 +#define V8_PATCH_LEVEL 44 // Use 1 for candidates and 0 otherwise. // (Boolean macro values are not supported by all preprocessors.) diff --git a/deps/v8/src/builtins/builtins-date.cc b/deps/v8/src/builtins/builtins-date.cc index c60275d94ea..73c7098c06b 100644 --- a/deps/v8/src/builtins/builtins-date.cc +++ b/deps/v8/src/builtins/builtins-date.cc @@ -166,11 +166,13 @@ void ToDateString(double time_val, Vector str, DateCache* date_cache, kShortMonths[month], day, year); return; case kTimeOnly: + // TODO(842085): str may be silently truncated. SNPrintF(str, "%02d:%02d:%02d GMT%c%02d%02d (%s)", hour, min, sec, (timezone_offset < 0) ? '-' : '+', timezone_hour, timezone_min, local_timezone); return; case kDateAndTime: + // TODO(842085): str may be silently truncated. SNPrintF(str, "%s %s %02d %04d %02d:%02d:%02d GMT%c%02d%02d (%s)", kShortWeekDays[weekday], kShortMonths[month], day, year, hour, min, sec, (timezone_offset < 0) ? '-' : '+', timezone_hour, diff --git a/deps/v8/src/intl.cc b/deps/v8/src/intl.cc index 5c2cb4e8fe1..139bb4daf54 100644 --- a/deps/v8/src/intl.cc +++ b/deps/v8/src/intl.cc @@ -358,17 +358,17 @@ ICUTimezoneCache::~ICUTimezoneCache() { Clear(); } const char* ICUTimezoneCache::LocalTimezone(double time_ms) { bool is_dst = DaylightSavingsOffset(time_ms) != 0; - char* name = is_dst ? dst_timezone_name_ : timezone_name_; - if (name[0] == '\0') { + std::string* name = is_dst ? &dst_timezone_name_ : &timezone_name_; + if (name->empty()) { icu::UnicodeString result; GetTimeZone()->getDisplayName(is_dst, icu::TimeZone::LONG, result); result += '\0'; - icu::CheckedArrayByteSink byte_sink(name, kMaxTimezoneChars); + icu::StringByteSink byte_sink(name); result.toUTF8(byte_sink); - CHECK(!byte_sink.Overflowed()); } - return const_cast(name); + DCHECK(!name->empty()); + return name->c_str(); } icu::TimeZone* ICUTimezoneCache::GetTimeZone() { @@ -418,8 +418,8 @@ double ICUTimezoneCache::LocalTimeOffset(double time_ms, bool is_utc) { void ICUTimezoneCache::Clear() { delete timezone_; timezone_ = nullptr; - timezone_name_[0] = '\0'; - dst_timezone_name_[0] = '\0'; + timezone_name_.clear(); + dst_timezone_name_.clear(); } } // namespace internal diff --git a/deps/v8/src/intl.h b/deps/v8/src/intl.h index 967a3e92777..627cb4980de 100644 --- a/deps/v8/src/intl.h +++ b/deps/v8/src/intl.h @@ -9,6 +9,8 @@ #ifndef V8_INTL_H_ #define V8_INTL_H_ +#include + #include "src/base/timezone-cache.h" #include "src/objects.h" #include "src/objects/string.h" @@ -64,9 +66,8 @@ class ICUTimezoneCache : public base::TimezoneCache { icu::TimeZone* timezone_; - static const int32_t kMaxTimezoneChars = 100; - char timezone_name_[kMaxTimezoneChars]; - char dst_timezone_name_[kMaxTimezoneChars]; + std::string timezone_name_; + std::string dst_timezone_name_; }; } // namespace internal From e5c2f575b110879f74a54211a93c29ef023dd35e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C3=ABl=20Zasso?= Date: Thu, 7 Jun 2018 17:03:08 +0200 Subject: [PATCH 07/17] deps: patch V8 to 6.7.288.45 Refs: https://github.com/v8/v8/compare/6.7.288.44...6.7.288.45 PR-URL: https://github.com/nodejs/node/pull/21192 Reviewed-By: Ali Ijaz Sheikh Reviewed-By: James M Snell --- deps/v8/include/v8-version.h | 2 +- deps/v8/include/v8.h | 13 +- deps/v8/src/global-handles.cc | 55 +++-- deps/v8/src/global-handles.h | 8 +- .../v8/src/profiler/sampling-heap-profiler.cc | 1 + deps/v8/test/cctest/test-api.cc | 193 ++++++++++++++++++ 6 files changed, 255 insertions(+), 17 deletions(-) diff --git a/deps/v8/include/v8-version.h b/deps/v8/include/v8-version.h index ed513b5b80d..649deb515aa 100644 --- a/deps/v8/include/v8-version.h +++ b/deps/v8/include/v8-version.h @@ -11,7 +11,7 @@ #define V8_MAJOR_VERSION 6 #define V8_MINOR_VERSION 7 #define V8_BUILD_NUMBER 288 -#define V8_PATCH_LEVEL 44 +#define V8_PATCH_LEVEL 45 // Use 1 for candidates and 0 otherwise. // (Boolean macro values are not supported by all preprocessors.) diff --git a/deps/v8/include/v8.h b/deps/v8/include/v8.h index ed1a6d4af1a..dcee4eed4e8 100644 --- a/deps/v8/include/v8.h +++ b/deps/v8/include/v8.h @@ -9065,6 +9065,7 @@ class Internals { static const int kNodeStateIsWeakValue = 2; static const int kNodeStateIsPendingValue = 3; static const int kNodeStateIsNearDeathValue = 4; + static const int kNodeIsIndependentShift = 3; static const int kNodeIsActiveShift = 4; static const int kFirstNonstringType = 0x80; @@ -9288,7 +9289,10 @@ void Persistent::Copy(const Persistent& that) { template bool PersistentBase::IsIndependent() const { - return true; + typedef internal::Internals I; + if (this->IsEmpty()) return false; + return I::GetNodeFlag(reinterpret_cast(this->val_), + I::kNodeIsIndependentShift); } template @@ -9377,7 +9381,12 @@ void PersistentBase::RegisterExternalReference(Isolate* isolate) const { } template -void PersistentBase::MarkIndependent() {} +void PersistentBase::MarkIndependent() { + typedef internal::Internals I; + if (this->IsEmpty()) return; + I::UpdateNodeFlag(reinterpret_cast(this->val_), true, + I::kNodeIsIndependentShift); +} template void PersistentBase::MarkActive() { diff --git a/deps/v8/src/global-handles.cc b/deps/v8/src/global-handles.cc index 90467168ab6..0e9b678ceb7 100644 --- a/deps/v8/src/global-handles.cc +++ b/deps/v8/src/global-handles.cc @@ -41,6 +41,8 @@ class GlobalHandles::Node { STATIC_ASSERT(WEAK == Internals::kNodeStateIsWeakValue); STATIC_ASSERT(PENDING == Internals::kNodeStateIsPendingValue); STATIC_ASSERT(NEAR_DEATH == Internals::kNodeStateIsNearDeathValue); + STATIC_ASSERT(static_cast(IsIndependent::kShift) == + Internals::kNodeIsIndependentShift); STATIC_ASSERT(static_cast(IsActive::kShift) == Internals::kNodeIsActiveShift); } @@ -52,6 +54,7 @@ class GlobalHandles::Node { object_ = reinterpret_cast(kGlobalHandleZapValue); class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId; index_ = 0; + set_independent(false); set_active(false); set_in_new_space_list(false); data_.next_free = nullptr; @@ -73,6 +76,7 @@ class GlobalHandles::Node { DCHECK(state() == FREE); object_ = object; class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId; + set_independent(false); set_active(false); set_state(NORMAL); data_.parameter = nullptr; @@ -92,6 +96,7 @@ class GlobalHandles::Node { // Zap the values for eager trapping. object_ = reinterpret_cast(kGlobalHandleZapValue); class_id_ = v8::HeapProfiler::kPersistentHandleNoClassId; + set_independent(false); set_active(false); weak_callback_ = nullptr; DecreaseBlockUses(); @@ -119,6 +124,9 @@ class GlobalHandles::Node { flags_ = NodeState::update(flags_, state); } + bool is_independent() { return IsIndependent::decode(flags_); } + void set_independent(bool v) { flags_ = IsIndependent::update(flags_, v); } + bool is_active() { return IsActive::decode(flags_); } @@ -183,6 +191,12 @@ class GlobalHandles::Node { set_state(PENDING); } + // Independent flag accessors. + void MarkIndependent() { + DCHECK(IsInUse()); + set_independent(true); + } + // Callback parameter accessors. void set_parameter(void* parameter) { DCHECK(IsInUse()); @@ -330,7 +344,7 @@ class GlobalHandles::Node { // Placed first to avoid offset computation. Object* object_; - // Next word stores class_id, index, and state. + // Next word stores class_id, index, state, and independent. // Note: the most aligned fields should go first. // Wrapper class ID. @@ -339,7 +353,10 @@ class GlobalHandles::Node { // Index in the containing handle block. uint8_t index_; + // This stores three flags (independent, partially_dependent and + // in_new_space_list) and a State. class NodeState : public BitField {}; + class IsIndependent : public BitField {}; // The following two fields are mutually exclusive class IsActive : public BitField {}; class IsInNewSpaceList : public BitField {}; @@ -591,6 +608,14 @@ void GlobalHandles::AnnotateStrongRetainer(Object** location, Node::FromLocation(location)->AnnotateStrongRetainer(label); } +void GlobalHandles::MarkIndependent(Object** location) { + Node::FromLocation(location)->MarkIndependent(); +} + +bool GlobalHandles::IsIndependent(Object** location) { + return Node::FromLocation(location)->is_independent(); +} + bool GlobalHandles::IsNearDeath(Object** location) { return Node::FromLocation(location)->IsNearDeath(); } @@ -647,7 +672,8 @@ void GlobalHandles::IdentifyWeakHandles(WeakSlotCallback should_reset_handle) { void GlobalHandles::IterateNewSpaceStrongAndDependentRoots(RootVisitor* v) { for (Node* node : new_space_nodes_) { if (node->IsStrongRetainer() || - (node->IsWeakRetainer() && node->is_active())) { + (node->IsWeakRetainer() && !node->is_independent() && + node->is_active())) { v->VisitRootPointer(Root::kGlobalHandles, node->label(), node->location()); } @@ -662,7 +688,8 @@ void GlobalHandles::IterateNewSpaceStrongAndDependentRootsAndIdentifyUnmodified( node->set_active(true); } if (node->IsStrongRetainer() || - (node->IsWeakRetainer() && node->is_active())) { + (node->IsWeakRetainer() && !node->is_independent() && + node->is_active())) { v->VisitRootPointer(Root::kGlobalHandles, node->label(), node->location()); } @@ -682,8 +709,8 @@ void GlobalHandles::MarkNewSpaceWeakUnmodifiedObjectsPending( WeakSlotCallbackWithHeap is_dead) { for (Node* node : new_space_nodes_) { DCHECK(node->is_in_new_space_list()); - if (node->IsWeak() && is_dead(isolate_->heap(), node->location())) { - DCHECK(!node->is_active()); + if ((node->is_independent() || !node->is_active()) && node->IsWeak() && + is_dead(isolate_->heap(), node->location())) { if (!node->IsPhantomCallback() && !node->IsPhantomResetHandle()) { node->MarkPending(); } @@ -695,8 +722,8 @@ void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForFinalizers( RootVisitor* v) { for (Node* node : new_space_nodes_) { DCHECK(node->is_in_new_space_list()); - if (!node->is_active() && node->IsWeakRetainer() && - (node->state() == Node::PENDING)) { + if ((node->is_independent() || !node->is_active()) && + node->IsWeakRetainer() && (node->state() == Node::PENDING)) { DCHECK(!node->IsPhantomCallback()); DCHECK(!node->IsPhantomResetHandle()); // Finalizers need to survive. @@ -710,8 +737,8 @@ void GlobalHandles::IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles( RootVisitor* v, WeakSlotCallbackWithHeap should_reset_handle) { for (Node* node : new_space_nodes_) { DCHECK(node->is_in_new_space_list()); - if (!node->is_active() && node->IsWeakRetainer() && - (node->state() != Node::PENDING)) { + if ((node->is_independent() || !node->is_active()) && + node->IsWeakRetainer() && (node->state() != Node::PENDING)) { DCHECK(node->IsPhantomResetHandle() || node->IsPhantomCallback()); if (should_reset_handle(isolate_->heap(), node->location())) { if (node->IsPhantomResetHandle()) { @@ -757,12 +784,15 @@ int GlobalHandles::PostScavengeProcessing( // the freed_nodes. continue; } - - // Active nodes are kept alive, so no further processing is requires. - if (node->is_active()) { + // Skip dependent or unmodified handles. Their weak callbacks might expect + // to be + // called between two global garbage collection callbacks which + // are not called for minor collections. + if (!node->is_independent() && (node->is_active())) { node->set_active(false); continue; } + node->set_active(false); if (node->PostGarbageCollectionProcessing(isolate_)) { if (initial_post_gc_processing_count != post_gc_processing_count_) { @@ -773,7 +803,6 @@ int GlobalHandles::PostScavengeProcessing( return freed_nodes; } } - if (!node->IsRetainer()) { freed_nodes++; } diff --git a/deps/v8/src/global-handles.h b/deps/v8/src/global-handles.h index e96b74b8834..2c2fbbd3f95 100644 --- a/deps/v8/src/global-handles.h +++ b/deps/v8/src/global-handles.h @@ -99,6 +99,11 @@ class GlobalHandles { // Clear the weakness of a global handle. static void* ClearWeakness(Object** location); + // Mark the reference to this object independent. + static void MarkIndependent(Object** location); + + static bool IsIndependent(Object** location); + // Tells whether global handle is near death. static bool IsNearDeath(Object** location); @@ -155,7 +160,8 @@ class GlobalHandles { void MarkNewSpaceWeakUnmodifiedObjectsPending( WeakSlotCallbackWithHeap is_dead); - // Iterates over weak unmodified handles. See the note above. + // Iterates over weak independent or unmodified handles. + // See the note above. void IterateNewSpaceWeakUnmodifiedRootsForFinalizers(RootVisitor* v); void IterateNewSpaceWeakUnmodifiedRootsForPhantomHandles( RootVisitor* v, WeakSlotCallbackWithHeap should_reset_handle); diff --git a/deps/v8/src/profiler/sampling-heap-profiler.cc b/deps/v8/src/profiler/sampling-heap-profiler.cc index 6912f3eba11..734c2ea36d7 100644 --- a/deps/v8/src/profiler/sampling-heap-profiler.cc +++ b/deps/v8/src/profiler/sampling-heap-profiler.cc @@ -99,6 +99,7 @@ void SamplingHeapProfiler::SampleObject(Address soon_object, size_t size) { Sample* sample = new Sample(size, node, loc, this); samples_.emplace(sample); sample->global.SetWeak(sample, OnWeakCallback, WeakCallbackType::kParameter); + sample->global.MarkIndependent(); } void SamplingHeapProfiler::OnWeakCallback( diff --git a/deps/v8/test/cctest/test-api.cc b/deps/v8/test/cctest/test-api.cc index 8b93944d933..7aa994fd077 100644 --- a/deps/v8/test/cctest/test-api.cc +++ b/deps/v8/test/cctest/test-api.cc @@ -7807,6 +7807,80 @@ struct FlagAndPersistent { v8::Global handle; }; +static void SetFlag(const v8::WeakCallbackInfo& data) { + data.GetParameter()->flag = true; + data.GetParameter()->handle.Reset(); +} + +static void IndependentWeakHandle(bool global_gc, bool interlinked) { + i::FLAG_stress_incremental_marking = false; + // Parallel scavenge introduces too much fragmentation. + i::FLAG_parallel_scavenge = false; + v8::Isolate* iso = CcTest::isolate(); + v8::HandleScope scope(iso); + v8::Local context = Context::New(iso); + Context::Scope context_scope(context); + + FlagAndPersistent object_a, object_b; + + size_t big_heap_size = 0; + size_t big_array_size = 0; + + { + v8::HandleScope handle_scope(iso); + Local a(v8::Object::New(iso)); + Local b(v8::Object::New(iso)); + object_a.handle.Reset(iso, a); + object_b.handle.Reset(iso, b); + if (interlinked) { + a->Set(context, v8_str("x"), b).FromJust(); + b->Set(context, v8_str("x"), a).FromJust(); + } + if (global_gc) { + CcTest::CollectAllGarbage(); + } else { + CcTest::CollectGarbage(i::NEW_SPACE); + } + v8::Local big_array = v8::Array::New(CcTest::isolate(), 5000); + // Verify that we created an array where the space was reserved up front. + big_array_size = + v8::internal::JSArray::cast(*v8::Utils::OpenHandle(*big_array)) + ->elements() + ->Size(); + CHECK_LE(20000, big_array_size); + a->Set(context, v8_str("y"), big_array).FromJust(); + big_heap_size = CcTest::heap()->SizeOfObjects(); + } + + object_a.flag = false; + object_b.flag = false; + object_a.handle.SetWeak(&object_a, &SetFlag, + v8::WeakCallbackType::kParameter); + object_b.handle.SetWeak(&object_b, &SetFlag, + v8::WeakCallbackType::kParameter); + CHECK(!object_b.handle.IsIndependent()); + object_a.handle.MarkIndependent(); + object_b.handle.MarkIndependent(); + CHECK(object_b.handle.IsIndependent()); + if (global_gc) { + CcTest::CollectAllGarbage(); + } else { + CcTest::CollectGarbage(i::NEW_SPACE); + } + // A single GC should be enough to reclaim the memory, since we are using + // phantom handles. + CHECK_GT(big_heap_size - big_array_size, CcTest::heap()->SizeOfObjects()); + CHECK(object_a.flag); + CHECK(object_b.flag); +} + +TEST(IndependentWeakHandle) { + IndependentWeakHandle(false, false); + IndependentWeakHandle(false, true); + IndependentWeakHandle(true, false); + IndependentWeakHandle(true, true); +} + class Trivial { public: explicit Trivial(int x) : x_(x) {} @@ -7899,6 +7973,125 @@ THREADED_TEST(InternalFieldCallback) { InternalFieldCallback(true); } +static void ResetUseValueAndSetFlag( + const v8::WeakCallbackInfo& data) { + // Blink will reset the handle, and then use the other handle, so they + // can't use the same backing slot. + data.GetParameter()->handle.Reset(); + data.GetParameter()->flag = true; +} + +void v8::internal::heap::HeapTester::ResetWeakHandle(bool global_gc) { + using v8::Context; + using v8::Local; + using v8::Object; + + v8::Isolate* iso = CcTest::isolate(); + v8::HandleScope scope(iso); + v8::Local context = Context::New(iso); + Context::Scope context_scope(context); + + FlagAndPersistent object_a, object_b; + + { + v8::HandleScope handle_scope(iso); + Local a(v8::Object::New(iso)); + Local b(v8::Object::New(iso)); + object_a.handle.Reset(iso, a); + object_b.handle.Reset(iso, b); + if (global_gc) { + CcTest::CollectAllGarbage(Heap::kAbortIncrementalMarkingMask); + } else { + CcTest::CollectGarbage(i::NEW_SPACE); + } + } + + object_a.flag = false; + object_b.flag = false; + object_a.handle.SetWeak(&object_a, &ResetUseValueAndSetFlag, + v8::WeakCallbackType::kParameter); + object_b.handle.SetWeak(&object_b, &ResetUseValueAndSetFlag, + v8::WeakCallbackType::kParameter); + if (!global_gc) { + object_a.handle.MarkIndependent(); + object_b.handle.MarkIndependent(); + CHECK(object_b.handle.IsIndependent()); + } + if (global_gc) { + CcTest::CollectAllGarbage(Heap::kAbortIncrementalMarkingMask); + } else { + CcTest::CollectGarbage(i::NEW_SPACE); + } + CHECK(object_a.flag); + CHECK(object_b.flag); +} + +THREADED_HEAP_TEST(ResetWeakHandle) { + v8::internal::heap::HeapTester::ResetWeakHandle(false); + v8::internal::heap::HeapTester::ResetWeakHandle(true); +} + +static void InvokeScavenge() { CcTest::CollectGarbage(i::NEW_SPACE); } + +static void InvokeMarkSweep() { CcTest::CollectAllGarbage(); } + +static void ForceScavenge2( + const v8::WeakCallbackInfo& data) { + data.GetParameter()->flag = true; + InvokeScavenge(); +} + +static void ForceScavenge1( + const v8::WeakCallbackInfo& data) { + data.GetParameter()->handle.Reset(); + data.SetSecondPassCallback(ForceScavenge2); +} + +static void ForceMarkSweep2( + const v8::WeakCallbackInfo& data) { + data.GetParameter()->flag = true; + InvokeMarkSweep(); +} + +static void ForceMarkSweep1( + const v8::WeakCallbackInfo& data) { + data.GetParameter()->handle.Reset(); + data.SetSecondPassCallback(ForceMarkSweep2); +} + +THREADED_TEST(GCFromWeakCallbacks) { + v8::Isolate* isolate = CcTest::isolate(); + v8::Locker locker(CcTest::isolate()); + v8::HandleScope scope(isolate); + v8::Local context = Context::New(isolate); + Context::Scope context_scope(context); + + static const int kNumberOfGCTypes = 2; + typedef v8::WeakCallbackInfo::Callback Callback; + Callback gc_forcing_callback[kNumberOfGCTypes] = {&ForceScavenge1, + &ForceMarkSweep1}; + + typedef void (*GCInvoker)(); + GCInvoker invoke_gc[kNumberOfGCTypes] = {&InvokeScavenge, &InvokeMarkSweep}; + + for (int outer_gc = 0; outer_gc < kNumberOfGCTypes; outer_gc++) { + for (int inner_gc = 0; inner_gc < kNumberOfGCTypes; inner_gc++) { + FlagAndPersistent object; + { + v8::HandleScope handle_scope(isolate); + object.handle.Reset(isolate, v8::Object::New(isolate)); + } + object.flag = false; + object.handle.SetWeak(&object, gc_forcing_callback[inner_gc], + v8::WeakCallbackType::kParameter); + object.handle.MarkIndependent(); + invoke_gc[outer_gc](); + EmptyMessageQueues(isolate); + CHECK(object.flag); + } + } +} + v8::Local args_fun; From cb5ec64956e4e6a61aae976ac908ab9952035ec5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 30 May 2018 11:18:43 +0200 Subject: [PATCH 08/17] src: reset TTY mode before cleaning up resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Otherwise, closing all handles associated with the main event loop would also mean that `uv_tty_reset_mode()` can’t function properly because the corresponding FDs have already been closed. Fixes: https://github.com/nodejs/node/issues/21020 PR-URL: https://github.com/nodejs/node/pull/21257 Reviewed-By: Richard Lau --- src/node.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/src/node.cc b/src/node.cc index eda97d81df1..481be857178 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4396,6 +4396,7 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data, WaitForInspectorDisconnect(&env); env.set_can_call_into_js(false); + uv_tty_reset_mode(); env.RunCleanup(); RunAtExit(&env); From 8e979482fab850b72940ee4283db67295f30fc43 Mon Sep 17 00:00:00 2001 From: Evan Lucas Date: Mon, 11 Jun 2018 09:20:50 -0500 Subject: [PATCH 09/17] Revert "src: restore stdio on program exit" This reverts commit c2c9c0c3d3199453cb74f64432fb76237d6f4ec4. It seems to be causing hangs when piping output to other processes. PR-URL: https://github.com/nodejs/node/pull/21257 Reviewed-By: Anna Henningsen Reviewed-By: Richard Lau --- src/node.cc | 100 ++++------------------------------------------------ 1 file changed, 6 insertions(+), 94 deletions(-) diff --git a/src/node.cc b/src/node.cc index 481be857178..8d2389c557c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -100,7 +100,6 @@ typedef int mode_t; #else #include #include // getrlimit, setrlimit -#include // tcgetattr, tcsetattr #include // setuid, getuid #endif @@ -173,9 +172,6 @@ using v8::Value; static Mutex process_mutex; static Mutex environ_mutex; -// Safe to call more than once and from signal handlers. -inline void PlatformExit(); - static bool print_eval = false; static bool force_repl = false; static bool syntax_check_only = false; @@ -1095,7 +1091,7 @@ void AppendExceptionLine(Environment* env, Mutex::ScopedLock lock(process_mutex); env->set_printed_error(true); - PlatformExit(); + uv_tty_reset_mode(); PrintErrorString("\n%s", arrow); return; } @@ -3029,7 +3025,7 @@ void SetupProcessObject(Environment* env, void SignalExit(int signo) { - PlatformExit(); + uv_tty_reset_mode(); v8_platform.StopTracingAgent(); #ifdef __FreeBSD__ // FreeBSD has a nasty bug, see RegisterSignalHandler for details @@ -3850,27 +3846,6 @@ static void DebugEnd(const FunctionCallbackInfo& args) { } -#ifdef __POSIX__ -static struct { - int flags; - bool isatty; - struct stat stat; - struct termios termios; -} stdio[1 + STDERR_FILENO]; - - -inline int GetFileDescriptorFlags(int fd) { - int flags; - - do { - flags = fcntl(fd, F_GETFL); - } while (flags == -1 && errno == EINTR); - - return flags; -} -#endif // __POSIX__ - - inline void PlatformInit() { #ifdef __POSIX__ #if HAVE_INSPECTOR @@ -3881,9 +3856,9 @@ inline void PlatformInit() { #endif // HAVE_INSPECTOR // Make sure file descriptors 0-2 are valid before we start logging anything. - for (auto& s : stdio) { - const int fd = &s - stdio; - if (fstat(fd, &s.stat) == 0) + for (int fd = STDIN_FILENO; fd <= STDERR_FILENO; fd += 1) { + struct stat ignored; + if (fstat(fd, &ignored) == 0) continue; // Anything but EBADF means something is seriously wrong. We don't // have to special-case EINTR, fstat() is not interruptible. @@ -3891,8 +3866,6 @@ inline void PlatformInit() { ABORT(); if (fd != open("/dev/null", O_RDWR)) ABORT(); - if (fstat(fd, &s.stat) != 0) - ABORT(); } #if HAVE_INSPECTOR @@ -3915,24 +3888,6 @@ inline void PlatformInit() { } #endif // !NODE_SHARED_MODE - // Record the state of the stdio file descriptors so we can restore it - // on exit. Needs to happen before installing signal handlers because - // they make use of that information. - for (auto& s : stdio) { - const int fd = &s - stdio; - int err; - - s.flags = GetFileDescriptorFlags(fd); - CHECK_NE(s.flags, -1); - - if (!isatty(fd)) continue; - s.isatty = true; - do { - err = tcgetattr(fd, &s.termios); - } while (err == -1 && errno == EINTR); - CHECK_EQ(err, 0); - } - RegisterSignalHandler(SIGINT, SignalExit, true); RegisterSignalHandler(SIGTERM, SignalExit, true); @@ -3973,49 +3928,6 @@ inline void PlatformInit() { } -// This function must be safe to call more than once and from signal handlers. -inline void PlatformExit() { -#ifdef __POSIX__ - for (auto& s : stdio) { - const int fd = &s - stdio; - - struct stat tmp; - if (-1 == fstat(fd, &tmp)) { - CHECK_EQ(errno, EBADF); // Program closed file descriptor. - continue; - } - - bool is_same_file = - (s.stat.st_dev == tmp.st_dev && s.stat.st_ino == tmp.st_ino); - if (!is_same_file) continue; // Program reopened file descriptor. - - int flags = GetFileDescriptorFlags(fd); - CHECK_NE(flags, -1); - - // Restore the O_NONBLOCK flag if it changed. - if (O_NONBLOCK & (flags ^ s.flags)) { - flags &= ~O_NONBLOCK; - flags |= s.flags & O_NONBLOCK; - - int err; - do { - err = fcntl(fd, F_SETFL, flags); - } while (err == -1 && errno == EINTR); - CHECK_NE(err, -1); - } - - if (s.isatty) { - int err; - do { - err = tcsetattr(fd, TCSANOW, &s.termios); - } while (err == -1 && errno == EINTR); - CHECK_NE(err, -1); - } - } -#endif // __POSIX__ -} - - void ProcessArgv(int* argc, const char** argv, int* exec_argc, @@ -4482,7 +4394,7 @@ inline int Start(uv_loop_t* event_loop, } int Start(int argc, char** argv) { - atexit([] () { PlatformExit(); }); + atexit([] () { uv_tty_reset_mode(); }); PlatformInit(); performance::performance_node_start = PERFORMANCE_NOW(); From 92d7b6c9a05aa568a1f5a27856ff76021eb54230 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Tue, 5 Jun 2018 09:12:19 -0400 Subject: [PATCH 10/17] fs: fix promises reads with pos > 4GB PR-URL: https://github.com/nodejs/node/pull/21148 Fixes: https://github.com/nodejs/node/issues/21121 Refs: https://github.com/nodejs/node/pull/21003 Reviewed-By: Anna Henningsen Reviewed-By: Joyee Cheung Reviewed-By: Refael Ackermann Reviewed-By: Trivikram Kamat Reviewed-By: Ujjwal Sharma --- lib/internal/fs/promises.js | 3 +-- test/parallel/test-fs-promises-file-handle-read.js | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 4c0a256f5ad..80b30c1a8fa 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -33,7 +33,6 @@ const { validatePath } = require('internal/fs/utils'); const { - isUint32, validateInt32, validateUint32 } = require('internal/validators'); @@ -213,7 +212,7 @@ async function read(handle, buffer, offset, length, position) { validateOffsetLengthRead(offset, length, buffer.length); - if (!isUint32(position)) + if (!Number.isSafeInteger(position)) position = -1; const bytesRead = (await binding.read(handle.fd, buffer, offset, length, diff --git a/test/parallel/test-fs-promises-file-handle-read.js b/test/parallel/test-fs-promises-file-handle-read.js index a397b0e260a..621e63c075a 100644 --- a/test/parallel/test-fs-promises-file-handle-read.js +++ b/test/parallel/test-fs-promises-file-handle-read.js @@ -8,6 +8,7 @@ const common = require('../common'); const fs = require('fs'); const { open } = fs.promises; const path = require('path'); +const fixtures = require('../common/fixtures'); const tmpdir = require('../common/tmpdir'); const assert = require('assert'); const tmpDir = tmpdir.path; @@ -40,6 +41,19 @@ async function validateEmptyRead() { assert.deepStrictEqual(buffer.length, readAsyncHandle.bytesRead); } +async function validateLargeRead() { + // Reading beyond file length (3 in this case) should return no data. + // This is a test for a bug where reads > uint32 would return data + // from the current position in the file. + const filePath = fixtures.path('x.txt'); + const fileHandle = await open(filePath, 'r'); + const pos = 0xffffffff + 1; // max-uint32 + 1 + const readHandle = await fileHandle.read(Buffer.alloc(1), 0, 1, pos); + + assert.strictEqual(readHandle.bytesRead, 0); +} + validateRead() .then(validateEmptyRead) + .then(validateLargeRead) .then(common.mustCall()); From eea2bce58d02c3171e1e997a1081327c2a899983 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 12 Apr 2018 21:53:59 +0200 Subject: [PATCH 11/17] tls: fix SSL write error handling Fix an use-after-free bug in the TLS implementation. If we return from `DoWrite()` with an early error, we should not be storing the `WriteWrap` object and complete it again at a later point, when it has already been freed (because of the write error). This issue was reported by Jordan Zebor at F5 Networks, who also helped with investigating this bug and coming up with a reproduction. This fixes CVE-2018-7162. Fixes: https://github.com/nodejs-private/security/issues/189 PR-URL: https://github.com/nodejs-private/node-private/pull/127 Reviewed-By: Evan Lucas --- src/stream_base.cc | 1 + src/tls_wrap.cc | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/stream_base.cc b/src/stream_base.cc index 71fe5db9758..bbb95bf8615 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -387,6 +387,7 @@ void ReportWritesToJSStreamListener::OnStreamAfterReqFinished( AsyncWrap* async_wrap = req_wrap->GetAsyncWrap(); HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); + CHECK(!async_wrap->persistent().IsEmpty()); Local req_wrap_obj = async_wrap->object(); Local argv[] = { diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index e74ee02aaa9..ec46088368d 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -622,8 +622,10 @@ int TLSWrap::DoWrite(WriteWrap* w, if (i != count) { int err; Local arg = GetSSLError(written, &err, &error_); - if (!arg.IsEmpty()) + if (!arg.IsEmpty()) { + current_write_ = nullptr; return UV_EPROTO; + } pending_cleartext_input_.insert(pending_cleartext_input_.end(), &bufs[i], From e87bf625ddbaf7f579fe823c71f2ecc9f3686c51 Mon Sep 17 00:00:00 2001 From: Shigeki Ohtsu Date: Thu, 12 Apr 2018 22:10:59 +0200 Subject: [PATCH 12/17] test: add tls write error regression test Add a mock TLS socket implementation and a regression test for the previous commit. Refs: https://github.com/nodejs-private/security/issues/189 PR-URL: https://github.com/nodejs-private/node-private/pull/127 Reviewed-By: Anna Henningsen Reviewed-By: Evan Lucas --- test/common/tls.js | 176 ++++++++++++++++++++++++++ test/parallel/test-tls-write-error.js | 55 ++++++++ 2 files changed, 231 insertions(+) create mode 100644 test/common/tls.js create mode 100644 test/parallel/test-tls-write-error.js diff --git a/test/common/tls.js b/test/common/tls.js new file mode 100644 index 00000000000..3560af671bc --- /dev/null +++ b/test/common/tls.js @@ -0,0 +1,176 @@ +/* eslint-disable node-core/required-modules, node-core/crypto-check */ + +'use strict'; +const crypto = require('crypto'); +const net = require('net'); + +exports.ccs = Buffer.from('140303000101', 'hex'); + +class TestTLSSocket extends net.Socket { + constructor(server_cert) { + super(); + this.server_cert = server_cert; + this.version = Buffer.from('0303', 'hex'); + this.handshake_list = []; + // AES128-GCM-SHA256 + this.ciphers = Buffer.from('000002009c0', 'hex'); + this.pre_master_secret = + Buffer.concat([this.version, crypto.randomBytes(46)]); + this.master_secret = null; + this.write_seq = 0; + this.client_random = crypto.randomBytes(32); + + this.on('handshake', (msg) => { + this.handshake_list.push(msg); + }); + + this.on('server_random', (server_random) => { + this.master_secret = PRF12('sha256', this.pre_master_secret, + 'master secret', + Buffer.concat([this.client_random, + server_random]), + 48); + const key_block = PRF12('sha256', this.master_secret, + 'key expansion', + Buffer.concat([server_random, + this.client_random]), + 40); + this.client_writeKey = key_block.slice(0, 16); + this.client_writeIV = key_block.slice(32, 36); + }); + } + + createClientHello() { + const compressions = Buffer.from('0100', 'hex'); // null + const msg = addHandshakeHeader(0x01, Buffer.concat([ + this.version, this.client_random, this.ciphers, compressions + ])); + this.emit('handshake', msg); + return addRecordHeader(0x16, msg); + } + + createClientKeyExchange() { + const encrypted_pre_master_secret = crypto.publicEncrypt({ + key: this.server_cert, + padding: crypto.constants.RSA_PKCS1_PADDING + }, this.pre_master_secret); + const length = Buffer.alloc(2); + length.writeUIntBE(encrypted_pre_master_secret.length, 0, 2); + const msg = addHandshakeHeader(0x10, Buffer.concat([ + length, encrypted_pre_master_secret])); + this.emit('handshake', msg); + return addRecordHeader(0x16, msg); + } + + createFinished() { + const shasum = crypto.createHash('sha256'); + shasum.update(Buffer.concat(this.handshake_list)); + const message_hash = shasum.digest(); + const r = PRF12('sha256', this.master_secret, + 'client finished', message_hash, 12); + const msg = addHandshakeHeader(0x14, r); + this.emit('handshake', msg); + return addRecordHeader(0x16, msg); + } + + createIllegalHandshake() { + const illegal_handshake = Buffer.alloc(5); + return addRecordHeader(0x16, illegal_handshake); + } + + parseTLSFrame(buf) { + let offset = 0; + const record = buf.slice(offset, 5); + const type = record[0]; + const length = record.slice(3, 5).readUInt16BE(0); + offset += 5; + let remaining = buf.slice(offset, offset + length); + if (type === 0x16) { + do { + remaining = this.parseTLSHandshake(remaining); + } while (remaining.length > 0); + } + offset += length; + return buf.slice(offset); + } + + parseTLSHandshake(buf) { + let offset = 0; + const handshake_type = buf[offset]; + if (handshake_type === 0x02) { + const server_random = buf.slice(6, 6 + 32); + this.emit('server_random', server_random); + } + offset += 1; + const length = buf.readUIntBE(offset, 3); + offset += 3; + const handshake = buf.slice(0, offset + length); + this.emit('handshake', handshake); + offset += length; + const remaining = buf.slice(offset); + return remaining; + } + + encrypt(plain) { + const type = plain.slice(0, 1); + const version = plain.slice(1, 3); + const nonce = crypto.randomBytes(8); + const iv = Buffer.concat([this.client_writeIV.slice(0, 4), nonce]); + const bob = crypto.createCipheriv('aes-128-gcm', this.client_writeKey, iv); + const write_seq = Buffer.alloc(8); + write_seq.writeUInt32BE(this.write_seq++, 4); + const aad = Buffer.concat([write_seq, plain.slice(0, 5)]); + bob.setAAD(aad); + const encrypted1 = bob.update(plain.slice(5)); + const encrypted = Buffer.concat([encrypted1, bob.final()]); + const tag = bob.getAuthTag(); + const length = Buffer.alloc(2); + length.writeUInt16BE(nonce.length + encrypted.length + tag.length, 0); + return Buffer.concat([type, version, length, nonce, encrypted, tag]); + } +} + +function addRecordHeader(type, frame) { + const record_layer = Buffer.from('0003030000', 'hex'); + record_layer[0] = type; + record_layer.writeUInt16BE(frame.length, 3); + return Buffer.concat([record_layer, frame]); +} + +function addHandshakeHeader(type, msg) { + const handshake_header = Buffer.alloc(4); + handshake_header[0] = type; + handshake_header.writeUIntBE(msg.length, 1, 3); + return Buffer.concat([handshake_header, msg]); +} + +function PRF12(algo, secret, label, seed, size) { + const newSeed = Buffer.concat([Buffer.from(label, 'utf8'), seed]); + return P_hash(algo, secret, newSeed, size); +} + +function P_hash(algo, secret, seed, size) { + const result = Buffer.alloc(size); + let hmac = crypto.createHmac(algo, secret); + hmac.update(seed); + let a = hmac.digest(); + let j = 0; + while (j < size) { + hmac = crypto.createHmac(algo, secret); + hmac.update(a); + hmac.update(seed); + const b = hmac.digest(); + let todo = b.length; + if (j + todo > size) { + todo = size - j; + } + b.copy(result, j, 0, todo); + j += todo; + hmac = crypto.createHmac(algo, secret); + hmac.update(a); + a = hmac.digest(); + } + return result; +} + +exports.TestTLSSocket = TestTLSSocket; diff --git a/test/parallel/test-tls-write-error.js b/test/parallel/test-tls-write-error.js new file mode 100644 index 00000000000..2783e62d063 --- /dev/null +++ b/test/parallel/test-tls-write-error.js @@ -0,0 +1,55 @@ +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const { TestTLSSocket, ccs } = require('../common/tls'); +const fixtures = require('../common/fixtures'); +const https = require('https'); + +// Regression test for an use-after-free bug in the TLS implementation that +// would occur when `SSL_write()` failed. +// Refs: https://github.com/nodejs-private/security/issues/189 + +const server_key = fixtures.readKey('agent1-key.pem'); +const server_cert = fixtures.readKey('agent1-cert.pem'); + +const opts = { + key: server_key, + cert: server_cert +}; + +const server = https.createServer(opts, (req, res) => { + res.write('hello'); +}).listen(0, common.mustCall(() => { + const client = new TestTLSSocket(server_cert); + + client.connect({ + host: 'localhost', + port: server.address().port + }, common.mustCall(() => { + const ch = client.createClientHello(); + client.write(ch); + })); + + client.once('data', common.mustCall((buf) => { + let remaining = buf; + do { + remaining = client.parseTLSFrame(remaining); + } while (remaining.length > 0); + + const cke = client.createClientKeyExchange(); + const finished = client.createFinished(); + const ill = client.createIllegalHandshake(); + const frames = Buffer.concat([ + cke, + ccs, + client.encrypt(finished), + client.encrypt(ill) + ]); + client.write(frames, common.mustCall(() => { + client.end(); + server.close(); + })); + })); +})); From 9ba8ed137158609263ca491b5b3b15c3bb14a121 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 29 Mar 2018 22:21:17 +0200 Subject: [PATCH 13/17] src: re-add `Realloc()` shrink after reading stream data This would otherwise keep a lot of unused memory lying around, and in particular add up to a page per chunk of memory overhead for network reads, potentially opening a DoS vector if the resulting `Buffer` objects are kept around indefinitely (e.g. stored in a list and not concatenated until the socket finishes). This fixes CVE-2018-7164. Refs: https://github.com/nodejs-private/security/issues/186 Refs: https://github.com/nodejs/node/commit/7c4b09b24bbe7d6a8cbad256f47b30a101a909ea PR-URL: https://github.com/nodejs-private/node-private/pull/128 Reviewed-By: Michael Dawson Reviewed-By: Evan Lucas --- src/stream_base.cc | 3 +- ...t-net-bytes-per-incoming-chunk-overhead.js | 41 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 test/sequential/test-net-bytes-per-incoming-chunk-overhead.js diff --git a/src/stream_base.cc b/src/stream_base.cc index bbb95bf8615..bb46ea1febb 100644 --- a/src/stream_base.cc +++ b/src/stream_base.cc @@ -374,8 +374,9 @@ void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) { } CHECK_LE(static_cast(nread), buf.len); + char* base = Realloc(buf.base, nread); - Local obj = Buffer::New(env, buf.base, nread).ToLocalChecked(); + Local obj = Buffer::New(env, base, nread).ToLocalChecked(); stream->CallJSOnreadMethod(nread, obj); } diff --git a/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js b/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js new file mode 100644 index 00000000000..8f766e8c7a4 --- /dev/null +++ b/test/sequential/test-net-bytes-per-incoming-chunk-overhead.js @@ -0,0 +1,41 @@ +// Flags: --expose-gc +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const net = require('net'); + +// Tests that, when receiving small chunks, we do not keep the full length +// of the original allocation for the libuv read call in memory. + +let client; +let baseRSS; +const receivedChunks = []; +const N = 250000; + +const server = net.createServer(common.mustCall((socket) => { + baseRSS = process.memoryUsage().rss; + + socket.setNoDelay(true); + socket.on('data', (chunk) => { + receivedChunks.push(chunk); + if (receivedChunks.length < N) { + client.write('a'); + } else { + client.end(); + server.close(); + } + }); +})).listen(0, common.mustCall(() => { + client = net.connect(server.address().port); + client.setNoDelay(true); + client.write('hello!'); +})); + +process.on('exit', () => { + global.gc(); + const bytesPerChunk = + (process.memoryUsage().rss - baseRSS) / receivedChunks.length; + // We should always have less than one page (usually ~ 4 kB) per chunk. + assert(bytesPerChunk < 512, `measured ${bytesPerChunk} bytes per chunk`); +}); From 86814022289e8924d48b63ae590bdf3a41876366 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Tue, 3 Apr 2018 11:26:38 -0700 Subject: [PATCH 14/17] http2: fixup http2stream cleanup and other nits This fixes CVE-2018-7161. PR-URL: https://github.com/nodejs-private/node-private/pull/115 Reviewed-By: Matteo Collina Reviewed-By: Anna Henningsen Reviewed-By: Evan Lucas --- src/node_http2.cc | 4 ++++ src/node_http2.h | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/node_http2.cc b/src/node_http2.cc index be43cc1e690..cb0d6a72e5b 100644 --- a/src/node_http2.cc +++ b/src/node_http2.cc @@ -499,6 +499,8 @@ Http2Session::Http2Session(Environment* env, Http2Session::~Http2Session() { CHECK_EQ(flags_ & SESSION_STATE_HAS_SCOPE, 0); Debug(this, "freeing nghttp2 session"); + for (const auto& iter : streams_) + iter.second->session_ = nullptr; nghttp2_session_del(session_); } @@ -651,6 +653,8 @@ inline void Http2Session::AddStream(Http2Stream* stream) { inline void Http2Session::RemoveStream(Http2Stream* stream) { + if (streams_.empty() || stream == nullptr) + return; // Nothing to remove, item was never added? streams_.erase(stream->id()); DecrementCurrentSessionMemory(stream->self_size()); } diff --git a/src/node_http2.h b/src/node_http2.h index e0a93c8f0fd..ea7a255b35f 100644 --- a/src/node_http2.h +++ b/src/node_http2.h @@ -651,8 +651,8 @@ class Http2Stream : public AsyncWrap, Statistics statistics_ = {}; private: - Http2Session* session_; // The Parent HTTP/2 Session - int32_t id_; // The Stream Identifier + Http2Session* session_ = nullptr; // The Parent HTTP/2 Session + int32_t id_ = 0; // The Stream Identifier int32_t code_ = NGHTTP2_NO_ERROR; // The RST_STREAM code (if any) int flags_ = NGHTTP2_STREAM_FLAG_NONE; // Internal state flags From 4c90ee8fc65070be8172c61c3da188f17e2a449b Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 13 Apr 2018 07:51:35 -0700 Subject: [PATCH 15/17] deps: update to nghttp2 1.32.0 This fixes CVE-2018-1000168. PR-URL: https://github.com/nodejs-private/node-private/pull/117 Reviewed-By: Matteo Collina Reviewed-By: Daniel Bevenius Reviewed-By: Colin Ihrig Reviewed-By: Evan Lucas --- deps/nghttp2/lib/CMakeLists.txt | 6 +- deps/nghttp2/lib/includes/nghttp2/nghttp2.h | 10 ++ .../nghttp2/lib/includes/nghttp2/nghttp2ver.h | 4 +- deps/nghttp2/lib/nghttp2_frame.c | 3 + deps/nghttp2/lib/nghttp2_http.c | 2 +- deps/nghttp2/lib/nghttp2_session.c | 151 ++++++++++++------ 6 files changed, 126 insertions(+), 50 deletions(-) diff --git a/deps/nghttp2/lib/CMakeLists.txt b/deps/nghttp2/lib/CMakeLists.txt index 0846d06789a..17e422b22db 100644 --- a/deps/nghttp2/lib/CMakeLists.txt +++ b/deps/nghttp2/lib/CMakeLists.txt @@ -49,7 +49,7 @@ target_include_directories(nghttp2 INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}/includes" ) -if(HAVE_CUNIT) +if(HAVE_CUNIT OR ENABLE_STATIC_LIB) # Static library (for unittests because of symbol visibility) add_library(nghttp2_static STATIC ${NGHTTP2_SOURCES}) set_target_properties(nghttp2_static PROPERTIES @@ -58,6 +58,10 @@ if(HAVE_CUNIT) ARCHIVE_OUTPUT_NAME nghttp2 ) target_compile_definitions(nghttp2_static PUBLIC "-DNGHTTP2_STATICLIB") + if(ENABLE_STATIC_LIB) + install(TARGETS nghttp2_static + DESTINATION "${CMAKE_INSTALL_LIBDIR}") + endif() endif() install(TARGETS nghttp2 diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h index 13cda9f29e2..14f8950bed8 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2.h @@ -3081,6 +3081,16 @@ NGHTTP2_EXTERN int nghttp2_session_set_stream_user_data(nghttp2_session *session, int32_t stream_id, void *stream_user_data); +/** + * @function + * + * Sets |user_data| to |session|, overwriting the existing user data + * specified in `nghttp2_session_client_new()`, or + * `nghttp2_session_server_new()`. + */ +NGHTTP2_EXTERN void nghttp2_session_set_user_data(nghttp2_session *session, + void *user_data); + /** * @function * diff --git a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h index 455706a5868..d32d2754441 100644 --- a/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h +++ b/deps/nghttp2/lib/includes/nghttp2/nghttp2ver.h @@ -29,7 +29,7 @@ * @macro * Version number of the nghttp2 library release */ -#define NGHTTP2_VERSION "1.29.0" +#define NGHTTP2_VERSION "1.32.0" /** * @macro @@ -37,6 +37,6 @@ * release. This is a 24 bit number with 8 bits for major number, 8 bits * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203. */ -#define NGHTTP2_VERSION_NUM 0x011d00 +#define NGHTTP2_VERSION_NUM 0x012000 #endif /* NGHTTP2VER_H */ diff --git a/deps/nghttp2/lib/nghttp2_frame.c b/deps/nghttp2/lib/nghttp2_frame.c index 210df058444..fa7cb6953bc 100644 --- a/deps/nghttp2/lib/nghttp2_frame.c +++ b/deps/nghttp2/lib/nghttp2_frame.c @@ -215,6 +215,9 @@ void nghttp2_frame_altsvc_free(nghttp2_extension *frame, nghttp2_mem *mem) { nghttp2_ext_altsvc *altsvc; altsvc = frame->payload; + if (altsvc == NULL) { + return; + } /* We use the same buffer for altsvc->origin and altsvc->field_value. */ nghttp2_mem_free(mem, altsvc->origin); diff --git a/deps/nghttp2/lib/nghttp2_http.c b/deps/nghttp2/lib/nghttp2_http.c index 8240f8d76d9..b08f8863f7c 100644 --- a/deps/nghttp2/lib/nghttp2_http.c +++ b/deps/nghttp2/lib/nghttp2_http.c @@ -244,7 +244,7 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_hd_nv *nv, return NGHTTP2_ERR_HTTP_HEADER; } stream->status_code = (int16_t)parse_uint(nv->value->base, nv->value->len); - if (stream->status_code == -1) { + if (stream->status_code == -1 || stream->status_code == 101) { return NGHTTP2_ERR_HTTP_HEADER; } break; diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c index b14ed77a25c..a9e7a62390e 100644 --- a/deps/nghttp2/lib/nghttp2_session.c +++ b/deps/nghttp2/lib/nghttp2_session.c @@ -219,6 +219,10 @@ static int session_terminate_session(nghttp2_session *session, return 0; } + /* Ignore all incoming frames because we are going to tear down the + session. */ + session->iframe.state = NGHTTP2_IB_IGN_ALL; + if (reason == NULL) { debug_data = NULL; debug_datalen = 0; @@ -2225,8 +2229,9 @@ static int session_prep_frame(nghttp2_session *session, assert(session->obq_flood_counter_ > 0); --session->obq_flood_counter_; } - - if (session_is_closing(session)) { + /* PING frame is allowed to be sent unless termination GOAWAY is + sent */ + if (session->goaway_flags & NGHTTP2_GOAWAY_TERM_ON_SEND) { return NGHTTP2_ERR_SESSION_CLOSING; } nghttp2_frame_pack_ping(&session->aob.framebufs, &frame->ping); @@ -5345,9 +5350,6 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (iframe->sbuf.pos[3] != NGHTTP2_SETTINGS || (iframe->sbuf.pos[4] & NGHTTP2_FLAG_ACK)) { - - iframe->state = NGHTTP2_IB_IGN_ALL; - rv = session_call_error_callback( session, NGHTTP2_ERR_SETTINGS_EXPECTED, "Remote peer returned unexpected data while we expected " @@ -5394,10 +5396,6 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, DEBUGF("recv: length is too large %zu > %u\n", iframe->frame.hd.length, session->local_settings.max_frame_size); - busy = 1; - - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_FRAME_SIZE_ERROR, "too large frame size"); @@ -5405,7 +5403,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } - break; + return (ssize_t)inlen; } switch (iframe->frame.hd.type) { @@ -5419,6 +5417,9 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; rv = session_on_data_received_fail_fast(session); + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } if (rv == NGHTTP2_ERR_IGN_PAYLOAD) { DEBUGF("recv: DATA not allowed stream_id=%d\n", iframe->frame.hd.stream_id); @@ -5432,7 +5433,6 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, rv = inbound_frame_handle_pad(iframe, &iframe->frame.hd); if (rv < 0) { - iframe->state = NGHTTP2_IB_IGN_DATA; rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_PROTOCOL_ERROR, "DATA: insufficient padding space"); @@ -5440,7 +5440,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } - break; + return (ssize_t)inlen; } if (rv == 1) { @@ -5461,17 +5461,13 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, rv = inbound_frame_handle_pad(iframe, &iframe->frame.hd); if (rv < 0) { - busy = 1; - - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_PROTOCOL_ERROR, "HEADERS: insufficient padding space"); if (nghttp2_is_fatal(rv)) { return rv; } - break; + return (ssize_t)inlen; } if (rv == 1) { @@ -5513,6 +5509,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { rv = nghttp2_session_add_rst_stream( session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR); @@ -5627,15 +5627,13 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, rv = inbound_frame_handle_pad(iframe, &iframe->frame.hd); if (rv < 0) { - busy = 1; - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_PROTOCOL_ERROR, "PUSH_PROMISE: insufficient padding space"); if (nghttp2_is_fatal(rv)) { return rv; } - break; + return (ssize_t)inlen; } if (rv == 1) { @@ -5695,11 +5693,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } - busy = 1; - - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - - break; + return (ssize_t)inlen; default: DEBUGF("recv: extension frame\n"); @@ -5769,6 +5763,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, case NGHTTP2_IB_IGN_PAYLOAD: case NGHTTP2_IB_FRAME_SIZE_ERROR: case NGHTTP2_IB_IGN_DATA: + case NGHTTP2_IB_IGN_ALL: break; default: rv = session_call_on_begin_frame(session, &iframe->frame.hd); @@ -5799,21 +5794,19 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, case NGHTTP2_HEADERS: if (iframe->padlen == 0 && (iframe->frame.hd.flags & NGHTTP2_FLAG_PADDED)) { + pri_fieldlen = nghttp2_frame_priority_len(iframe->frame.hd.flags); padlen = inbound_frame_compute_pad(iframe); - if (padlen < 0) { - busy = 1; + if (padlen < 0 || + (size_t)padlen + pri_fieldlen > 1 + iframe->payloadleft) { rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_PROTOCOL_ERROR, "HEADERS: invalid padding"); if (nghttp2_is_fatal(rv)) { return rv; } - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - break; + return (ssize_t)inlen; } iframe->frame.headers.padlen = (size_t)padlen; - pri_fieldlen = nghttp2_frame_priority_len(iframe->frame.hd.flags); - if (pri_fieldlen > 0) { if (iframe->payloadleft < pri_fieldlen) { busy = 1; @@ -5836,6 +5829,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { rv = nghttp2_session_add_rst_stream( session, iframe->frame.hd.stream_id, NGHTTP2_INTERNAL_ERROR); @@ -5860,6 +5857,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -5869,6 +5870,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -5876,16 +5881,15 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (iframe->padlen == 0 && (iframe->frame.hd.flags & NGHTTP2_FLAG_PADDED)) { padlen = inbound_frame_compute_pad(iframe); - if (padlen < 0) { - busy = 1; + if (padlen < 0 || (size_t)padlen + 4 /* promised stream id */ + > 1 + iframe->payloadleft) { rv = nghttp2_session_terminate_session_with_reason( session, NGHTTP2_PROTOCOL_ERROR, "PUSH_PROMISE: invalid padding"); if (nghttp2_is_fatal(rv)) { return rv; } - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - break; + return (ssize_t)inlen; } iframe->frame.push_promise.padlen = (size_t)padlen; @@ -5910,6 +5914,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, busy = 1; + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + if (rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { rv = nghttp2_session_add_rst_stream( session, iframe->frame.push_promise.promised_stream_id, @@ -5935,6 +5943,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -5966,6 +5978,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -6027,6 +6043,12 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, data_readlen = inbound_frame_effective_readlen( iframe, iframe->payloadleft - readlen, readlen); + + if (data_readlen == -1) { + /* everything is padding */ + data_readlen = 0; + } + trail_padlen = nghttp2_frame_trail_padlen(&iframe->frame, iframe->padlen); final = (iframe->frame.hd.flags & NGHTTP2_FLAG_END_HEADERS) && @@ -6046,6 +6068,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + if (rv == NGHTTP2_ERR_PAUSE) { in += hd_proclen; iframe->payloadleft -= hd_proclen; @@ -6155,11 +6181,9 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } - busy = 1; - - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; + assert(iframe->state == NGHTTP2_IB_IGN_ALL); - break; + return (ssize_t)inlen; case NGHTTP2_IB_READ_SETTINGS: DEBUGF("recv: [IB_READ_SETTINGS]\n"); @@ -6188,6 +6212,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -6218,6 +6246,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + session_inbound_frame_reset(session); break; @@ -6257,11 +6289,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } - busy = 1; - - iframe->state = NGHTTP2_IB_IGN_PAYLOAD; - - break; + return (ssize_t)inlen; } /* CONTINUATION won't bear NGHTTP2_PADDED flag */ @@ -6305,6 +6333,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + /* Pad Length field is consumed immediately */ rv = nghttp2_session_consume(session, iframe->frame.hd.stream_id, readlen); @@ -6313,6 +6345,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + stream = nghttp2_session_get_stream(session, iframe->frame.hd.stream_id); if (stream) { rv = session_update_recv_stream_window_size( @@ -6333,8 +6369,7 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } - iframe->state = NGHTTP2_IB_IGN_DATA; - break; + return (ssize_t)inlen; } iframe->frame.data.padlen = (size_t)padlen; @@ -6368,6 +6403,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + rv = session_update_recv_stream_window_size( session, stream, readlen, iframe->payloadleft || @@ -6394,6 +6433,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } + + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } } DEBUGF("recv: data_readlen=%zd\n", data_readlen); @@ -6409,6 +6452,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } + + if (iframe->state == NGHTTP2_IB_IGN_DATA) { + return (ssize_t)inlen; + } } rv = nghttp2_session_add_rst_stream( @@ -6466,6 +6513,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, return rv; } + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } + if (session->opt_flags & NGHTTP2_OPTMASK_NO_AUTO_WINDOW_UPDATE) { /* Ignored DATA is considered as "consumed" immediately. */ @@ -6474,6 +6525,10 @@ ssize_t nghttp2_session_mem_recv(nghttp2_session *session, const uint8_t *in, if (nghttp2_is_fatal(rv)) { return rv; } + + if (iframe->state == NGHTTP2_IB_IGN_ALL) { + return (ssize_t)inlen; + } } } @@ -7520,3 +7575,7 @@ size_t nghttp2_session_get_hd_deflate_dynamic_table_size(nghttp2_session *session) { return nghttp2_hd_deflate_get_dynamic_table_size(&session->hd_deflater); } + +void nghttp2_session_set_user_data(nghttp2_session *session, void *user_data) { + session->user_data = user_data; +} From ae5567eaea7e53f333f6b442944d57bb9c5a6669 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Fri, 13 Apr 2018 08:11:26 -0700 Subject: [PATCH 16/17] test: add regression test for nghttp2 CVE-2018-1000168 PR-URL: https://github.com/nodejs-private/node-private/pull/117 Reviewed-By: Matteo Collina Reviewed-By: Daniel Bevenius Reviewed-By: Colin Ihrig Reviewed-By: Evan Lucas --- test/common/http2.js | 10 +++++ test/parallel/test-http2-malformed-altsvc.js | 39 ++++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 test/parallel/test-http2-malformed-altsvc.js diff --git a/test/common/http2.js b/test/common/http2.js index 0f3378e9b80..f84a6686175 100644 --- a/test/common/http2.js +++ b/test/common/http2.js @@ -127,8 +127,18 @@ class PingFrame extends Frame { } } +class AltSvcFrame extends Frame { + constructor(size) { + const buffers = [Buffer.alloc(size)]; + super(size, 10, 0, 0); + buffers.unshift(this[kFrameData]); + this[kFrameData] = Buffer.concat(buffers); + } +} + module.exports = { Frame, + AltSvcFrame, DataFrame, HeadersFrame, SettingsFrame, diff --git a/test/parallel/test-http2-malformed-altsvc.js b/test/parallel/test-http2-malformed-altsvc.js new file mode 100644 index 00000000000..28c0fb46b42 --- /dev/null +++ b/test/parallel/test-http2-malformed-altsvc.js @@ -0,0 +1,39 @@ +'use strict'; + +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const http2 = require('http2'); +const net = require('net'); +const h2test = require('../common/http2'); + +const server = http2.createServer(); +server.on('stream', common.mustNotCall()); + +const settings = new h2test.SettingsFrame(); +const settingsAck = new h2test.SettingsFrame(true); +const altsvc = new h2test.AltSvcFrame((1 << 14) + 1); + +server.listen(0, () => { + const client = net.connect(server.address().port, () => { + client.write(h2test.kClientMagic, () => { + client.write(settings.data, () => { + client.write(settingsAck.data); + // Prior to nghttp2 1.31.1, sending this malformed altsvc frame + // would cause a segfault. This test is successful if a segfault + // does not occur. + client.write(altsvc.data, common.mustCall(() => { + client.destroy(); + })); + }); + }); + }); + + // An error may or may not be emitted on the client side, we don't care + // either way if it is, but we don't want to die if it is. + client.on('error', () => {}); + client.on('close', common.mustCall(() => server.close())); + client.resume(); +}); From 1442cfef73c0a7157b1552658529ae9ff9154de9 Mon Sep 17 00:00:00 2001 From: Evan Lucas Date: Tue, 12 Jun 2018 05:30:54 -0500 Subject: [PATCH 17/17] 2018-06-12, Version 10.4.1 (Current) Notable changes: * **Fixes memory exhaustion DoS** (CVE-2018-7164): Fixes a bug introduced in 9.7.0 that increases the memory consumed when reading from the network into JavaScript using the net.Socket object directly as a stream. * **http2** * (CVE-2018-7161): Fixes Denial of Service vulnerability by updating the http2 implementation to not crash under certain circumstances during cleanup * (CVE-2018-1000168): Fixes Denial of Service vulnerability by upgrading nghttp2 to 1.32.0 * **tls** (CVE-2018-7162): Fixes Denial of Service vulnerability by updating the TLS implementation to not crash upon receiving * **n-api**: Prevent use-after-free in napi_delete_async_work PR-URL: https://github.com/nodejs-private/node-private/pull/136 --- CHANGELOG.md | 3 ++- doc/changelogs/CHANGELOG_V10.md | 31 +++++++++++++++++++++++++++++++ src/node_version.h | 2 +- 3 files changed, 34 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3c26a907b6..d676fb51d23 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,8 @@ release. -10.4.0
+10.4.1
+10.4.0
10.3.0
10.2.1
10.2.0
diff --git a/doc/changelogs/CHANGELOG_V10.md b/doc/changelogs/CHANGELOG_V10.md index 5487e7b30db..de3d7a18eeb 100644 --- a/doc/changelogs/CHANGELOG_V10.md +++ b/doc/changelogs/CHANGELOG_V10.md @@ -9,6 +9,7 @@ +10.4.1
10.4.0
10.3.0
10.2.1
@@ -31,6 +32,36 @@ * [io.js](CHANGELOG_IOJS.md) * [Archive](CHANGELOG_ARCHIVE.md) + +## 2018-06-12, Version 10.4.1 (Current), @evanlucas + +### Notable Changes + +* **Fixes memory exhaustion DoS** (CVE-2018-7164): Fixes a bug introduced in 9.7.0 that increases the memory consumed when reading from the network into JavaScript using the net.Socket object directly as a stream. +* **http2** + * (CVE-2018-7161): Fixes Denial of Service vulnerability by updating the http2 implementation to not crash under certain circumstances during cleanup + * (CVE-2018-1000168): Fixes Denial of Service vulnerability by upgrading nghttp2 to 1.32.0 +* **tls** (CVE-2018-7162): Fixes Denial of Service vulnerability by updating the TLS implementation to not crash upon receiving +* **n-api**: Prevent use-after-free in napi_delete_async_work + +### Commits + +* [[`1bbfe9a72b`](https://github.com/nodejs/node/commit/1bbfe9a72b)] - **build**: fix configure script for double-digits (Misty De Meo) [#21183](https://github.com/nodejs/node/pull/21183) +* [[`4c90ee8fc6`](https://github.com/nodejs/node/commit/4c90ee8fc6)] - **deps**: update to nghttp2 1.32.0 (James M Snell) [nodejs-private/node-private#117](https://github.com/nodejs-private/node-private/pull/117) +* [[`e5c2f575b1`](https://github.com/nodejs/node/commit/e5c2f575b1)] - **deps**: patch V8 to 6.7.288.45 (Michaël Zasso) [#21192](https://github.com/nodejs/node/pull/21192) +* [[`03ded94ffe`](https://github.com/nodejs/node/commit/03ded94ffe)] - **deps**: patch V8 to 6.7.288.44 (Michaël Zasso) [#21146](https://github.com/nodejs/node/pull/21146) +* [[`4de7e0c96c`](https://github.com/nodejs/node/commit/4de7e0c96c)] - **deps,npm**: float node-gyp patch on npm (Rich Trott) [#21239](https://github.com/nodejs/node/pull/21239) +* [[`92d7b6c9a0`](https://github.com/nodejs/node/commit/92d7b6c9a0)] - **fs**: fix promises reads with pos \> 4GB (cjihrig) [#21148](https://github.com/nodejs/node/pull/21148) +* [[`8681402228`](https://github.com/nodejs/node/commit/8681402228)] - **http2**: fixup http2stream cleanup and other nits (James M Snell) [nodejs-private/node-private#115](https://github.com/nodejs-private/node-private/pull/115) +* [[`53f8563353`](https://github.com/nodejs/node/commit/53f8563353)] - **n-api**: back up env before async work finalize (Gabriel Schulhof) [#21129](https://github.com/nodejs/node/pull/21129) +* [[`9ba8ed1371`](https://github.com/nodejs/node/commit/9ba8ed1371)] - **src**: re-add `Realloc()` shrink after reading stream data (Anna Henningsen) [nodejs-private/node-private#128](https://github.com/nodejs-private/node-private/pull/128) +* [[`8e979482fa`](https://github.com/nodejs/node/commit/8e979482fa)] - ***Revert*** "**src**: restore stdio on program exit" (Evan Lucas) [#21257](https://github.com/nodejs/node/pull/21257) +* [[`cb5ec64956`](https://github.com/nodejs/node/commit/cb5ec64956)] - **src**: reset TTY mode before cleaning up resources (Anna Henningsen) [#21257](https://github.com/nodejs/node/pull/21257) +* [[`ae5567eaea`](https://github.com/nodejs/node/commit/ae5567eaea)] - **test**: add regression test for nghttp2 CVE-2018-1000168 (James M Snell) [nodejs-private/node-private#117](https://github.com/nodejs-private/node-private/pull/117) +* [[`e87bf625dd`](https://github.com/nodejs/node/commit/e87bf625dd)] - **test**: add tls write error regression test (Shigeki Ohtsu) [nodejs-private/node-private#127](https://github.com/nodejs-private/node-private/pull/127) +* [[`eea2bce58d`](https://github.com/nodejs/node/commit/eea2bce58d)] - **tls**: fix SSL write error handling (Anna Henningsen) [nodejs-private/node-private#127](https://github.com/nodejs-private/node-private/pull/127) +* [[`1e49eadd68`](https://github.com/nodejs/node/commit/1e49eadd68)] - **tools,gyp**: fix regex for version matching (Rich Trott) [#21216](https://github.com/nodejs/node/pull/21216) + ## 2018-06-06, Version 10.4.0 (Current), @MylesBorins diff --git a/src/node_version.h b/src/node_version.h index 47ff6d71793..4b4452d3e88 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -29,7 +29,7 @@ #define NODE_VERSION_IS_LTS 0 #define NODE_VERSION_LTS_CODENAME "" -#define NODE_VERSION_IS_RELEASE 0 +#define NODE_VERSION_IS_RELEASE 1 #ifndef NODE_STRINGIFY #define NODE_STRINGIFY(n) NODE_STRINGIFY_HELPER(n)