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.

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

V8 Fatal Error when trying to convert a buffered integer to string #649

Closed
kevinmartin opened this issue Jan 29, 2015 · 23 comments
Closed
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@kevinmartin
Copy link

IO.js Version 1.0.4

$ iojs
> Date.now();
1422508723712
> Buffer(Date.now());
<Buffer 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... >
> Buffer(Date.now()).toString('utf8');


#
# Fatal error in ../deps/v8/src/handles.h, line 48
# CHECK(location_ != NULL) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::String::NewFromUtf8(v8::Isolate*, char const*, v8::String::NewStringType, int)
 3: node::StringBytes::Encode(v8::Isolate*, char const*, unsigned long, node::encoding)
 4: node::Buffer::Utf8Slice(v8::FunctionCallbackInfo<v8::Value> const&)
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
 6: ??
 7: ??
Illegal instruction
@jonathanong
Copy link
Contributor

Date.now() is an integer, so you're creating a buffer with Date.now() bytes... which is gigantic. I think you want new Buffer(String(Date.now()))

@kevinmartin
Copy link
Author

I understand what's wrong, haha. I'm just wondering why its giving a scary V8 error like that instead of a generic Javascript error.

In Node 0.10.31:

$ node
> Buffer(5).toString('utf8')
'\u0000 \u0000\u0000\u0000'
> Buffer(Date.now()).toString('utf8')
TypeError: Bad argument
    at new Buffer (buffer.js:194:21)
    at Buffer (buffer.js:158:12)
    at repl:1:2
    at REPLServer.self.eval (repl.js:110:21)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.emit (events.js:95:17)
    at Interface._onLine (readline.js:202:10)
    at Interface._line (readline.js:531:8)
    at Interface._ttyWrite (readline.js:760:14)
    at ReadStream.onkeypress (readline.js:99:10)

@brendanashworth
Copy link
Contributor

@kevinmartin well, it does have an extraordinary length.

> var buf = new Buffer(Date.now())
undefined
> buf.length
878611666

@targos
Copy link
Member

targos commented Jan 29, 2015

this issue reminds me of similar crash I had last year on node. I just retried it on iojs and it is still crashing.

test.js :

var fs = require('fs');
var zlib = require('zlib');

fs.readFile('./Compound_006625001_006650000.sdf.gz', function (err, content) {
  zlib.gunzip(content, function (err, sdf) {
    sdf.toString();
  });
});

The gzipped file : ftp://ftp.ncbi.nlm.nih.gov/pubchem/Compound/CURRENT-Full/SDF/Compound_006625001_006650000.sdf.gz

> iojs test.js
#
# Fatal error in ../deps/v8/src/handles.h, line 48
# CHECK(location_ != NULL) failed
#

==== C stack trace ===============================

 1: V8_Fatal
 2: v8::String::NewFromUtf8(v8::Isolate*, char const*, v8::String::NewStringType, int)
 3: node::StringBytes::Encode(v8::Isolate*, char const*, unsigned long, node::encoding)
 4: node::Buffer::Utf8Slice(v8::FunctionCallbackInfo<v8::Value> const&)
 5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&))
 6: ??
 7: ??
zsh: illegal hardware instruction (core dumped)  iojs test.js

The uncompressed file is valid utf-8.
Could it be a v8 limit on the size (it is 276,9 MB) ?

@bnoordhuis
Copy link
Member

There is a logic error in lib/buffer.js: the length argument to the constructor gets coerced to uint32 with length >>> 0, which makes it bypass the length < kMaxLength check because e.g. (1422546646261 >>> 0) < 0x3fffffff.

In short, confirmed. :-)

@Fishrock123 Fishrock123 added buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs. labels Jan 29, 2015
@cjb
Copy link

cjb commented Jan 29, 2015

I think @bnoordhuis found a bug, but I don't think it was this bug. The length > kMaxLength after length >>> 0 check he mentions is part of SlowBuffer(), and that's not called at all in this stack trace.

We just have a plain Buffer (for which util.isNumber(subject) returns true), and when it hits the if (this.length > kMaxLength) check at lib/buffer.js:52 it has a length of 921032252 and a kMaxLength of 1073741823, so it's under the limit at 85% of kMaxLength.

@bnoordhuis
Copy link
Member

See #657 for a proposed mitigation. It's not perfect because the real issue lies in V8, it aborts when internal operations OOM. For example, Buffer(0x3fffffff-1).toString() still hits the assert.

@bnoordhuis
Copy link
Member

@bnoordhuis
Copy link
Member

I've been poking around V8 and I do believe I understand the motivation for aborting. When you apply this patch:

