Skip to content

Commit

Permalink
src: remove UncheckedMalloc(0) workaround
Browse files Browse the repository at this point in the history
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: #8571
Refs: #8572
PR-URL: #44543
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
tniessen authored and danielleadams committed Oct 10, 2022
1 parent 0606f92 commit 3abb607
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 30 deletions.
14 changes: 8 additions & 6 deletions src/string_bytes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -704,20 +704,21 @@ MaybeLocal<Value> StringBytes::Encode(Isolate* isolate,
}

case UCS2: {
size_t str_len = buflen / 2;
if (IsBigEndian()) {
uint16_t* dst = node::UncheckedMalloc<uint16_t>(buflen / 2);
if (dst == nullptr) {
uint16_t* dst = node::UncheckedMalloc<uint16_t>(str_len);
if (str_len != 0 && dst == nullptr) {
*error = node::ERR_MEMORY_ALLOCATION_FAILED(isolate);
return MaybeLocal<Value>();
}
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<uint8_t>(buf[i + 1]);
const uint8_t lo = static_cast<uint8_t>(buf[i + 0]);
dst[k] = static_cast<uint16_t>(hi) << 8 | lo;
}
return ExternTwoByteString::New(isolate, dst, buflen / 2, error);
return ExternTwoByteString::New(isolate, dst, str_len, error);
}
if (reinterpret_cast<uintptr_t>(buf) % 2 != 0) {
// Unaligned data still means we can't directly pass it to V8.
Expand All @@ -728,10 +729,10 @@ MaybeLocal<Value> StringBytes::Encode(Isolate* isolate,
}
memcpy(dst, buf, buflen);
return ExternTwoByteString::New(
isolate, reinterpret_cast<uint16_t*>(dst), buflen / 2, error);
isolate, reinterpret_cast<uint16_t*>(dst), str_len, error);
}
return ExternTwoByteString::NewFromCopy(
isolate, reinterpret_cast<const uint16_t*>(buf), buflen / 2, error);
isolate, reinterpret_cast<const uint16_t*>(buf), str_len, error);
}

default:
Expand All @@ -747,6 +748,7 @@ MaybeLocal<Value> StringBytes::Encode(Isolate* isolate,
const uint16_t* buf,
size_t buflen,
Local<Value>* error) {
if (buflen == 0) return String::Empty(isolate);
CHECK_BUFLEN_IN_RANGE(buflen);

// Node's "ucs2" encoding expects LE character data inside a
Expand Down
4 changes: 1 addition & 3 deletions src/util-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -361,14 +361,12 @@ T* UncheckedRealloc(T* pointer, size_t n) {
// As per spec realloc behaves like malloc if passed nullptr.
template <typename T>
inline T* UncheckedMalloc(size_t n) {
if (n == 0) n = 1;
return UncheckedRealloc<T>(nullptr, n);
}

template <typename T>
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<T*>(calloc(n, sizeof(T)));
}

Expand Down
42 changes: 21 additions & 21 deletions test/cctest/test_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<char>(0));
TEST_AND_FREE(Malloc<char>(1));
TEST_AND_FREE(Malloc(0));
TEST_AND_FREE(Malloc(1));
TEST_AND_FREE(Malloc<char>, 0);
TEST_AND_FREE(Malloc<char>, 1);
TEST_AND_FREE(Malloc, 0);
TEST_AND_FREE(Malloc, 1);
}

TEST(UtilTest, Calloc) {
TEST_AND_FREE(Calloc<char>(0));
TEST_AND_FREE(Calloc<char>(1));
TEST_AND_FREE(Calloc(0));
TEST_AND_FREE(Calloc(1));
TEST_AND_FREE(Calloc<char>, 0);
TEST_AND_FREE(Calloc<char>, 1);
TEST_AND_FREE(Calloc, 0);
TEST_AND_FREE(Calloc, 1);
}

TEST(UtilTest, UncheckedMalloc) {
TEST_AND_FREE(UncheckedMalloc<char>(0));
TEST_AND_FREE(UncheckedMalloc<char>(1));
TEST_AND_FREE(UncheckedMalloc(0));
TEST_AND_FREE(UncheckedMalloc(1));
TEST_AND_FREE(UncheckedMalloc<char>, 0);
TEST_AND_FREE(UncheckedMalloc<char>, 1);
TEST_AND_FREE(UncheckedMalloc, 0);
TEST_AND_FREE(UncheckedMalloc, 1);
}

TEST(UtilTest, UncheckedCalloc) {
TEST_AND_FREE(UncheckedCalloc<char>(0));
TEST_AND_FREE(UncheckedCalloc<char>(1));
TEST_AND_FREE(UncheckedCalloc(0));
TEST_AND_FREE(UncheckedCalloc(1));
TEST_AND_FREE(UncheckedCalloc<char>, 0);
TEST_AND_FREE(UncheckedCalloc<char>, 1);
TEST_AND_FREE(UncheckedCalloc, 0);
TEST_AND_FREE(UncheckedCalloc, 1);
}

template <typename T>
Expand Down

0 comments on commit 3abb607

Please sign in to comment.