Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: normalize malloc, realloc #7564

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/cares_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,8 @@ 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_ares_task*>(malloc(sizeof(*task)));
node_ares_task* task =
static_cast<node_ares_task*>(node::Malloc(sizeof(*task)));

if (task == nullptr) {
/* Out of memory. */
Expand Down
4 changes: 2 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -977,9 +977,9 @@ Local<Value> WinapiErrnoException(Isolate* isolate,

void* ArrayBufferAllocator::Allocate(size_t size) {
if (zero_fill_field_ || zero_fill_all_buffers)
return calloc(size, 1);
return node::Calloc(size, 1);
else
return malloc(size);
return node::Malloc(size);
}

static bool DomainHasErrorHandler(const Environment* env,
Expand Down
12 changes: 4 additions & 8 deletions src/node_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
size_t length = end - start;

#define BUFFER_MALLOC(length) \
zero_fill_all_buffers ? calloc(length, 1) : malloc(length)
zero_fill_all_buffers ? node::Calloc(length, 1) : node::Malloc(length)

#if defined(__GNUC__) || defined(__clang__)
#define BSWAP_INTRINSIC_2(x) __builtin_bswap16(x)
Expand Down Expand Up @@ -265,10 +265,6 @@ MaybeLocal<Object> New(Isolate* isolate,
size_t actual = 0;
char* data = nullptr;

// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
// 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.
if (length > 0) {
data = static_cast<char*>(BUFFER_MALLOC(length));

Expand All @@ -282,7 +278,7 @@ MaybeLocal<Object> New(Isolate* isolate,
free(data);
data = nullptr;
} else if (actual < length) {
data = static_cast<char*>(realloc(data, actual));
data = static_cast<char*>(node::Realloc(data, actual));
CHECK_NE(data, nullptr);
}
}
Expand Down Expand Up @@ -361,7 +357,7 @@ MaybeLocal<Object> Copy(Environment* env, const char* data, size_t length) {
void* new_data;
if (length > 0) {
CHECK_NE(data, nullptr);
new_data = malloc(length);
new_data = node::Malloc(length);
if (new_data == nullptr)
return Local<Object>();
memcpy(new_data, data, length);
Expand Down Expand Up @@ -1083,7 +1079,7 @@ void IndexOfString(const FunctionCallbackInfo<Value>& args) {
offset,
is_forward);
} else if (enc == LATIN1) {
uint8_t* needle_data = static_cast<uint8_t*>(malloc(needle_length));
uint8_t* needle_data = static_cast<uint8_t*>(node::Malloc(needle_length));
if (needle_data == nullptr) {
return args.GetReturnValue().Set(-1);
}
Expand Down
22 changes: 11 additions & 11 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2279,7 +2279,7 @@ int SSLWrap<Base>::TLSExtStatusCallback(SSL* s, void* arg) {
size_t len = Buffer::Length(obj);

// OpenSSL takes control of the pointer after accepting it
char* data = reinterpret_cast<char*>(malloc(len));
char* data = reinterpret_cast<char*>(node::Malloc(len));
CHECK_NE(data, nullptr);
memcpy(data, resp, len);

Expand Down Expand Up @@ -3330,7 +3330,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<char*>(malloc(auth_tag_len_));
*out = static_cast<char*>(node::Malloc(auth_tag_len_));
CHECK_NE(*out, nullptr);
memcpy(*out, auth_tag_, auth_tag_len_);
return true;
Expand Down Expand Up @@ -4906,7 +4906,7 @@ void ECDH::ComputeSecret(const FunctionCallbackInfo<Value>& 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<char*>(malloc(out_len));
char* out = static_cast<char*>(node::Malloc(out_len));
CHECK_NE(out, nullptr);

int r = ECDH_compute_key(out, out_len, pub, ecdh->key_, nullptr);
Expand Down Expand Up @@ -4942,7 +4942,7 @@ void ECDH::GetPublicKey(const FunctionCallbackInfo<Value>& args) {
if (size == 0)
return env->ThrowError("Failed to get public key length");

unsigned char* out = static_cast<unsigned char*>(malloc(size));
unsigned char* out = static_cast<unsigned char*>(node::Malloc(size));
CHECK_NE(out, nullptr);

int r = EC_POINT_point2oct(ecdh->group_, pub, form, out, size, nullptr);
Expand All @@ -4968,7 +4968,7 @@ void ECDH::GetPrivateKey(const FunctionCallbackInfo<Value>& args) {
return env->ThrowError("Failed to get ECDH private key");

int size = BN_num_bytes(b);
unsigned char* out = static_cast<unsigned char*>(malloc(size));
unsigned char* out = static_cast<unsigned char*>(node::Malloc(size));
CHECK_NE(out, nullptr);

if (size != BN_bn2bin(b, out)) {
Expand Down Expand Up @@ -5099,7 +5099,7 @@ class PBKDF2Request : public AsyncWrap {
saltlen_(saltlen),
salt_(salt),
keylen_(keylen),
key_(static_cast<char*>(malloc(keylen))),
key_(static_cast<char*>(node::Malloc(keylen))),
iter_(iter) {
if (key() == nullptr)
FatalError("node::PBKDF2Request()", "Out of Memory");
Expand Down Expand Up @@ -5262,7 +5262,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {

THROW_AND_RETURN_IF_NOT_BUFFER(args[1], "Salt");

pass = static_cast<char*>(malloc(passlen));
pass = static_cast<char*>(node::Malloc(passlen));
if (pass == nullptr) {
FatalError("node::PBKDF2()", "Out of Memory");
}
Expand All @@ -5274,7 +5274,7 @@ void PBKDF2(const FunctionCallbackInfo<Value>& args) {
goto err;
}

salt = static_cast<char*>(malloc(saltlen));
salt = static_cast<char*>(node::Malloc(saltlen));
if (salt == nullptr) {
FatalError("node::PBKDF2()", "Out of Memory");
}
Expand Down Expand Up @@ -5367,8 +5367,8 @@ class RandomBytesRequest : public AsyncWrap {
: AsyncWrap(env, object, AsyncWrap::PROVIDER_CRYPTO),
error_(0),
size_(size),
data_(static_cast<char*>(malloc(size))) {
if (data() == nullptr)
data_(static_cast<char*>(node::Malloc(size))) {
if (data() == nullptr && size > 0)
FatalError("node::RandomBytesRequest()", "Out of Memory");
Wrap(object, this);
}
Expand Down Expand Up @@ -5596,7 +5596,7 @@ void GetCurves(const FunctionCallbackInfo<Value>& args) {

if (num_curves) {
alloc_size = sizeof(*curves) * num_curves;
curves = static_cast<EC_builtin_curve*>(malloc(alloc_size));
curves = static_cast<EC_builtin_curve*>(node::Malloc(alloc_size));

CHECK_NE(curves, nullptr);

Expand Down
3 changes: 2 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ class ArrayBufferAllocator : public v8::ArrayBuffer::Allocator {
inline uint32_t* zero_fill_field() { return &zero_fill_field_; }

virtual void* Allocate(size_t size); // Defined in src/node.cc
virtual void* AllocateUninitialized(size_t size) { return malloc(size); }
virtual void* AllocateUninitialized(size_t size)
{ return node::Malloc(size); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter accepts this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It complained when it was on one line and passed with it this way !

virtual void Free(void* data, size_t) { free(data); }

private:
Expand Down
4 changes: 2 additions & 2 deletions src/stream_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<char*>(malloc(size));
buf->base = static_cast<char*>(node::Malloc(size));
buf->len = size;

if (buf->base == nullptr && size > 0) {
Expand Down Expand Up @@ -204,7 +204,7 @@ void StreamWrap::OnReadImpl(ssize_t nread,
return;
}

char* base = static_cast<char*>(realloc(buf->base, nread));
char* base = static_cast<char*>(node::Realloc(buf->base, nread));
CHECK_LE(static_cast<size_t>(nread), buf->len);

if (pending == UV_TCP) {
Expand Down
8 changes: 4 additions & 4 deletions src/string_bytes.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class ExternString: public ResourceType {
return scope.Escape(String::Empty(isolate));

TypeName* new_data =
static_cast<TypeName*>(malloc(length * sizeof(*new_data)));
static_cast<TypeName*>(node::Malloc(length * sizeof(*new_data)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A future improvement is to replace Malloc calls like this one with a Calloc-like version that does overflow checks (but doesn't zero the memory.)

if (new_data == nullptr) {
return Local<String>();
}
Expand Down Expand Up @@ -624,7 +624,7 @@ Local<Value> StringBytes::Encode(Isolate* isolate,

case ASCII:
if (contains_non_ascii(buf, buflen)) {
char* out = static_cast<char*>(malloc(buflen));
char* out = static_cast<char*>(node::Malloc(buflen));
if (out == nullptr) {
return Local<String>();
}
Expand Down Expand Up @@ -659,7 +659,7 @@ Local<Value> StringBytes::Encode(Isolate* isolate,

case BASE64: {
size_t dlen = base64_encoded_size(buflen);
char* dst = static_cast<char*>(malloc(dlen));
char* dst = static_cast<char*>(node::Malloc(dlen));
if (dst == nullptr) {
return Local<String>();
}
Expand All @@ -678,7 +678,7 @@ Local<Value> StringBytes::Encode(Isolate* isolate,

case HEX: {
size_t dlen = buflen * 2;
char* dst = static_cast<char*>(malloc(dlen));
char* dst = static_cast<char*>(node::Malloc(dlen));
if (dst == nullptr) {
return Local<String>();
}
Expand Down
2 changes: 1 addition & 1 deletion src/tls_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -661,7 +661,7 @@ void TLSWrap::OnReadImpl(ssize_t nread,


void TLSWrap::OnAllocSelf(size_t suggested_size, uv_buf_t* buf, void* ctx) {
buf->base = static_cast<char*>(malloc(suggested_size));
buf->base = static_cast<char*>(node::Malloc(suggested_size));
CHECK_NE(buf->base, nullptr);
buf->len = suggested_size;
}
Expand Down
4 changes: 2 additions & 2 deletions src/udp_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -384,7 +384,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<char*>(malloc(suggested_size));
buf->base = static_cast<char*>(node::Malloc(suggested_size));
buf->len = suggested_size;

if (buf->base == nullptr && suggested_size > 0) {
Expand Down Expand Up @@ -426,7 +426,7 @@ void UDPWrap::OnRecv(uv_udp_t* handle,
return;
}

char* base = static_cast<char*>(realloc(buf->base, nread));
char* base = static_cast<char*>(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);
Expand Down
26 changes: 26 additions & 0 deletions src/util-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,32 @@ bool StringEqualNoCaseN(const char* a, const char* b, size_t length) {
return true;
}

// 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
// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
// 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) {
free(pointer);
return nullptr;
}
return realloc(pointer, size);
}

// as per spec realloc behaves like malloc if passed nullptr
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you capitalize and punctuate the comment?

void* Malloc(size_t size) {
return Realloc(nullptr, size);
}

void* Calloc(size_t n, size_t size) {
if ((n == 0) || (size == 0)) return nullptr;
CHECK_GE(n * size, n); // Overflow guard.
return calloc(n, size);
}

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
Expand Down
13 changes: 12 additions & 1 deletion src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@

namespace node {

// 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
// malloc(0) and realloc(ptr, 0) have implementation-defined behavior in
// 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);

#ifdef __GNUC__
#define NO_RETURN __attribute__((noreturn))
#else
Expand Down Expand Up @@ -300,7 +311,7 @@ class MaybeStackBuffer {
// Guard against overflow.
CHECK_LE(storage, sizeof(T) * storage);

buf_ = static_cast<T*>(malloc(sizeof(T) * storage));
buf_ = static_cast<T*>(Malloc(sizeof(T) * storage));
CHECK_NE(buf_, nullptr);
}

Expand Down
5 changes: 0 additions & 5 deletions test/parallel/parallel.status
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ test-debug-signal-cluster : PASS,FLAKY
test-fs-watch-enoent : FAIL, PASS
test-fs-watch-encoding : FAIL, PASS

# being worked under https://github.com/nodejs/node/pull/7564
test-async-wrap-post-did-throw : PASS, FLAKY
test-async-wrap-throw-from-callback : PASS, FLAKY
test-crypto-random : PASS, FLAKY

#being worked under https://github.com/nodejs/node/issues/7973
test-stdio-closed : PASS, FLAKY

Expand Down