src: add Malloc() size param + overflow detection

addaleax · jasnell · commit de946013c25f · 2016-09-29T11:15:08.000-07:00
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>
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -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);
 
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
@@ -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>();
     }
diff --git a/src/util-inl.h b/src/util-inl.h
@@ -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);
 }
 
diff --git a/src/util.h b/src/util.h
@@ -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))
@@ -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);
     }
 

Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,7 @@ class ExternString: public ResourceType {`
`54`	`54`	`return scope.Escape(String::Empty(isolate));`
`55`	`55`
`56`	`56`	`TypeName* new_data =`
`57`		`- static_cast<TypeName>(node::Malloc(length sizeof(*new_data)));`
	`57`	`+ static_cast<TypeName>(node::Malloc(length, sizeof(new_data)));`
`58`	`58`	`if (new_data == nullptr) {`
`59`	`59`	`return Local<String>();`
`60`	`60`	`}`