From 59358182e2d30c60858cd42a9b3ba4ace23736ce Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Sep 2016 16:43:08 +0200 Subject: [PATCH 1/4] src: add Malloc() size param + overflow detection Adds an optional second parameter to `node::Malloc()` and an optional third parameter to `node::Realloc()` giving the size/number of items to be allocated, in the style of `calloc(3)`. Use a proper overflow check using division; the previous `CHECK_GE(n * size, n);` would not detect all cases of overflow (e.g. `size == SIZE_MAX / 2 && n == 3`). PR-URL: https://github.com/nodejs/node/pull/8482 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Ilkka Myller --- src/node_crypto.cc | 5 ++--- src/string_bytes.cc | 2 +- src/util-inl.h | 24 ++++++++++++++++++------ src/util.h | 11 ++++------- 4 files changed, 25 insertions(+), 17 deletions(-) diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 99ed0ddf0808bb..fefd471c6b7eda 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -5826,11 +5826,10 @@ void GetCurves(const FunctionCallbackInfo& args) { const size_t num_curves = EC_get_builtin_curves(nullptr, 0); Local arr = Array::New(env->isolate(), num_curves); EC_builtin_curve* curves; - size_t alloc_size; if (num_curves) { - alloc_size = sizeof(*curves) * num_curves; - curves = static_cast(node::Malloc(alloc_size)); + curves = static_cast(node::Malloc(sizeof(*curves), + num_curves)); CHECK_NE(curves, nullptr); diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 9d1619d864b495..771ef034004b83 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -54,7 +54,7 @@ class ExternString: public ResourceType { return scope.Escape(String::Empty(isolate)); TypeName* new_data = - static_cast(node::Malloc(length * sizeof(*new_data))); + static_cast(node::Malloc(length, sizeof(*new_data))); if (new_data == nullptr) { return Local(); } diff --git a/src/util-inl.h b/src/util-inl.h index 27bced48fe2198..8f23a59651b2f7 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -320,6 +320,14 @@ bool StringEqualNoCaseN(const char* a, const char* b, size_t length) { return true; } +inline size_t MultiplyWithOverflowCheck(size_t a, size_t b) { + size_t ret = a * b; + if (a != 0) + CHECK_EQ(b, ret / a); + + return ret; +} + // These should be used in our code as opposed to the native // versions as they abstract out some platform and or // compiler version specific functionality. @@ -327,24 +335,28 @@ bool StringEqualNoCaseN(const char* a, const char* b, size_t length) { // that the standard allows them to either return a unique pointer or a // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. -void* Realloc(void* pointer, size_t size) { - if (size == 0) { +void* Realloc(void* pointer, size_t n, size_t size) { + size_t full_size = MultiplyWithOverflowCheck(size, n); + + if (full_size == 0) { free(pointer); return nullptr; } - return realloc(pointer, size); + + return realloc(pointer, full_size); } // As per spec realloc behaves like malloc if passed nullptr. -void* Malloc(size_t size) { +void* Malloc(size_t n, size_t size) { + if (n == 0) n = 1; if (size == 0) size = 1; - return Realloc(nullptr, size); + return Realloc(nullptr, n, size); } void* Calloc(size_t n, size_t size) { if (n == 0) n = 1; if (size == 0) size = 1; - CHECK_GE(n * size, n); // Overflow guard. + MultiplyWithOverflowCheck(size, n); return calloc(n, size); } diff --git a/src/util.h b/src/util.h index f415141a58e997..38c17b390e1fab 100644 --- a/src/util.h +++ b/src/util.h @@ -31,9 +31,9 @@ namespace node { // that the standard allows them to either return a unique pointer or a // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. -inline void* Realloc(void* pointer, size_t size); -inline void* Malloc(size_t size); -inline void* Calloc(size_t n, size_t size); +inline void* Realloc(void* pointer, size_t n, size_t size = 1); +inline void* Malloc(size_t n, size_t size = 1); +inline void* Calloc(size_t n, size_t size = 1); #ifdef __GNUC__ #define NO_RETURN __attribute__((noreturn)) @@ -294,10 +294,7 @@ class MaybeStackBuffer { if (storage <= kStackStorageSize) { buf_ = buf_st_; } else { - // Guard against overflow. - CHECK_LE(storage, sizeof(T) * storage); - - buf_ = static_cast(Malloc(sizeof(T) * storage)); + buf_ = static_cast(Malloc(sizeof(T), storage)); CHECK_NE(buf_, nullptr); } From e0a53288a8f82c6ec5fba00d4897fc08c7599c21 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Sep 2016 17:50:02 +0200 Subject: [PATCH 2/4] src: pass desired return type to allocators Pass the desired return type directly to the allocation functions, so that the resulting `static_cast` from `void*` becomes unneccessary and the return type can be use as a reasonable default value for the `size` parameter. PR-URL: https://github.com/nodejs/node/pull/8482 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Ilkka Myller --- src/cares_wrap.cc | 17 +++++++---------- src/node.cc | 2 +- src/node_buffer.cc | 20 +++++++++++++------- src/node_crypto.cc | 21 ++++++++++----------- src/stream_wrap.cc | 4 ++-- src/string_bytes.cc | 9 ++++----- src/tls_wrap.cc | 2 +- src/udp_wrap.cc | 4 ++-- src/util-inl.h | 21 +++++++++++---------- src/util.h | 15 +++++++++++---- test/cctest/test_util.cc | 10 ++++++---- 11 files changed, 68 insertions(+), 57 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index d40d4b3256f193..86afbd681e0ab8 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -175,8 +175,7 @@ static void ares_poll_close_cb(uv_handle_t* watcher) { /* Allocates and returns a new node_ares_task */ static node_ares_task* ares_task_create(Environment* env, ares_socket_t sock) { - node_ares_task* task = - static_cast(node::Malloc(sizeof(*task))); + auto task = node::Malloc(1); if (task == nullptr) { /* Out of memory. */ @@ -329,11 +328,10 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { alias_count++) { } - dest->h_aliases = static_cast(node::Malloc((alias_count + 1) * - sizeof(char*))); + dest->h_aliases = node::Malloc(alias_count + 1); for (size_t i = 0; i < alias_count; i++) { cur_alias_length = strlen(src->h_aliases[i]); - dest->h_aliases[i] = static_cast(node::Malloc(cur_alias_length + 1)); + dest->h_aliases[i] = node::Malloc(cur_alias_length + 1); memcpy(dest->h_aliases[i], src->h_aliases[i], cur_alias_length + 1); } dest->h_aliases[alias_count] = nullptr; @@ -345,10 +343,9 @@ void cares_wrap_hostent_cpy(struct hostent* dest, struct hostent* src) { list_count++) { } - dest->h_addr_list = static_cast(node::Malloc((list_count + 1) * - sizeof(char*))); + dest->h_addr_list = node::Malloc(list_count + 1); for (size_t i = 0; i < list_count; i++) { - dest->h_addr_list[i] = static_cast(node::Malloc(src->h_length)); + dest->h_addr_list[i] = node::Malloc(src->h_length); memcpy(dest->h_addr_list[i], src->h_addr_list[i], src->h_length); } dest->h_addr_list[list_count] = nullptr; @@ -507,7 +504,7 @@ class QueryWrap : public AsyncWrap { unsigned char* buf_copy = nullptr; if (status == ARES_SUCCESS) { - buf_copy = static_cast(node::Malloc(answer_len)); + buf_copy = node::Malloc(answer_len); memcpy(buf_copy, answer_buf, answer_len); } @@ -534,7 +531,7 @@ class QueryWrap : public AsyncWrap { struct hostent* host_copy = nullptr; if (status == ARES_SUCCESS) { - host_copy = static_cast(node::Malloc(sizeof(hostent))); + host_copy = node::Malloc(1); cares_wrap_hostent_cpy(host_copy, host); } diff --git a/src/node.cc b/src/node.cc index f4218ca6795933..731f33495e039c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1054,7 +1054,7 @@ void* ArrayBufferAllocator::Allocate(size_t size) { if (env_ == nullptr || !env_->array_buffer_allocator_info()->no_zero_fill() || zero_fill_all_buffers) - return node::Calloc(size, 1); + return node::Calloc(size); env_->array_buffer_allocator_info()->reset_fill_flag(); return node::Malloc(size); } diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 9a7ee754d4f532..f06b00318ddaa6 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -48,14 +48,20 @@ THROW_AND_RETURN_IF_OOB(end <= end_max); \ size_t length = end - start; -#define BUFFER_MALLOC(length) \ - zero_fill_all_buffers ? node::Calloc(length, 1) : node::Malloc(length) - namespace node { // if true, all Buffer and SlowBuffer instances will automatically zero-fill bool zero_fill_all_buffers = false; +namespace { + +inline void* BufferMalloc(size_t length) { + return zero_fill_all_buffers ? node::Calloc(length) : + node::Malloc(length); +} + +} // namespace + namespace Buffer { using v8::ArrayBuffer; @@ -234,7 +240,7 @@ MaybeLocal New(Isolate* isolate, char* data = nullptr; if (length > 0) { - data = static_cast(BUFFER_MALLOC(length)); + data = static_cast(BufferMalloc(length)); if (data == nullptr) return Local(); @@ -246,7 +252,7 @@ MaybeLocal New(Isolate* isolate, free(data); data = nullptr; } else if (actual < length) { - data = static_cast(node::Realloc(data, actual)); + data = node::Realloc(data, actual); CHECK_NE(data, nullptr); } } @@ -280,7 +286,7 @@ MaybeLocal New(Environment* env, size_t length) { void* data; if (length > 0) { - data = BUFFER_MALLOC(length); + data = BufferMalloc(length); if (data == nullptr) return Local(); } else { @@ -1063,7 +1069,7 @@ void IndexOfString(const FunctionCallbackInfo& args) { offset, is_forward); } else if (enc == LATIN1) { - uint8_t* needle_data = static_cast(node::Malloc(needle_length)); + uint8_t* needle_data = node::Malloc(needle_length); if (needle_data == nullptr) { return args.GetReturnValue().Set(-1); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index fefd471c6b7eda..e1ae4d893b2108 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2386,7 +2386,7 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { size_t len = Buffer::Length(obj); // OpenSSL takes control of the pointer after accepting it - char* data = reinterpret_cast(node::Malloc(len)); + char* data = node::Malloc(len); CHECK_NE(data, nullptr); memcpy(data, resp, len); @@ -3466,7 +3466,7 @@ bool CipherBase::GetAuthTag(char** out, unsigned int* out_len) const { if (initialised_ || kind_ != kCipher || !auth_tag_) return false; *out_len = auth_tag_len_; - *out = static_cast(node::Malloc(auth_tag_len_)); + *out = node::Malloc(auth_tag_len_); CHECK_NE(*out, nullptr); memcpy(*out, auth_tag_, auth_tag_len_); return true; @@ -5138,7 +5138,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { // NOTE: field_size is in bits int field_size = EC_GROUP_get_degree(ecdh->group_); size_t out_len = (field_size + 7) / 8; - char* out = static_cast(node::Malloc(out_len)); + char* out = node::Malloc(out_len); CHECK_NE(out, nullptr); int r = ECDH_compute_key(out, out_len, pub, ecdh->key_, nullptr); @@ -5174,7 +5174,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { if (size == 0) return env->ThrowError("Failed to get public key length"); - unsigned char* out = static_cast(node::Malloc(size)); + unsigned char* out = node::Malloc(size); CHECK_NE(out, nullptr); int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr); @@ -5200,7 +5200,7 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to get ECDH private key"); int size = BN_num_bytes(b); - unsigned char* out = static_cast(node::Malloc(size)); + unsigned char* out = node::Malloc(size); CHECK_NE(out, nullptr); if (size != BN_bn2bin(b, out)) { @@ -5333,7 +5333,7 @@ class PBKDF2Request : public AsyncWrap { saltlen_(saltlen), salt_(salt), keylen_(keylen), - key_(static_cast(node::Malloc(keylen))), + key_(node::Malloc(keylen)), iter_(iter) { if (key() == nullptr) FatalError("node::PBKDF2Request()", "Out of Memory"); @@ -5496,7 +5496,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Salt"); - pass = static_cast(node::Malloc(passlen)); + pass = node::Malloc(passlen); if (pass == nullptr) { FatalError("node::PBKDF2()", "Out of Memory"); } @@ -5508,7 +5508,7 @@ void PBKDF2(const FunctionCallbackInfo& args) { goto err; } - salt = static_cast(node::Malloc(saltlen)); + salt = node::Malloc(saltlen); if (salt == nullptr) { FatalError("node::PBKDF2()", "Out of Memory"); } @@ -5601,7 +5601,7 @@ class RandomBytesRequest : public AsyncWrap { : AsyncWrap(env, object, AsyncWrap::PROVIDER_CRYPTO), error_(0), size_(size), - data_(static_cast(node::Malloc(size))) { + data_(node::Malloc(size)) { if (data() == nullptr) FatalError("node::RandomBytesRequest()", "Out of Memory"); Wrap(object, this); @@ -5828,8 +5828,7 @@ void GetCurves(const FunctionCallbackInfo& args) { EC_builtin_curve* curves; if (num_curves) { - curves = static_cast(node::Malloc(sizeof(*curves), - num_curves)); + curves = node::Malloc(num_curves); CHECK_NE(curves, nullptr); diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index 7709e24a6b4d93..f5bc4ad8c4eca3 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -148,7 +148,7 @@ void StreamWrap::OnAlloc(uv_handle_t* handle, void StreamWrap::OnAllocImpl(size_t size, uv_buf_t* buf, void* ctx) { - buf->base = static_cast(node::Malloc(size)); + buf->base = node::Malloc(size); buf->len = size; if (buf->base == nullptr && size > 0) { @@ -204,7 +204,7 @@ void StreamWrap::OnReadImpl(ssize_t nread, return; } - char* base = static_cast(node::Realloc(buf->base, nread)); + char* base = node::Realloc(buf->base, nread); CHECK_LE(static_cast(nread), buf->len); if (pending == UV_TCP) { diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 771ef034004b83..065a8ece15a06c 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -53,8 +53,7 @@ class ExternString: public ResourceType { if (length == 0) return scope.Escape(String::Empty(isolate)); - TypeName* new_data = - static_cast(node::Malloc(length, sizeof(*new_data))); + TypeName* new_data = node::Malloc(length); if (new_data == nullptr) { return Local(); } @@ -610,7 +609,7 @@ Local StringBytes::Encode(Isolate* isolate, case ASCII: if (contains_non_ascii(buf, buflen)) { - char* out = static_cast(node::Malloc(buflen)); + char* out = node::Malloc(buflen); if (out == nullptr) { return Local(); } @@ -645,7 +644,7 @@ Local StringBytes::Encode(Isolate* isolate, case BASE64: { size_t dlen = base64_encoded_size(buflen); - char* dst = static_cast(node::Malloc(dlen)); + char* dst = node::Malloc(dlen); if (dst == nullptr) { return Local(); } @@ -664,7 +663,7 @@ Local StringBytes::Encode(Isolate* isolate, case HEX: { size_t dlen = buflen * 2; - char* dst = static_cast(node::Malloc(dlen)); + char* dst = node::Malloc(dlen); if (dst == nullptr) { return Local(); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 1f1e1eeb2d8169..4b8a575d56f2d8 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -663,7 +663,7 @@ void TLSWrap::OnDestructImpl(void* ctx) { void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) { - buf->base = static_cast(node::Malloc(suggested_size)); + buf->base = node::Malloc(suggested_size); CHECK_NE(buf->base, nullptr); buf->len = suggested_size; } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 6e6c46aad00939..43378199fe0188 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -373,7 +373,7 @@ void UDPWrap::OnSend(uv_udp_send_t* req, int status) { void UDPWrap::OnAlloc(uv_handle_t* handle, size_t suggested_size, uv_buf_t* buf) { - buf->base = static_cast(node::Malloc(suggested_size)); + buf->base = node::Malloc(suggested_size); buf->len = suggested_size; if (buf->base == nullptr && suggested_size > 0) { @@ -415,7 +415,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, return; } - char* base = static_cast(node::Realloc(buf->base, nread)); + char* base = node::Realloc(buf->base, nread); argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); diff --git a/src/util-inl.h b/src/util-inl.h index 8f23a59651b2f7..7d4eda49152b16 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -335,29 +335,30 @@ inline size_t MultiplyWithOverflowCheck(size_t a, size_t b) { // that the standard allows them to either return a unique pointer or a // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. -void* Realloc(void* pointer, size_t n, size_t size) { - size_t full_size = MultiplyWithOverflowCheck(size, n); +template +T* Realloc(T* pointer, size_t n) { + size_t full_size = MultiplyWithOverflowCheck(sizeof(T), n); if (full_size == 0) { free(pointer); return nullptr; } - return realloc(pointer, full_size); + return static_cast(realloc(pointer, full_size)); } // As per spec realloc behaves like malloc if passed nullptr. -void* Malloc(size_t n, size_t size) { +template +T* Malloc(size_t n) { if (n == 0) n = 1; - if (size == 0) size = 1; - return Realloc(nullptr, n, size); + return Realloc(nullptr, n); } -void* Calloc(size_t n, size_t size) { +template +T* Calloc(size_t n) { if (n == 0) n = 1; - if (size == 0) size = 1; - MultiplyWithOverflowCheck(size, n); - return calloc(n, size); + MultiplyWithOverflowCheck(sizeof(T), n); + return static_cast(calloc(n, sizeof(T))); } } // namespace node diff --git a/src/util.h b/src/util.h index 38c17b390e1fab..59a26fb8527735 100644 --- a/src/util.h +++ b/src/util.h @@ -31,9 +31,16 @@ namespace node { // that the standard allows them to either return a unique pointer or a // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. -inline void* Realloc(void* pointer, size_t n, size_t size = 1); -inline void* Malloc(size_t n, size_t size = 1); -inline void* Calloc(size_t n, size_t size = 1); +template +inline T* Realloc(T* pointer, size_t n); +template +inline T* Malloc(size_t n); +template +inline T* Calloc(size_t n); + +// Shortcuts for char*. +inline char* Malloc(size_t n) { return Malloc(n); } +inline char* Calloc(size_t n) { return Calloc(n); } #ifdef __GNUC__ #define NO_RETURN __attribute__((noreturn)) @@ -294,7 +301,7 @@ class MaybeStackBuffer { if (storage <= kStackStorageSize) { buf_ = buf_st_; } else { - buf_ = static_cast(Malloc(sizeof(T), storage)); + buf_ = Malloc(storage); CHECK_NE(buf_, nullptr); } diff --git a/test/cctest/test_util.cc b/test/cctest/test_util.cc index 65a382bd3893fa..7bbf53af13d3c4 100644 --- a/test/cctest/test_util.cc +++ b/test/cctest/test_util.cc @@ -92,14 +92,16 @@ TEST(UtilTest, ToLower) { TEST(UtilTest, Malloc) { using node::Malloc; + EXPECT_NE(nullptr, Malloc(0)); + EXPECT_NE(nullptr, Malloc(1)); EXPECT_NE(nullptr, Malloc(0)); EXPECT_NE(nullptr, Malloc(1)); } TEST(UtilTest, Calloc) { using node::Calloc; - EXPECT_NE(nullptr, Calloc(0, 0)); - EXPECT_NE(nullptr, Calloc(1, 0)); - EXPECT_NE(nullptr, Calloc(0, 1)); - EXPECT_NE(nullptr, Calloc(1, 1)); + EXPECT_NE(nullptr, Calloc(0)); + EXPECT_NE(nullptr, Calloc(1)); + EXPECT_NE(nullptr, Calloc(0)); + EXPECT_NE(nullptr, Calloc(1)); } From 28af6bbe4a0715e0ad4d8a6ce09d3148751e1fad Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Sep 2016 18:19:24 +0200 Subject: [PATCH 3/4] src: provide allocation + nullptr check shortcuts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Provide shortcut `node::CheckedMalloc()` and friends that replace `node::Malloc()` + `CHECK_NE(ยท, nullptr);` combinations in a few places. PR-URL: https://github.com/nodejs/node/pull/8482 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Ilkka Myller --- src/cares_wrap.cc | 2 +- src/node.cc | 4 ++-- src/node_buffer.cc | 9 ++++----- src/node_crypto.cc | 17 ----------------- src/node_internals.h | 2 +- src/stream_wrap.cc | 8 +------- src/string_bytes.cc | 8 ++++---- src/tls_wrap.cc | 1 - src/udp_wrap.cc | 7 +------ src/util-inl.h | 29 +++++++++++++++++++++++++---- src/util.cc | 1 + src/util.h | 12 +++++++++++- test/cctest/test_util.cc | 16 ++++++++++++++++ 13 files changed, 67 insertions(+), 49 deletions(-) diff --git a/src/cares_wrap.cc b/src/cares_wrap.cc index 86afbd681e0ab8..638daef76226ca 100644 --- a/src/cares_wrap.cc +++ b/src/cares_wrap.cc @@ -175,7 +175,7 @@ static void ares_poll_close_cb(uv_handle_t* watcher) { /* Allocates and returns a new node_ares_task */ static node_ares_task* ares_task_create(Environment* env, ares_socket_t sock) { - auto task = node::Malloc(1); + auto task = node::UncheckedMalloc(1); if (task == nullptr) { /* Out of memory. */ diff --git a/src/node.cc b/src/node.cc index 731f33495e039c..6345abdb312dda 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1054,9 +1054,9 @@ void* ArrayBufferAllocator::Allocate(size_t size) { if (env_ == nullptr || !env_->array_buffer_allocator_info()->no_zero_fill() || zero_fill_all_buffers) - return node::Calloc(size); + return node::UncheckedCalloc(size); env_->array_buffer_allocator_info()->reset_fill_flag(); - return node::Malloc(size); + return node::UncheckedMalloc(size); } static bool DomainHasErrorHandler(const Environment* env, diff --git a/src/node_buffer.cc b/src/node_buffer.cc index f06b00318ddaa6..5e6de043ee6006 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -56,8 +56,8 @@ bool zero_fill_all_buffers = false; namespace { inline void* BufferMalloc(size_t length) { - return zero_fill_all_buffers ? node::Calloc(length) : - node::Malloc(length); + return zero_fill_all_buffers ? node::UncheckedCalloc(length) : + node::UncheckedMalloc(length); } } // namespace @@ -253,7 +253,6 @@ MaybeLocal New(Isolate* isolate, data = nullptr; } else if (actual < length) { data = node::Realloc(data, actual); - CHECK_NE(data, nullptr); } } @@ -331,7 +330,7 @@ MaybeLocal Copy(Environment* env, const char* data, size_t length) { void* new_data; if (length > 0) { CHECK_NE(data, nullptr); - new_data = node::Malloc(length); + new_data = node::UncheckedMalloc(length); if (new_data == nullptr) return Local(); memcpy(new_data, data, length); @@ -1069,7 +1068,7 @@ void IndexOfString(const FunctionCallbackInfo& args) { offset, is_forward); } else if (enc == LATIN1) { - uint8_t* needle_data = node::Malloc(needle_length); + uint8_t* needle_data = node::UncheckedMalloc(needle_length); if (needle_data == nullptr) { return args.GetReturnValue().Set(-1); } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index e1ae4d893b2108..09002972a94b5f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -2387,7 +2387,6 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { // OpenSSL takes control of the pointer after accepting it char* data = node::Malloc(len); - CHECK_NE(data, nullptr); memcpy(data, resp, len); if (!SSL_set_tlsext_status_ocsp_resp(s, data, len)) @@ -3467,7 +3466,6 @@ bool CipherBase::GetAuthTag(char** out, unsigned int* out_len) const { return false; *out_len = auth_tag_len_; *out = node::Malloc(auth_tag_len_); - CHECK_NE(*out, nullptr); memcpy(*out, auth_tag_, auth_tag_len_); return true; } @@ -5139,7 +5137,6 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo& args) { int field_size = EC_GROUP_get_degree(ecdh->group_); size_t out_len = (field_size + 7) / 8; char* out = node::Malloc(out_len); - CHECK_NE(out, nullptr); int r = ECDH_compute_key(out, out_len, pub, ecdh->key_, nullptr); EC_POINT_free(pub); @@ -5175,7 +5172,6 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo& args) { return env->ThrowError("Failed to get public key length"); unsigned char* out = node::Malloc(size); - CHECK_NE(out, nullptr); int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr); if (r != size) { @@ -5201,7 +5197,6 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo& args) { int size = BN_num_bytes(b); unsigned char* out = node::Malloc(size); - CHECK_NE(out, nullptr); if (size != BN_bn2bin(b, out)) { free(out); @@ -5335,8 +5330,6 @@ class PBKDF2Request : public AsyncWrap { keylen_(keylen), key_(node::Malloc(keylen)), iter_(iter) { - if (key() == nullptr) - FatalError("node::PBKDF2Request()", "Out of Memory"); Wrap(object, this); } @@ -5497,9 +5490,6 @@ void PBKDF2(const FunctionCallbackInfo& args) { THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Salt"); pass = node::Malloc(passlen); - if (pass == nullptr) { - FatalError("node::PBKDF2()", "Out of Memory"); - } memcpy(pass, Buffer::Data(args[0]), passlen); saltlen = Buffer::Length(args[1]); @@ -5509,9 +5499,6 @@ void PBKDF2(const FunctionCallbackInfo& args) { } salt = node::Malloc(saltlen); - if (salt == nullptr) { - FatalError("node::PBKDF2()", "Out of Memory"); - } memcpy(salt, Buffer::Data(args[1]), saltlen); if (!args[2]->IsNumber()) { @@ -5602,8 +5589,6 @@ class RandomBytesRequest : public AsyncWrap { error_(0), size_(size), data_(node::Malloc(size)) { - if (data() == nullptr) - FatalError("node::RandomBytesRequest()", "Out of Memory"); Wrap(object, this); } @@ -5830,8 +5815,6 @@ void GetCurves(const FunctionCallbackInfo& args) { if (num_curves) { curves = node::Malloc(num_curves); - CHECK_NE(curves, nullptr); - if (EC_get_builtin_curves(curves, num_curves)) { for (size_t i = 0; i < num_curves; i++) { arr->Set(i, OneByteString(env->isolate(), OBJ_nid2sn(curves[i].nid))); diff --git a/src/node_internals.h b/src/node_internals.h index adcb7f835a3451..130af5d1c0f97f 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -199,7 +199,7 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator { virtual void* Allocate(size_t size); // Defined in src/node.cc virtual void* AllocateUninitialized(size_t size) - { return node::Malloc(size); } + { return node::UncheckedMalloc(size); } virtual void Free(void* data, size_t) { free(data); } private: diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc index f5bc4ad8c4eca3..ba03221696a539 100644 --- a/src/stream_wrap.cc +++ b/src/stream_wrap.cc @@ -150,12 +150,6 @@ void StreamWrap::OnAlloc(uv_handle_t* handle, void StreamWrap::OnAllocImpl(size_t size, uv_buf_t* buf, void* ctx) { buf->base = node::Malloc(size); buf->len = size; - - if (buf->base == nullptr && size > 0) { - FatalError( - "node::StreamWrap::DoAlloc(size_t, uv_buf_t*, void*)", - "Out Of Memory"); - } } @@ -204,8 +198,8 @@ void StreamWrap::OnReadImpl(ssize_t nread, return; } - char* base = node::Realloc(buf->base, nread); CHECK_LE(static_cast(nread), buf->len); + char* base = node::Realloc(buf->base, nread); if (pending == UV_TCP) { pending_obj = AcceptHandle(env, wrap); diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 065a8ece15a06c..882ca6e3e89bd3 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -53,7 +53,7 @@ class ExternString: public ResourceType { if (length == 0) return scope.Escape(String::Empty(isolate)); - TypeName* new_data = node::Malloc(length); + TypeName* new_data = node::UncheckedMalloc(length); if (new_data == nullptr) { return Local(); } @@ -609,7 +609,7 @@ Local StringBytes::Encode(Isolate* isolate, case ASCII: if (contains_non_ascii(buf, buflen)) { - char* out = node::Malloc(buflen); + char* out = node::UncheckedMalloc(buflen); if (out == nullptr) { return Local(); } @@ -644,7 +644,7 @@ Local StringBytes::Encode(Isolate* isolate, case BASE64: { size_t dlen = base64_encoded_size(buflen); - char* dst = node::Malloc(dlen); + char* dst = node::UncheckedMalloc(dlen); if (dst == nullptr) { return Local(); } @@ -663,7 +663,7 @@ Local StringBytes::Encode(Isolate* isolate, case HEX: { size_t dlen = buflen * 2; - char* dst = node::Malloc(dlen); + char* dst = node::UncheckedMalloc(dlen); if (dst == nullptr) { return Local(); } diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index 4b8a575d56f2d8..813f7ef869ecce 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -664,7 +664,6 @@ void TLSWrap::OnDestructImpl(void* ctx) { void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) { buf->base = node::Malloc(suggested_size); - CHECK_NE(buf->base, nullptr); buf->len = suggested_size; } diff --git a/src/udp_wrap.cc b/src/udp_wrap.cc index 43378199fe0188..d14eefd64d600a 100644 --- a/src/udp_wrap.cc +++ b/src/udp_wrap.cc @@ -375,11 +375,6 @@ void UDPWrap::OnAlloc(uv_handle_t* handle, uv_buf_t* buf) { buf->base = node::Malloc(suggested_size); buf->len = suggested_size; - - if (buf->base == nullptr && suggested_size > 0) { - FatalError("node::UDPWrap::OnAlloc(uv_handle_t*, size_t, uv_buf_t*)", - "Out Of Memory"); - } } @@ -415,7 +410,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle, return; } - char* base = node::Realloc(buf->base, nread); + char* base = node::UncheckedRealloc(buf->base, nread); argv[2] = Buffer::New(env, base, nread).ToLocalChecked(); argv[3] = AddressToJS(env, addr); wrap->MakeCallback(env->onmessage_string(), arraysize(argv), argv); diff --git a/src/util-inl.h b/src/util-inl.h index 7d4eda49152b16..886b8569d63d2b 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -336,7 +336,7 @@ inline size_t MultiplyWithOverflowCheck(size_t a, size_t b) { // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. template -T* Realloc(T* pointer, size_t n) { +T* UncheckedRealloc(T* pointer, size_t n) { size_t full_size = MultiplyWithOverflowCheck(sizeof(T), n); if (full_size == 0) { @@ -349,18 +349,39 @@ T* Realloc(T* pointer, size_t n) { // As per spec realloc behaves like malloc if passed nullptr. template -T* Malloc(size_t n) { +T* UncheckedMalloc(size_t n) { if (n == 0) n = 1; - return Realloc(nullptr, n); + return UncheckedRealloc(nullptr, n); } template -T* Calloc(size_t n) { +T* UncheckedCalloc(size_t n) { if (n == 0) n = 1; MultiplyWithOverflowCheck(sizeof(T), n); return static_cast(calloc(n, sizeof(T))); } +template +T* Realloc(T* pointer, size_t n) { + T* ret = UncheckedRealloc(pointer, n); + if (n > 0) CHECK_NE(ret, nullptr); + return ret; +} + +template +T* Malloc(size_t n) { + T* ret = UncheckedMalloc(n); + if (n > 0) CHECK_NE(ret, nullptr); + return ret; +} + +template +T* Calloc(size_t n) { + T* ret = UncheckedCalloc(n); + if (n > 0) CHECK_NE(ret, nullptr); + return ret; +} + } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/util.cc b/src/util.cc index 7ce99d5c76aa93..14aa68996f56cc 100644 --- a/src/util.cc +++ b/src/util.cc @@ -1,6 +1,7 @@ #include "util.h" #include "string_bytes.h" #include "node_buffer.h" +#include "node_internals.h" #include namespace node { diff --git a/src/util.h b/src/util.h index 59a26fb8527735..8b2db6f5c321e8 100644 --- a/src/util.h +++ b/src/util.h @@ -32,6 +32,15 @@ namespace node { // nullptr for zero-sized allocation requests. Normalize by always using // a nullptr. template +inline T* UncheckedRealloc(T* pointer, size_t n); +template +inline T* UncheckedMalloc(size_t n); +template +inline T* UncheckedCalloc(size_t n); + +// Same things, but aborts immediately instead of returning nullptr when +// no memory is available. +template inline T* Realloc(T* pointer, size_t n); template inline T* Malloc(size_t n); @@ -41,6 +50,8 @@ inline T* Calloc(size_t n); // Shortcuts for char*. inline char* Malloc(size_t n) { return Malloc(n); } inline char* Calloc(size_t n) { return Calloc(n); } +inline char* UncheckedMalloc(size_t n) { return UncheckedMalloc(n); } +inline char* UncheckedCalloc(size_t n) { return UncheckedCalloc(n); } #ifdef __GNUC__ #define NO_RETURN __attribute__((noreturn)) @@ -302,7 +313,6 @@ class MaybeStackBuffer { buf_ = buf_st_; } else { buf_ = Malloc(storage); - CHECK_NE(buf_, nullptr); } // Remember how much was allocated to check against that in SetLength(). diff --git a/test/cctest/test_util.cc b/test/cctest/test_util.cc index 7bbf53af13d3c4..f1446ae0345153 100644 --- a/test/cctest/test_util.cc +++ b/test/cctest/test_util.cc @@ -105,3 +105,19 @@ TEST(UtilTest, Calloc) { EXPECT_NE(nullptr, Calloc(0)); EXPECT_NE(nullptr, Calloc(1)); } + +TEST(UtilTest, UncheckedMalloc) { + using node::UncheckedMalloc; + EXPECT_NE(nullptr, UncheckedMalloc(0)); + EXPECT_NE(nullptr, UncheckedMalloc(1)); + EXPECT_NE(nullptr, UncheckedMalloc(0)); + EXPECT_NE(nullptr, UncheckedMalloc(1)); +} + +TEST(UtilTest, UncheckedCalloc) { + using node::UncheckedCalloc; + EXPECT_NE(nullptr, UncheckedCalloc(0)); + EXPECT_NE(nullptr, UncheckedCalloc(1)); + EXPECT_NE(nullptr, UncheckedCalloc(0)); + EXPECT_NE(nullptr, UncheckedCalloc(1)); +} From 618ca4276d344be2c6d1ebfe22df5fbe8c3c9fef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sat, 10 Sep 2016 18:21:20 +0200 Subject: [PATCH 4/4] src: notify V8 for low memory when alloc fails Call `v8::Isolate::GetCurrent()->LowMemoryNotification()` when an allocation fails to give V8 a chance to clean up and return memory before retrying (and possibly giving up). PR-URL: https://github.com/nodejs/node/pull/8482 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson Reviewed-By: Ilkka Myller --- src/node.cc | 3 +++ src/node_internals.h | 3 +++ src/util-inl.h | 10 +++++++++- src/util.cc | 9 +++++++++ src/util.h | 5 +++++ 5 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/node.cc b/src/node.cc index 6345abdb312dda..98fde0dbbe3f65 100644 --- a/src/node.cc +++ b/src/node.cc @@ -198,6 +198,7 @@ bool trace_warnings = false; // that is used by lib/module.js bool config_preserve_symlinks = false; +bool v8_initialized = false; // Set in node.cc by ParseArgs when --expose-internals or --expose_internals is // used. @@ -4895,6 +4896,7 @@ int Start(int argc, char** argv) { v8_platform.Initialize(v8_thread_pool_size); V8::Initialize(); + v8_initialized = true; int exit_code = 1; { @@ -4908,6 +4910,7 @@ int Start(int argc, char** argv) { StartNodeInstance(&instance_data); exit_code = instance_data.exit_code(); } + v8_initialized = false; V8::Dispose(); v8_platform.Dispose(); diff --git a/src/node_internals.h b/src/node_internals.h index 130af5d1c0f97f..9ead8b10377075 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -43,6 +43,9 @@ extern std::string openssl_config; // that is used by lib/module.js extern bool config_preserve_symlinks; +// Tells whether it is safe to call v8::Isolate::GetCurrent(). +extern bool v8_initialized; + // Set in node.cc by ParseArgs when --expose-internals or --expose_internals is // used. // Used in node_config.cc to set a constant on process.binding('config') diff --git a/src/util-inl.h b/src/util-inl.h index 886b8569d63d2b..5ffe5b857f5381 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -344,7 +344,15 @@ T* UncheckedRealloc(T* pointer, size_t n) { return nullptr; } - return static_cast(realloc(pointer, full_size)); + void* allocated = realloc(pointer, full_size); + + if (UNLIKELY(allocated == nullptr)) { + // Tell V8 that memory is low and retry. + LowMemoryNotification(); + allocated = realloc(pointer, full_size); + } + + return static_cast(allocated); } // As per spec realloc behaves like malloc if passed nullptr. diff --git a/src/util.cc b/src/util.cc index 14aa68996f56cc..9fb5c3fd2855d3 100644 --- a/src/util.cc +++ b/src/util.cc @@ -77,4 +77,13 @@ BufferValue::BufferValue(Isolate* isolate, Local value) { } } +void LowMemoryNotification() { + if (v8_initialized) { + auto isolate = v8::Isolate::GetCurrent(); + if (isolate != nullptr) { + isolate->LowMemoryNotification(); + } + } +} + } // namespace node diff --git a/src/util.h b/src/util.h index 8b2db6f5c321e8..4ce25e4622f4b2 100644 --- a/src/util.h +++ b/src/util.h @@ -53,6 +53,11 @@ inline char* Calloc(size_t n) { return Calloc(n); } inline char* UncheckedMalloc(size_t n) { return UncheckedMalloc(n); } inline char* UncheckedCalloc(size_t n) { return UncheckedCalloc(n); } +// Used by the allocation functions when allocation fails. +// Thin wrapper around v8::Isolate::LowMemoryNotification() that checks +// whether V8 is initialized. +void LowMemoryNotification(); + #ifdef __GNUC__ #define NO_RETURN __attribute__((noreturn)) #else