Skip to content

Commit

Permalink
src: add Malloc() size param + overflow detection
Browse files Browse the repository at this point in the history
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`).

Backport-PR-URL: #16587
PR-URL: #8482
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ilkka Myller <[email protected]>
addaleax authored and MylesBorins committed Nov 14, 2017

Verified

This commit was signed with the committer’s verified signature.
ararslan Alex Arslan
1 parent 92b13e4 commit 19f3ac9
Showing 4 changed files with 25 additions and 17 deletions.
5 changes: 2 additions & 3 deletions src/node_crypto.cc
Original file line number Diff line number Diff line change
@@ -5826,11 +5826,10 @@ void GetCurves(const FunctionCallbackInfo<Value>& args) {
const size_t num_curves = EC_get_builtin_curves(nullptr, 0);
Local<Array> 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<EC_builtin_curve*>(node::Malloc(alloc_size));
curves = static_cast<EC_builtin_curve*>(node::Malloc(sizeof(*curves),
num_curves));

CHECK_NE(curves, nullptr);

2 changes: 1 addition & 1 deletion src/string_bytes.cc
Original file line number Diff line number Diff line change
@@ -54,7 +54,7 @@ class ExternString: public ResourceType {
return scope.Escape(String::Empty(isolate));

TypeName* new_data =
static_cast<TypeName*>(node::Malloc(length * sizeof(*new_data)));
static_cast<TypeName*>(node::Malloc(length, sizeof(*new_data)));
if (new_data == nullptr) {
return Local<String>();
}
24 changes: 18 additions & 6 deletions src/util-inl.h
Original file line number Diff line number Diff line change
@@ -320,31 +320,43 @@ 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.
// 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) {
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);
}

11 changes: 4 additions & 7 deletions src/util.h
Original file line number Diff line number Diff line change
@@ -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<T*>(Malloc(sizeof(T) * storage));
buf_ = static_cast<T*>(Malloc(sizeof(T), storage));
CHECK_NE(buf_, nullptr);
}

0 comments on commit 19f3ac9

Please sign in to comment.