Permalink
Browse files

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 <trev.norris@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

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
  • Loading branch information...
skomski authored and rvagg committed Aug 16, 2015
1 parent 93ba585 commit 5201cb0ff188fd68ba66fcc75489379c8dd1d274
Showing with 107 additions and 14 deletions.
  1. +8 −4 lib/buffer.js
  2. +4 −0 src/node_buffer.cc
  3. +29 −10 src/string_bytes.cc
  4. +66 −0 test/parallel/test-stringbytes-external.js
View
@@ -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;
};
View
@@ -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();
}
View
@@ -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);
}
@@ -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;
+})();

0 comments on commit 5201cb0

Please sign in to comment.