Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix memory leak in ExternString #2402

Closed
wants to merge 1 commit into from
Closed

src: fix memory leak in ExternString #2402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-declaration of result in different branches.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's legal JS and we do it in other places as well.

} else {
var result = slowToString.apply(this, arguments);
}
if (result === undefined)
throw new Error('toString failed');
return result;
};


Expand Down
4 changes: 4 additions & 0 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}


Expand Down
39 changes: 29 additions & 10 deletions src/string_bytes.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand All @@ -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,
Expand All @@ -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>();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't NewFromCopy() have the same check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewFromCopy uses New

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it leaks new_data on error now, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? new_data is used as data in new ExternString

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's the copy of the data? Maybe I'm misunderstanding the question.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using new TypeName[length] is a bug. If the machine is OOM then you're screwed. Here's an example script:

var a = [];
var f = false;

while (!f) {
  try {
    a.push(new Buffer(1024 * 1024 * 1024).fill('a'));
  } catch (e) {
    f = true;
  }
}

console.log(a[a.length - 1].length);      // 1073741824
console.log(a[a.length - 1].toString());  // undefined

So NewFromCopy() should use malloc(), check if ptr == nullptr and return Local<Object>() if that's the case. This will account for whether kStringMaxLength was exceeded and if ran out of memory while creating the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your example code does not use NewFromCopy because it uses the fast lane via NewFromUtf8 and then returns undefined because v8 returns an empty handle if length > kStringMaxLength.

Example code with NewFromCopy:

var a = [];
var buffer = new Buffer(200 * 1024 * 1024);
while (true) {
  a.push(buffer.toString('binary'));
}

If new TypeName[length] fails:

terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
[1]    2013 abort (core dumped)  ./iojs test

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skomski Thanks for the test fix. That's actually what I expected, but I ignored whether the value was intercepted in transit. So my comment about handling this case still stands. Don't want to see aborts on devices that have little memory (e.g. raspberry pi).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Complaint withdrawn, I misread the code.

Aside, it's kind of weird that both New() and NewFromCopy() call AdjustAmountOfExternalAllocatedMemory() but depend on the destructor to lower it again. I'd expect the constructor to take care of that, now the logic is duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only New calls AdjustAmountOfExternalAllocatedMemory and I think it's not called in the constructor of ExternString because it makes only sense after String::NewExternal.

It would make sense to only call AdjustAmountOfExternalAllocatedMemory after the returned Handle is not empty but since it is not the norm I guess it doesn't matter.


return scope.Escape(str.ToLocalChecked());
}

inline Isolate* isolate() const { return isolate_; }
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
Expand Down
66 changes: 66 additions & 0 deletions test/parallel/test-stringbytes-external.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
})();