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

skilledDeveloper · 2015-12-23T03:38:39Z

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

Original commit message: api: introduce SealHandleScope When debugging Handle leaks in io.js we found it very convenient to be able to Seal some specific (root in our case) scope to prevent Handle allocations in it, and easily find leakage. R=yangguo BUG= Review URL: https://codereview.chromium.org/1079713002 Cr-Commit-Position: refs/heads/master@{nodejs#27766} Should help us identify and fix Handle leaks in core and user-space code. PR-URL: nodejs#3945 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James Snell <jasnell@gmail.com>

Helps to find Handle leaks in Debug mode. Ref: a5244d3 "deps: backport 1f8555 from v8's upstream" PR-URL: nodejs#3945 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James Snell <jasnell@gmail.com>

The call to node::Environment::GetCurrent(Isolate*) makes the call to v8::Isolate::GetCurrentContext(). Doing so creates a new handle that bubbled to the v8::SealHandleScope(). PR-URL: nodejs#3945 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James Snell <jasnell@gmail.com>

test-domain-exit-dispose-again had been written for node v0.10.x, and was using the fact that callbacks scheduled with `process.nextTick` wouldn't run if the domain attached to it was disposed. This is not longer the case, and as a result the test would not catch any regression: it would always pass. This change rewrites that test to check that the current domain is cleared properly when processing the rest of the timers list if a timer's callback throws an error. This makes the test fail without the original fix, and pass with the original fix, as expected. PR: nodejs#3991 PR-URL: nodejs#3991 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

* Include reference to CVE-2015-8027 * Fix "socket may no longer have a socket" reference * Expand on non-existent parser causing the error * Clarify that CVE-2015-3194 affects TLS servers using _client certificate authentication_ PR-URL: nodejs#4154 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Backport the tools/install.py changes from 628a3ab that were missed when 6fb0b92 backported the corresponding changes to the Makefile to build the headers only archive. PR-URL: nodejs#4149 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rod Vagg <rod@vagg.org>

PR nodejs#3890 [1] introduced the variable ALLOW_INSECURE_SERVER_DHPARAM defined in src/node_crypto.cc. However, if nodejs is built without OpenSSL support, the build fails: error: ‘ALLOW_INSECURE_SERVER_DHPARAM’ was not declared in this scope ALLOW_INSECURE_SERVER_DHPARAM = true; Fix this by using the preprocessor macro HAVE_OPENSSL to opt-out the use of ALLOW_INSECURE_SERVER_DHPARAM in non-OpenSSL builds. [1] nodejs#3890 PR-URL: nodejs#4201 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Rod Vagg <rod@vagg.org>

d1ba82a "fixed" test-domain-exit-dispose-again by changing its logic to test that process.domain was cleared properly in case an error was thrown from a timer's callback. However, it became clear when reviewing a recent change that refactors lib/timers.js that it was not quite the intention of the original test. Thus, this change adds the original implementation of test-domain-exit-dispose-again back, with comments that make its implementation easier to understand. It also preserves the changes made by d1ba82a, but it moves them to a new test file named test-timers-reset-process-domain-on-throw.js. PR: nodejs#4278 PR-URL: nodejs#4278 Reviewed-By: James M Snell <jasnell@gmail.com>

Fix node exiting due to an exception being thrown rather than emitting an 'uncaughtException' event on the process object when: 1. no error handler is set on the domain within which an error is thrown 2. an 'uncaughtException' event listener is set on the process Also fix an issue where the process would not abort in the proper function call if an error is thrown within a domain with no error handler and --abort-on-uncaught-exception is used. Fixes nodejs#3607 and nodejs#3653. PR: nodejs#3885 PR-URL: nodejs#3885 Reviewed-By: James M Snell <jasnell@gmail.com>

ofrobots · 2015-12-29T15:54:37Z

@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 · 2015-12-29T17:11:02Z

src/string_bytes.cc

