src: fix memory leak in ExternString

v8 will silently return an empty handle which doesn't delete our data if string length is above String::kMaxLength Fixes: #1374 PR-URL: #2402 Reviewed-By: trevnorris - Trevor Norris <[email protected]> Reviewed-By: indutny - Fedor Indutny <[email protected]> Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]> Amended by @rvagg to change author date from "1970-08-16 16:09:02 +0200" to "2015-08-16 16:09:02 +0200" as per discussion @ #2713
nodejs · Sep 6, 2015 · 5201cb0 · 5201cb0
1 parent 93ba585
commit 5201cb0
Show file tree

Hide file tree

Showing 4 changed files with 107 additions and 14 deletions.
diff --git a/lib/buffer.js b/lib/buffer.js
@@ -350,10 +350,14 @@ function slowToString(encoding, start, end) {
 
 
 Buffer.prototype.toString = function() {
-  const length = this.length | 0;
-  if (arguments.length === 0)
-    return this.utf8Slice(0, length);
-  return slowToString.apply(this, arguments);
+  if (arguments.length === 0) {
+    var result = this.utf8Slice(0, this.length);
+  } else {
+    var result = slowToString.apply(this, arguments);
+  }
+  if (result === undefined)
+    throw new Error('toString failed');
+  return result;
 };
 
 

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
@@ -1024,6 +1024,10 @@ void Initialize(Handle<Object> target,
   target->Set(env->context(),
               FIXED_ONE_BYTE_STRING(env->isolate(), "kMaxLength"),
               Integer::NewFromUnsigned(env->isolate(), kMaxLength)).FromJust();
+
+  target->Set(env->context(),
+              FIXED_ONE_BYTE_STRING(env->isolate(), "kStringMaxLength"),
+              Integer::New(env->isolate(), String::kMaxLength)).FromJust();
 }
 
 

diff --git a/src/string_bytes.cc b/src/string_bytes.cc
@@ -22,13 +22,14 @@ using v8::Local;
 using v8::Object;
 using v8::String;
 using v8::Value;
+using v8::MaybeLocal;
 
 
 template <typename ResourceType, typename TypeName>
 class ExternString: public ResourceType {
   public:
     ~ExternString() override {
-      delete[] data_;
+      free(const_cast<TypeName*>(data_));
       isolate()->AdjustAmountOfExternalAllocatedMemory(-byte_length());
     }
 
@@ -52,7 +53,11 @@ class ExternString: public ResourceType {
       if (length == 0)
         return scope.Escape(String::Empty(isolate));
 
-      TypeName* new_data = new TypeName[length];
+      TypeName* new_data =
+          static_cast<TypeName*>(malloc(length * sizeof(*new_data)));
+      if (new_data == nullptr) {
+        return Local<String>();
+      }
       memcpy(new_data, data, length * sizeof(*new_data));
 
       return scope.Escape(ExternString<ResourceType, TypeName>::New(isolate,
@@ -72,10 +77,15 @@ class ExternString: public ResourceType {
       ExternString* h_str = new ExternString<ResourceType, TypeName>(isolate,
                                                                      data,
                                                                      length);
-      Local<String> str = String::NewExternal(isolate, h_str);
+      MaybeLocal<String> str = String::NewExternal(isolate, h_str);
       isolate->AdjustAmountOfExternalAllocatedMemory(h_str->byte_length());
 
-      return scope.Escape(str);
+      if (str.IsEmpty()) {
+        delete h_str;
+        return Local<String>();
+      }
+
+      return scope.Escape(str.ToLocalChecked());
     }
 
     inline Isolate* isolate() const { return isolate_; }
@@ -765,11 +775,14 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
 
     case ASCII:
       if (contains_non_ascii(buf, buflen)) {
-        char* out = new char[buflen];
+        char* out = static_cast<char*>(malloc(buflen));
+        if (out == nullptr) {
+          return Local<String>();
+        }
         force_ascii(buf, out, buflen);
         if (buflen < EXTERN_APEX) {
           val = OneByteString(isolate, out, buflen);
-          delete[] out;
+          free(out);
         } else {
           val = ExternOneByteString::New(isolate, out, buflen);
         }
@@ -797,14 +810,17 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
 
     case BASE64: {
       size_t dlen = base64_encoded_size(buflen);
-      char* dst = new char[dlen];
+      char* dst = static_cast<char*>(malloc(dlen));
+      if (dst == nullptr) {
+        return Local<String>();
+      }
 
       size_t written = base64_encode(buf, buflen, dst, dlen);
       CHECK_EQ(written, dlen);
 
       if (dlen < EXTERN_APEX) {
         val = OneByteString(isolate, dst, dlen);
-        delete[] dst;
+        free(dst);
       } else {
         val = ExternOneByteString::New(isolate, dst, dlen);
       }
@@ -813,13 +829,16 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
 
     case HEX: {
       size_t dlen = buflen * 2;
-      char* dst = new char[dlen];
+      char* dst = static_cast<char*>(malloc(dlen));
+      if (dst == nullptr) {
+        return Local<String>();
+      }
       size_t written = hex_encode(buf, buflen, dst, dlen);
       CHECK_EQ(written, dlen);
 
       if (dlen < EXTERN_APEX) {
         val = OneByteString(isolate, dst, dlen);
-        delete[] dst;
+        free(dst);
       } else {
         val = ExternOneByteString::New(isolate, dst, dlen);
       }

diff --git a/test/parallel/test-stringbytes-external.js b/test/parallel/test-stringbytes-external.js
@@ -107,3 +107,69 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS;
   assert.equal(a, b);
   assert.equal(b, c);
 })();
+
+// v8 fails silently if string length > v8::String::kMaxLength
+(function() {
+  // v8::String::kMaxLength defined in v8.h
+  const kStringMaxLength = process.binding('buffer').kStringMaxLength;
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString();
+  }, /toString failed|Buffer allocation failed/);
+
+  try {
+    new Buffer(kStringMaxLength * 4);
+  } catch(e) {
+    assert.equal(e.message, 'Buffer allocation failed - process out of memory');
+    console.log(
+        '1..0 # Skipped: intensive toString tests due to memory confinements');
+    return;
+  }
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString('ascii');
+  }, /toString failed/);
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString('utf8');
+  }, /toString failed/);
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength * 2 + 2).toString('utf16le');
+  }, /toString failed/);
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString('binary');
+  }, /toString failed/);
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString('base64');
+  }, /toString failed/);
+
+  assert.throws(function() {
+    new Buffer(kStringMaxLength + 1).toString('hex');
+  }, /toString failed/);
+
+  var maxString = new Buffer(kStringMaxLength).toString();
+  assert.equal(maxString.length, kStringMaxLength);
+  // Free the memory early instead of at the end of the next assignment
+  maxString = undefined;
+
+  maxString = new Buffer(kStringMaxLength).toString('binary');
+  assert.equal(maxString.length, kStringMaxLength);
+  maxString = undefined;
+
+  maxString =
+      new Buffer(kStringMaxLength + 1).toString('binary', 1);
+  assert.equal(maxString.length, kStringMaxLength);
+  maxString = undefined;
+
+  maxString =
+      new Buffer(kStringMaxLength + 1).toString('binary', 0, kStringMaxLength);
+  assert.equal(maxString.length, kStringMaxLength);
+  maxString = undefined;
+
+  maxString = new Buffer(kStringMaxLength + 2).toString('utf16le');
+  assert.equal(maxString.length, (kStringMaxLength + 2) / 2);
+  maxString = undefined;
+})();