buffer: workaround for v8 buffer.toString failure. fixes #4266

[skilledDeveloper]

A workaround to return undefined when Buffer.toString fails because of exceeding the maximum supported length in V8.
@bnoordhuis Please review.

[ofrobots]

@skilledDeveloper Can you update the commit log messages to match the contribution guidelines. Specifically, use subsystem prefix (buffer and test), and restrict the abstract to 50 chars max.

[bnoordhuis]

This comment is too vague; I'd phrase it as // V8 aborts if we try to allocate a string that is too large. (punctuation intentional.)

Shouldn't NewFromCopy() have this logic as well?

[rvagg]

Thanks for this @skilledDeveloper, I think this is your first contribution to core, thanks for taking the time. Please let us know if you need any help sorting out the issues raised by @ofrobots and @bnoordhuis to get this ready for landing.

[trevnorris]

Please use String::kMaxLength instead. More forwards compatible.

[trevnorris]

oh wait. missed that this targets v0.12 branch.

[skilledDeveloper]

That's right. There is no such thing in v0.12 and kStringMaxLength is not exposed anywhere.

[skilledDeveloper]

@rvagg Not a problem at all!
Last night I was able to find some "free" time and applied the reviewer's suggestions. I also added some more tests.
@ofrobots@bnoordhuis@trevnorris@thealphanerd, Please review the changes.

p.s. Jsut noticed that one of the commits messages is still more than 50 chars!

[skilledDeveloper]

This PR will fix the issue #4266.

[jasnell]

Should use an appropriate assert.throws or assert.doesNotThrow here instead of throwing the raw error

[trevnorris]

This is how it's done in all other test-stringbytes-external-*.js tests (on master). We're doing a check if the machine has enough memory available, and quitting the test without error if there's not. No assert should be used.

[trevnorris]

LGTM. This was my last review to prevent myself from making the same irrelevant comments again.

[rvagg]

Need to squash the commits closer to ~1. Without having the time to dive into the details of this myself, @trevnorris or @ofrobots do either of you have suggestions for commit compaction here?

[skilledDeveloper]

Here's the summary of this PR so far:

• We initially wanted to return undefined in case of exceeding the maximum allowed bytes. This has been ready to be landed since last month
• Some argued it's better to throw an exception in such cases (which is more reasonable IMO too). I have tried to do so by checking the return value in buffer.toString but ran into some issues. The suggested solutions has not helped.

[trevnorris]

@skilledDeveloper Below is a patch that fixes things up. Except for the fact that for some reason throwing from toString() causes a FATAL ERROR. Not sure why. @bnoordhuis ?