diff --git a/deps/v8/src/api.cc b/deps/v8/src/api.cc
index 88d3c88..04f0295 100644
--- a/deps/v8/src/api.cc
+++ b/deps/v8/src/api.cc
@@ -5464,10 +5464,14 @@ inline Local<String> NewString(Isolate* v8_isolate,
   ENTER_V8(isolate);
   if (length == -1) length = StringLength(data);
   // We do not expect this to fail. Change this if it does.
-  i::Handle<i::String> result = NewString(
+  i::MaybeHandle<i::String> maybe_result = NewString(
       isolate->factory(),
       type,
-      i::Vector<const Char>(data, length)).ToHandleChecked();
+      i::Vector<const Char>(data, length));
+  if (maybe_result.is_null()) {
+    return Local<String>();
+  }
+  i::Handle<i::String> result = maybe_result.ToHandleChecked();
   if (type == String::kUndetectableString) {
     result->MarkAsUndetectable();
   }

And run iojs -p 'Buffer(0x3ffffffe).toString()', it prints undefined. But when you also apply this patch:

diff --git a/lib/buffer.js b/lib/buffer.js
index 41de85e..c0340f6 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -231,35 +231,48 @@ Buffer.prototype.toString = function(encoding, start, end) {
   if (end <= start) return '';

   while (true) {
+    let result;
     switch (encoding) {
       case 'hex':
-        return this.hexSlice(start, end);
+        result = this.hexSlice(start, end);
+        break;

       case 'utf8':
       case 'utf-8':
-        return this.utf8Slice(start, end);
+        result = this.utf8Slice(start, end);
+        break;

       case 'ascii':
-        return this.asciiSlice(start, end);
+        result = this.asciiSlice(start, end);
+        break;

       case 'binary':
-        return this.binarySlice(start, end);
+        result = this.binarySlice(start, end);
+        break;

       case 'base64':
-        return this.base64Slice(start, end);
+        result = this.base64Slice(start, end);
+        break;

       case 'ucs2':
       case 'ucs-2':
       case 'utf16le':
       case 'utf-16le':
-        return this.ucs2Slice(start, end);
+        result = this.ucs2Slice(start, end);
+        break;

       default:
         if (loweredCase)
           throw new TypeError('Unknown encoding: ' + encoding);
         encoding = (encoding + '').toLowerCase();
         loweredCase = true;
+        continue;
     }
     }
+
+    if (result === undefined)
+      throw new Error('Failed to convert buffer to string. Not enough memory?');
+
+    return result;
   }
 };

That same test fails with:

$ out/Release/iojs -p 'Buffer(0x3ffffffe).toString()'
FATAL ERROR: invalid array length Allocation failed - process out of memory

I speculate that there simply isn't enough memory left to create the exception object.

@meandmycode
Copy link

Love the irony of not having enough memory to build an oom error, I know in .NET at least they pre allocate their oom so they can better grantee they can throw it later if needed. Not sure how relevant that is though.

@bnoordhuis
Copy link
Member

@meandmycode I believe V8 used to do something similar. That they moved away from that approach probably means it wasn't bulletproof. I remember some discussion about it on v8-users or v8-dev but I can't find the thread now.

@targos
Copy link
Member

targos commented Jan 30, 2015

@bnoordhuis What memory are we talking about ? Certainly not the system's RAM, since I have more than 6GB free when I try to convert my buffer to string.
And the size of this buffer is less than kMaxLength...

@bnoordhuis
Copy link
Member

@targos The JS heap, default ~1.5 GB and configurable with --max_old_space_size (but see #648.)

And the size of this buffer is less than kMaxLength...

Yes, but the result of buf.toString() isn't.

@bnoordhuis
Copy link
Member

#657 landed. I'm leaving this issue open pending https://code.google.com/p/v8/issues/detail?id=3853.

@trevnorris
Copy link
Contributor

Fixed by v8/v8@8aae1b3

@petkaantonov
Copy link
Contributor

The default/utf-8 .toString() should also allocate the string externally for >1mb buffers (in fact why don't we always allocate externally? the js-heap is notoriously limited and we should allocate there as little as possible). I cannot see any good reason to allocate such strings in the js heap at all.

@bnoordhuis
Copy link
Member

@petkaantonov Externalized strings need special handling by the GC so I'm not sure if it would be a win for small strings. I can also see it making fragmentation of the C/C++ heap worse.

The current 1+ MB cutoff point may be on the high side but some careful benchmarking is in order to find out what a better cutoff point is.

@trevnorris
Copy link
Contributor

@petkaantonov External strings can only be one or two byte strings. Thus excluding the possibility of an external UTF8 string.

@trevnorris
Copy link
Contributor

Also, the cutoff point that's currently in there was chosen after meticulous benchmarking. Here's a basic graph showing what happens to allocation time as string size increases: http://trevnorris.github.io/nodeconf.eu/#/17

@bnoordhuis
Copy link
Member

External strings can only be one or two byte strings. Thus excluding the possibility of an external UTF8 string.

True, but internal strings aren't UTF-8 either, V8 always decodes them to ASCII or UTF-16.

@petkaantonov
Copy link
Contributor

True, but internal strings aren't UTF-8 either, V8 always decodes them to ASCII or UTF-16.

Yes, we are not gaining anything by outsourcing this work to v8. We can do it ourselves and make it external.

@trevnorris
Copy link
Contributor

Sure, but do we really want to implement our own encoding parser so we can externalize all strings?

@petkaantonov
Copy link
Contributor

We don't have to implement our own, even though it's a fun task.

@Fishrock123 Fishrock123 added the v8 engine Issues and PRs related to the V8 dependency. label Apr 8, 2015
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 4, 2016
- pass a regexp to assert.throws()
joyeecheung added a commit to joyeecheung/node that referenced this issue Dec 4, 2016
jasnell pushed a commit that referenced this issue Dec 6, 2016
pass a regexp to assert.throws()

PR-URL: #9924
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Dec 8, 2016
pass a regexp to assert.throws()

PR-URL: #9924
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this issue Dec 8, 2016
pass a regexp to assert.throws()

PR-URL: nodejs#9924
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 21, 2016
pass a regexp to assert.throws()

PR-URL: #9924
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants