Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e73eafd

Browse filesBrowse files
skomskirvagg
skomski
authored and
rvagg
committed
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
1 parent 1865cad commit e73eafd
Copy full SHA for e73eafd

File tree

Expand file treeCollapse file tree

4 files changed

+107
-14
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+107
-14
lines changed
Open diff view settings
Collapse file

‎lib/buffer.js‎

Copy file name to clipboardExpand all lines: lib/buffer.js
+8-4Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -350,10 +350,14 @@ function slowToString(encoding, start, end) {
350350

351351

352352
Buffer.prototype.toString = function() {
353-
const length = this.length | 0;
354-
if (arguments.length === 0)
355-
return this.utf8Slice(0, length);
356-
return slowToString.apply(this, arguments);
353+
if (arguments.length === 0) {
354+
var result = this.utf8Slice(0, this.length);
355+
} else {
356+
var result = slowToString.apply(this, arguments);
357+
}
358+
if (result === undefined)
359+
throw new Error('toString failed');
360+
return result;
357361
};
358362

359363

Collapse file

‎src/node_buffer.cc‎

Copy file name to clipboardExpand all lines: src/node_buffer.cc
+4Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1024,6 +1024,10 @@ void Initialize(Handle<Object> target,
10241024
target->Set(env->context(),
10251025
FIXED_ONE_BYTE_STRING(env->isolate(), "kMaxLength"),
10261026
Integer::NewFromUnsigned(env->isolate(), kMaxLength)).FromJust();
1027+
1028+
target->Set(env->context(),
1029+
FIXED_ONE_BYTE_STRING(env->isolate(), "kStringMaxLength"),
1030+
Integer::New(env->isolate(), String::kMaxLength)).FromJust();
10271031
}
10281032

10291033

Collapse file

‎src/string_bytes.cc‎

Copy file name to clipboardExpand all lines: src/string_bytes.cc
+29-10Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,14 @@ using v8::Local;
2222
using v8::Object;
2323
using v8::String;
2424
using v8::Value;
25+
using v8::MaybeLocal;
2526

2627

2728
template <typename ResourceType, typename TypeName>
2829
class ExternString: public ResourceType {
2930
public:
3031
~ExternString() override {
31-
delete[] data_;
32+
free(const_cast<TypeName*>(data_));
3233
isolate()->AdjustAmountOfExternalAllocatedMemory(-byte_length());
3334
}
3435

@@ -52,7 +53,11 @@ class ExternString: public ResourceType {
5253
if (length == 0)
5354
return scope.Escape(String::Empty(isolate));
5455

55-
TypeName* new_data = new TypeName[length];
56+
TypeName* new_data =
57+
static_cast<TypeName*>(malloc(length * sizeof(*new_data)));
58+
if (new_data == nullptr) {
59+
return Local<String>();
60+
}
5661
memcpy(new_data, data, length * sizeof(*new_data));
5762

5863
return scope.Escape(ExternString<ResourceType, TypeName>::New(isolate,
@@ -72,10 +77,15 @@ class ExternString: public ResourceType {
7277
ExternString* h_str = new ExternString<ResourceType, TypeName>(isolate,
7378
data,
7479
length);
75-
Local<String> str = String::NewExternal(isolate, h_str);
80+
MaybeLocal<String> str = String::NewExternal(isolate, h_str);
7681
isolate->AdjustAmountOfExternalAllocatedMemory(h_str->byte_length());
7782

78-
return scope.Escape(str);
83+
if (str.IsEmpty()) {
84+
delete h_str;
85+
return Local<String>();
86+
}
87+
88+
return scope.Escape(str.ToLocalChecked());
7989
}
8090

8191
inline Isolate* isolate() const { return isolate_; }
@@ -765,11 +775,14 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
765775

766776
case ASCII:
767777
if (contains_non_ascii(buf, buflen)) {
768-
char* out = new char[buflen];
778+
char* out = static_cast<char*>(malloc(buflen));
779+
if (out == nullptr) {
780+
return Local<String>();
781+
}
769782
force_ascii(buf, out, buflen);
770783
if (buflen < EXTERN_APEX) {
771784
val = OneByteString(isolate, out, buflen);
772-
delete[] out;
785+
free(out);
773786
} else {
774787
val = ExternOneByteString::New(isolate, out, buflen);
775788
}
@@ -797,14 +810,17 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
797810

798811
case BASE64: {
799812
size_t dlen = base64_encoded_size(buflen);
800-
char* dst = new char[dlen];
813+
char* dst = static_cast<char*>(malloc(dlen));
814+
if (dst == nullptr) {
815+
return Local<String>();
816+
}
801817

802818
size_t written = base64_encode(buf, buflen, dst, dlen);
803819
CHECK_EQ(written, dlen);
804820

805821
if (dlen < EXTERN_APEX) {
806822
val = OneByteString(isolate, dst, dlen);
807-
delete[] dst;
823+
free(dst);
808824
} else {
809825
val = ExternOneByteString::New(isolate, dst, dlen);
810826
}
@@ -813,13 +829,16 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
813829

814830
case HEX: {
815831
size_t dlen = buflen * 2;
816-
char* dst = new char[dlen];
832+
char* dst = static_cast<char*>(malloc(dlen));
833+
if (dst == nullptr) {
834+
return Local<String>();
835+
}
817836
size_t written = hex_encode(buf, buflen, dst, dlen);
818837
CHECK_EQ(written, dlen);
819838

820839
if (dlen < EXTERN_APEX) {
821840
val = OneByteString(isolate, dst, dlen);
822-
delete[] dst;
841+
free(dst);
823842
} else {
824843
val = ExternOneByteString::New(isolate, dst, dlen);
825844
}
Collapse file

‎test/parallel/test-stringbytes-external.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-stringbytes-external.js
+66Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,3 +107,69 @@ var PRE_3OF4_APEX = Math.ceil((EXTERN_APEX / 4) * 3) - RADIOS;
107107
assert.equal(a, b);
108108
assert.equal(b, c);
109109
})();
110+
111+
// v8 fails silently if string length > v8::String::kMaxLength
112+
(function() {
113+
// v8::String::kMaxLength defined in v8.h
114+
const kStringMaxLength = process.binding('buffer').kStringMaxLength;
115+
116+
assert.throws(function() {
117+
new Buffer(kStringMaxLength + 1).toString();
118+
}, /toString failed|Buffer allocation failed/);
119+
120+
try {
121+
new Buffer(kStringMaxLength * 4);
122+
} catch(e) {
123+
assert.equal(e.message, 'Buffer allocation failed - process out of memory');
124+
console.log(
125+
'1..0 # Skipped: intensive toString tests due to memory confinements');
126+
return;
127+
}
128+
129+
assert.throws(function() {
130+
new Buffer(kStringMaxLength + 1).toString('ascii');
131+
}, /toString failed/);
132+
133+
assert.throws(function() {
134+
new Buffer(kStringMaxLength + 1).toString('utf8');
135+
}, /toString failed/);
136+
137+
assert.throws(function() {
138+
new Buffer(kStringMaxLength * 2 + 2).toString('utf16le');
139+
}, /toString failed/);
140+
141+
assert.throws(function() {
142+
new Buffer(kStringMaxLength + 1).toString('binary');
143+
}, /toString failed/);
144+
145+
assert.throws(function() {
146+
new Buffer(kStringMaxLength + 1).toString('base64');
147+
}, /toString failed/);
148+
149+
assert.throws(function() {
150+
new Buffer(kStringMaxLength + 1).toString('hex');
151+
}, /toString failed/);
152+
153+
var maxString = new Buffer(kStringMaxLength).toString();
154+
assert.equal(maxString.length, kStringMaxLength);
155+
// Free the memory early instead of at the end of the next assignment
156+
maxString = undefined;
157+
158+
maxString = new Buffer(kStringMaxLength).toString('binary');
159+
assert.equal(maxString.length, kStringMaxLength);
160+
maxString = undefined;
161+
162+
maxString =
163+
new Buffer(kStringMaxLength + 1).toString('binary', 1);
164+
assert.equal(maxString.length, kStringMaxLength);
165+
maxString = undefined;
166+
167+
maxString =
168+
new Buffer(kStringMaxLength + 1).toString('binary', 0, kStringMaxLength);
169+
assert.equal(maxString.length, kStringMaxLength);
170+
maxString = undefined;
171+
172+
maxString = new Buffer(kStringMaxLength + 2).toString('utf16le');
173+
assert.equal(maxString.length, (kStringMaxLength + 2) / 2);
174+
maxString = undefined;
175+
})();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.