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`).

PR-URL: #8482
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
  • Loading branch information
addaleax authored and jasnell committed Sep 29, 2016
1 parent f77cdf8 commit de94601
Show file tree
Hide file tree
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
Expand Up @@ -5592,11 +5592,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);

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

Expand Down
11 changes: 4 additions & 7 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -298,10 +298,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);
}

Expand Down

0 comments on commit de94601

Please sign in to comment.