commit 62c5a08157825a5882aec35c5ab58e900464c303
Author: Trevor Norris <trev.norris@gmail.com>
Date: Thu Mar 3 14:29:50 2016 -0700
FIX IT UP
diff --git a/lib/buffer.js b/lib/buffer.js
index 1a9f7ca..31f721f 100644
--- a/lib/buffer.js+++ b/lib/buffer.js@@ -247,8 +247,6 @@ Buffer.byteLength = function(str, enc) {
// toString(encoding, start=0, end=buffer.length)
Buffer.prototype.toString = function(encoding, start, end) {
- var loweredCase = false;-
start = start >>> 0;
end = util.isUndefined(end) || end === Infinity ? this.length : end >>> 0;
@@ -257,29 +255,41 @@ Buffer.prototype.toString = function(encoding, start, end) {
if (end > this.length) end = this.length;
if (end <= start) return '';
+ var ret = getSlice(this, start, end, encoding);+ if (ret === undefined)+ // XXX Throwing from toString causes:+ // FATAL ERROR: invalid array length Allocation failed+ throw new Error('"toString()" failed');+ return ret;+};+++function getSlice(self, start, end, encoding) {+ var loweredCase = false;+
while (true) {
switch (encoding) {
case 'hex':
- return this.hexSlice(start, end);+ return self.hexSlice(start, end);
case 'utf8':
case 'utf-8':
- return this.utf8Slice(start, end);+ return self.utf8Slice(start, end);
case 'ascii':
- return this.asciiSlice(start, end);+ return self.asciiSlice(start, end);
case 'binary':
- return this.binarySlice(start, end);+ return self.binarySlice(start, end);
case 'base64':
- return this.base64Slice(start, end);+ return self.base64Slice(start, end);
case 'ucs2':
case 'ucs-2':
case 'utf16le':
case 'utf-16le':
- return this.ucs2Slice(start, end);+ return self.ucs2Slice(start, end);
default:
if (loweredCase)
@@ -288,7 +298,7 @@ Buffer.prototype.toString = function(encoding, start, end) {
loweredCase = true;
}
}
-};+}
Buffer.prototype.equals = function equals(b) {
diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index cd66a8a..8fa1b70 100644
--- a/src/node_buffer.cc+++ b/src/node_buffer.cc@@ -262,8 +262,11 @@ void StringSlice(const FunctionCallbackInfo<Value>& args) {
ARGS_THIS(args.This())
SLICE_START_END(args[0], args[1], obj_length)
- args.GetReturnValue().Set(- StringBytes::Encode(env->isolate(), obj_data + start, length, encoding));+ Local<Value> ret =+ StringBytes::Encode(env->isolate(), obj_data + start, length, encoding);++ if (!ret.IsEmpty())+ args.GetReturnValue().Set(ret);
}
diff --git a/src/string_bytes.cc b/src/string_bytes.cc
index 5805c7c..84c4b4c 100644
--- a/src/string_bytes.cc+++ b/src/string_bytes.cc@@ -733,10 +733,14 @@ Local<Value> StringBytes::Encode(Isolate* isolate,
break;
case UTF8:
- val = String::NewFromUtf8(isolate,- buf,- String::kNormalString,- buflen);+ if (buflen > ExternOneByteString::kMaxLength) {+ val = Local<String>();+ } else {+ val = String::NewFromUtf8(isolate,+ buf,+ String::kNormalString,+ buflen);+ }
break;
case BINARY:

[skilledDeveloper]

@trevnorris That's exactly the fatal error I've been talking about. I don't believe it's caused by throwing though. It seems to be caused by variable assignment: var ret = getSlice(...)

[trevnorris]

@skilledDeveloper I'm at a lose. This has got to be some sort of v8 bug. How about we just throw from C++ if the returned handle is empty?

[skilledDeveloper]

@trevnorris@bnoordhuis
I tried throwing error from C++ but regardless of how and where I do it, I get this error and process aborts:
FATAL ERROR: invalid array length Allocation failed - process out of memory

I even tried throwing error first thing in the JS but I got the same error.
Here's the code:

if(this.length>268435440){console.log("len:",this.length);thrownewError('toString failed');}

Here's the result.
I can see the console log message but not the 'toString failed' error!

len: 268435441
FATAL ERROR: invalid array length Allocation failed - process out of memory

[trevnorris]

@skilledDeveloper The following patch may fix that problem. Try this out:

diff --git a/src/node.cc b/src/node.cc
index 624865b..c12c993 100644
--- a/src/node.cc+++ b/src/node.cc@@ -177,21 +177,19 @@ ArrayBufferAllocator ArrayBufferAllocator::the_singleton;
void* ArrayBufferAllocator::Allocate(size_t length) {
if (length > kMaxLength)
return NULL;
- char* data = new char[length];- memset(data, 0, length);- return data;+ return calloc(length, 1);
}
void* ArrayBufferAllocator::AllocateUninitialized(size_t length) {
if (length > kMaxLength)
return NULL;
- return new char[length];+ return malloc(length);
}
void ArrayBufferAllocator::Free(void* data, size_t length) {
- delete[] static_cast<char*>(data);+ free(data);
}

new char throws when it can't allocate. Replacing it with malloc means NULL will be returned allowing the empty handle to propagate.

[skilledDeveloper]

@trevnorris Did not help. Thanks though.
Looks like whenever the buffer is accessed somehow this happens.

[skilledDeveloper]

@trevnorris@bnoordhuis@jasnell@ofrobots@thealphanerd@rvagg@targos@ChALkeR

Here's the summary of this PR so far:

• We initially wanted to return undefined in case of exceeding the maximum allowed bytes. This has been ready to be landed since last month
• Some argued it's better to throw an exception in such cases (which is more reasonable IMO too). I have tried to do so by checking the return value in buffer.toString but ran into some issues. The suggested solutions has not helped.

There is not much progress on this PR and we are apparently blocked by a V8 (or some memory management) bug.

Shall we close this unsolved?
Or do we want to merge the first solution (i.e. returning undefined) which at least prevents the original issue from breaking the node process?

[jasnell]

Let's leave it open and revisit.

[Trott]

More than three months ago, @jasnell wrote:

Let's leave it open and revisit.

Are we at a point where we can make a decision? Seems like the choices are (in order of degree of difficulty):

• close it and don't worry about it
• go with undefined
• figure out what's up with the Buffer stuff so we can throw an error

[jasnell]

+1 on moving forward with something here. Options 2 or 3 sound best but
I'll need to dig back in on the details again.

On Saturday, August 6, 2016, Rich Trott notifications@github.com wrote:

More than three months ago, @jasnellhttps://github.com/jasnell wrote:

Let's leave it open and revisit.

Are we at a point where we can make a decision? Seems like the choices are
(in order of degree of difficulty):

• close it and don't worry about it
• go with undefined
• figure out what's up with the Buffer stuff so we can throw an error

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4394 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eerj6pleBzyZBLym3m_R9Z_JWS_gks5qdSxXgaJpZM4G6XGb
.

[rvagg]

not gonna make it now, EOL around the corner for v0.12, thanks for the effort on this anyway!

[jasnell]

@rvagg ... should we go ahead and close?

[MylesBorins]

@jasnell I'm going to close this assuming you did not mean to repoen

[karuppasamy20]

if any ways to possible to get above 265 MB via nodejs.
i got same error be like Error: "toString()" failed during downloading large files (>256MB)

[targos]

The limit is ~1024 MB from Node 8 if you are on a 64-bit system

[nashid]

What would be the workaround to mitigate this issue?

[nashid]

even when I give more memory to node i.e. node --max-old-space-size=4096 this issue is happening.

What can be a potential work around?

[addaleax]

@nashid Can you open a new issue at https://github.com/nodejs/help/issues/? Whatever you’re seeing is unrelated to this PR, and commenting on a 4-year-old PR isn’t going to give you any helpful information either.