@@ -87,6 +91,10 @@ class ExternString: public ResourceType {
      if (length == 0)
        return scope.Escape(String::Empty(isolate));

+      // Workaround to avoid process crash caused by a possible V8 bug


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 · 2016-01-04T04:30:40Z

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 · 2016-01-07T17:55:41Z

src/string_bytes.cc

@@ -62,6 +62,10 @@ class ExternString: public ResourceType {
      return length_;
    }

+    // Maximum length in bytes that V8 can handle. 
+    // This equals to 268435440 bytes
+    static const size_t kMaxLength = (256 * 1024 * 1024) - 16;


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

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

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

skilledDeveloper · 2016-01-08T18:10:50Z

@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 · 2016-01-10T20:37:05Z

This PR will fix the issue #4266.

jasnell · 2016-01-11T18:20:57Z

test/parallel/test-buffer-tostring-ascii.js

+}
+assert(typeof gc === 'function', 'Run this test with --expose-gc');
+
+try {


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

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 · 2016-01-11T19:32:44Z

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

rvagg · 2016-01-12T00:54:02Z

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 · 2016-03-03T19:05:32Z

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 · 2016-03-03T21:34:32Z

@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 · 2016-03-03T22:22:29Z

@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 · 2016-03-08T18:07:16Z

@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 · 2016-03-15T02:27:42Z

@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);
    throw new Error('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 · 2016-03-16T19:06:11Z

@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 · 2016-03-16T19:56:19Z

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

skilledDeveloper · 2016-04-19T21:42:10Z

@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 · 2016-04-19T23:24:54Z

Let's leave it open and revisit.

Trott · 2016-08-07T00:52:32Z

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 · 2016-08-07T06:04:29Z

+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, @jasnell https://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
.

* Workaround for Buffer.toString failure caused by exceeding the maximum supported length in V8. * See issue nodejs/node#4266 and PR nodejs/node#4394 Fixes #22

rvagg · 2016-12-20T10:06:59Z

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

jasnell · 2016-12-22T01:55:47Z

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

MylesBorins · 2016-12-29T18:03:26Z

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

* Workaround for Buffer.toString failure caused by exceeding the maximum supported length in V8. * See issue nodejs/node#4266 and PR nodejs/node#4394 Fixes #22

karuppasamy20 · 2018-11-26T08:46:31Z

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 · 2018-11-26T09:06:28Z

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

nashid · 2020-05-25T23:13:04Z

What would be the workaround to mitigate this issue?

nashid · 2020-06-01T07:28:28Z

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 · 2020-06-01T11:19:33Z

@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.

trevnorris and others added 9 commits December 4, 2015 14:28

src: use global SealHandleScope

409f6a9

Helps to find Handle leaks in Debug mode. Ref: a5244d3 "deps: backport 1f8555 from v8's upstream" PR-URL: nodejs#3945 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: James Snell <jasnell@gmail.com>

mscdex added buffer Issues and PRs related to the buffer subsystem. v8 engine Issues and PRs related to the V8 dependency. labels Dec 23, 2015

bnoordhuis reviewed Dec 29, 2015
View reviewed changes

trevnorris reviewed Jan 7, 2016
View reviewed changes

skilledDeveloper changed the title ~~Workaround for a possible V8 bug. fixes #4266~~ buffer: workaround for v8 buffer.toString failure. fixes #4266 Jan 10, 2016

MylesBorins added v0.12 labels Jan 11, 2016

silverwind mentioned this pull request Jan 11, 2016

buffer.toString fails for buffers larger than 192MB #4266

Closed

jasnell reviewed Jan 11, 2016
View reviewed changes

jasnell added stalled Issues and PRs that are stalled. blocked PRs that are blocked by other issues or PRs. labels Apr 30, 2016

This was referenced Nov 2, 2016

Uncaught Error: toString failed nteract/hydrogen#491

Closed

Uncaught Error: toString failed n-riesco/jmp#22

Closed

richardlau mentioned this pull request Dec 20, 2016

v0.12.18 release proposal #10352

Merged

jasnell closed this Dec 22, 2016

jasnell reopened this Dec 22, 2016

MylesBorins closed this Dec 29, 2016

hysh mentioned this pull request Mar 22, 2017

Error: "toString()" failed during downloading large files (>256MB) jsforce/jsforce#615

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

skilledDeveloper commented Dec 23, 2015

ofrobots commented Dec 29, 2015

bnoordhuis Dec 29, 2015

rvagg commented Jan 4, 2016

trevnorris Jan 7, 2016

trevnorris Jan 7, 2016

skilledDeveloper Jan 8, 2016

skilledDeveloper commented Jan 8, 2016

skilledDeveloper commented Jan 10, 2016

jasnell Jan 11, 2016

trevnorris Jan 11, 2016

trevnorris commented Jan 11, 2016

rvagg commented Jan 12, 2016

skilledDeveloper commented Mar 3, 2016

trevnorris commented Mar 3, 2016

skilledDeveloper commented Mar 3, 2016

trevnorris commented Mar 8, 2016

skilledDeveloper commented Mar 15, 2016

trevnorris commented Mar 16, 2016

skilledDeveloper commented Mar 16, 2016

skilledDeveloper commented Apr 19, 2016

jasnell commented Apr 19, 2016

Trott commented Aug 7, 2016

jasnell commented Aug 7, 2016

rvagg commented Dec 20, 2016

jasnell commented Dec 22, 2016

MylesBorins commented Dec 29, 2016

karuppasamy20 commented Nov 26, 2018 •

edited

Loading

targos commented Nov 26, 2018

nashid commented May 25, 2020

nashid commented Jun 1, 2020

addaleax commented Jun 1, 2020

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

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

Conversation

skilledDeveloper commented Dec 23, 2015

ofrobots commented Dec 29, 2015

bnoordhuis Dec 29, 2015

Choose a reason for hiding this comment

rvagg commented Jan 4, 2016

trevnorris Jan 7, 2016

Choose a reason for hiding this comment

trevnorris Jan 7, 2016

Choose a reason for hiding this comment

skilledDeveloper Jan 8, 2016

Choose a reason for hiding this comment

skilledDeveloper commented Jan 8, 2016

skilledDeveloper commented Jan 10, 2016

jasnell Jan 11, 2016

Choose a reason for hiding this comment

trevnorris Jan 11, 2016

Choose a reason for hiding this comment

trevnorris commented Jan 11, 2016

rvagg commented Jan 12, 2016

skilledDeveloper commented Mar 3, 2016

trevnorris commented Mar 3, 2016

skilledDeveloper commented Mar 3, 2016

trevnorris commented Mar 8, 2016

skilledDeveloper commented Mar 15, 2016

trevnorris commented Mar 16, 2016

skilledDeveloper commented Mar 16, 2016

skilledDeveloper commented Apr 19, 2016

jasnell commented Apr 19, 2016

Trott commented Aug 7, 2016

jasnell commented Aug 7, 2016

rvagg commented Dec 20, 2016

jasnell commented Dec 22, 2016

MylesBorins commented Dec 29, 2016

karuppasamy20 commented Nov 26, 2018 • edited Loading

targos commented Nov 26, 2018

nashid commented May 25, 2020

nashid commented Jun 1, 2020

addaleax commented Jun 1, 2020

karuppasamy20 commented Nov 26, 2018 •

edited

Loading