FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: fix memory leak in ExternString · nodejs/node@5201cb0 · GitHub

/ node Public

Commit 5201cb0

Browse files
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 93ba585 commit 5201cb0

4 files changed

Lines changed: 107 additions & 14 deletions

File tree

‎lib/buffer.js‎

Lines 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

‎src/node_buffer.cc‎

Lines 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

‎src/string_bytes.cc‎

Lines 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
}

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

Lines 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)
Back | FazBrowse Home | New Git URL