From 3abb607f3a706507a95671beb26c5bfe2f74b258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Wed, 5 Oct 2022 17:39:27 +0200 Subject: [PATCH] src: remove UncheckedMalloc(0) workaround Assuming that UncheckedMalloc(0) returns a non-nullptr is non-standard and we use other allocators as well (e.g., OPENSSL_malloc) that do not guarantee this behavior. It is the caller's responsibility to check that size != 0 implies UncheckedMalloc(size) != nullptr, and that's exactly what the checked variants (Malloc etc.) already do. The current behavior is also inconsistent with UncheckedRealloc(), which always returns a nullptr when the size is 0, and with the documentation in src/README.md as well as with multiple comments in the source code. This changes UncheckedMalloc(), UncheckedCalloc(), and UncheckedRealloc() to always return a nullptr when the size is 0 instead of doing fake allocations in UncheckedMalloc() and UncheckedCalloc() while returning a nullptr from UncheckedRealloc(). This is consistent with existing documentation and comments. Refs: https://github.com/nodejs/node/issues/8571 Refs: https://github.com/nodejs/node/pull/8572 PR-URL: https://github.com/nodejs/node/pull/44543 Reviewed-By: Rafael Gonzaga Reviewed-By: James M Snell --- src/string_bytes.cc | 14 ++++++++------ src/util-inl.h | 4 +--- test/cctest/test_util.cc | 42 ++++++++++++++++++++-------------------- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/string_bytes.cc b/src/string_bytes.cc index 2f512a844f193d..6dcc32f36e01bd 100644 --- a/src/string_bytes.cc +++ b/src/string_bytes.cc @@ -704,20 +704,21 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, } case UCS2: { + size_t str_len = buflen / 2; if (IsBigEndian()) { - uint16_t* dst = node::UncheckedMalloc(buflen / 2); - if (dst == nullptr) { + uint16_t* dst = node::UncheckedMalloc(str_len); + if (str_len != 0 && dst == nullptr) { *error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate); return MaybeLocal(); } - for (size_t i = 0, k = 0; k < buflen / 2; i += 2, k += 1) { + for (size_t i = 0, k = 0; k < str_len; i += 2, k += 1) { // The input is in *little endian*, because that's what Node.js // expects, so the high byte comes after the low byte. const uint8_t hi = static_cast(buf[i + 1]); const uint8_t lo = static_cast(buf[i + 0]); dst[k] = static_cast(hi) << 8 | lo; } - return ExternTwoByteString::New(isolate, dst, buflen / 2, error); + return ExternTwoByteString::New(isolate, dst, str_len, error); } if (reinterpret_cast(buf) % 2 != 0) { // Unaligned data still means we can't directly pass it to V8. @@ -728,10 +729,10 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, } memcpy(dst, buf, buflen); return ExternTwoByteString::New( - isolate, reinterpret_cast(dst), buflen / 2, error); + isolate, reinterpret_cast(dst), str_len, error); } return ExternTwoByteString::NewFromCopy( - isolate, reinterpret_cast(buf), buflen / 2, error); + isolate, reinterpret_cast(buf), str_len, error); } default: @@ -747,6 +748,7 @@ MaybeLocal StringBytes::Encode(Isolate* isolate, const uint16_t* buf, size_t buflen, Local* error) { + if (buflen == 0) return String::Empty(isolate); CHECK_BUFLEN_IN_RANGE(buflen); // Node's "ucs2" encoding expects LE character data inside a diff --git a/src/util-inl.h b/src/util-inl.h index caadba9dae2caa..bdf9732e853485 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -361,14 +361,12 @@ T* UncheckedRealloc(T* pointer, size_t n) { // As per spec realloc behaves like malloc if passed nullptr. template inline T* UncheckedMalloc(size_t n) { - if (n == 0) n = 1; return UncheckedRealloc(nullptr, n); } template inline T* UncheckedCalloc(size_t n) { - if (n == 0) n = 1; - MultiplyWithOverflowCheck(sizeof(T), n); + if (MultiplyWithOverflowCheck(sizeof(T), n) == 0) return nullptr; return static_cast(calloc(n, sizeof(T))); } diff --git a/test/cctest/test_util.cc b/test/cctest/test_util.cc index a64942a8a35009..75861ba00d52ba 100644 --- a/test/cctest/test_util.cc +++ b/test/cctest/test_util.cc @@ -97,39 +97,39 @@ TEST(UtilTest, ToLower) { EXPECT_EQ('a', ToLower('A')); } -#define TEST_AND_FREE(expression) \ - do { \ - auto pointer = expression; \ - EXPECT_NE(nullptr, pointer); \ - free(pointer); \ +#define TEST_AND_FREE(expression, size) \ + do { \ + auto pointer = expression(size); \ + EXPECT_EQ(pointer == nullptr, size == 0); \ + free(pointer); \ } while (0) TEST(UtilTest, Malloc) { - TEST_AND_FREE(Malloc(0)); - TEST_AND_FREE(Malloc(1)); - TEST_AND_FREE(Malloc(0)); - TEST_AND_FREE(Malloc(1)); + TEST_AND_FREE(Malloc, 0); + TEST_AND_FREE(Malloc, 1); + TEST_AND_FREE(Malloc, 0); + TEST_AND_FREE(Malloc, 1); } TEST(UtilTest, Calloc) { - TEST_AND_FREE(Calloc(0)); - TEST_AND_FREE(Calloc(1)); - TEST_AND_FREE(Calloc(0)); - TEST_AND_FREE(Calloc(1)); + TEST_AND_FREE(Calloc, 0); + TEST_AND_FREE(Calloc, 1); + TEST_AND_FREE(Calloc, 0); + TEST_AND_FREE(Calloc, 1); } TEST(UtilTest, UncheckedMalloc) { - TEST_AND_FREE(UncheckedMalloc(0)); - TEST_AND_FREE(UncheckedMalloc(1)); - TEST_AND_FREE(UncheckedMalloc(0)); - TEST_AND_FREE(UncheckedMalloc(1)); + TEST_AND_FREE(UncheckedMalloc, 0); + TEST_AND_FREE(UncheckedMalloc, 1); + TEST_AND_FREE(UncheckedMalloc, 0); + TEST_AND_FREE(UncheckedMalloc, 1); } TEST(UtilTest, UncheckedCalloc) { - TEST_AND_FREE(UncheckedCalloc(0)); - TEST_AND_FREE(UncheckedCalloc(1)); - TEST_AND_FREE(UncheckedCalloc(0)); - TEST_AND_FREE(UncheckedCalloc(1)); + TEST_AND_FREE(UncheckedCalloc, 0); + TEST_AND_FREE(UncheckedCalloc, 1); + TEST_AND_FREE(UncheckedCalloc, 0); + TEST_AND_FREE(UncheckedCalloc, 1); } template