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

buffer: segfault writing values with noAssert=true #8724

Closed
bnoordhuis opened this issue Sep 22, 2016 · 8 comments
Closed

buffer: segfault writing values with noAssert=true #8724

bnoordhuis opened this issue Sep 22, 2016 · 8 comments
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@bnoordhuis
Copy link
Member

Reported by @guidovranken. Test case:

$ gdb --args ./out/Release/node -e 'new Buffer(10).writeFloatBE(1, 0xFFFFFFFF-1000, 1);'
Reading symbols from ./out/Release/node...done.
(gdb) run
# <elided>
Thread 1 "node" received signal SIGSEGV, Segmentation fault.
0x00007ffff6be36be in __memcpy_sse2_unaligned () from /lib64/libc.so.6
Missing separate debuginfos, use: dnf debuginfo-install libgcc-6.1.1-3.fc24.x86_64 libstdc++-6.1.1-3.fc24.x86_64
(gdb) backtrace 5
#0  0x00007ffff6be36be in __memcpy_sse2_unaligned () from /lib64/libc.so.6
#1  0x00000000012cf00c in void node::Buffer::WriteFloatGeneric<float, (node::Endianness)1>(v8::FunctionCallbackInfo<v8::Value> const&) ()
#2  0x0000000000a129dd in v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) ()
#3  0x0000000000a882b8 in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Ha
ndle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) ()
#4  0x0000000000a88fcd in v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) ()

The documentation says this:

`offset` {Integer} Where to start writing. Must satisfy: `0 <= offset <= buf.length - 4`
`noAssert` {Boolean} Skip `value` and `offset` validation? **Default:** `false`

IOW, it's technically allowed for node.js to crash but whether that's actually a good idea is something reasonable people can disagree on. Anyone have opinions on either:

  1. Removing noAssert; i.e., always checking the inputs, or
  2. Skipping out-of-bounds reads and writes?
@bnoordhuis bnoordhuis added the buffer Issues and PRs related to the buffer subsystem. label Sep 22, 2016
@addaleax
Copy link
Member

Skipping out-of-bounds reads and writes?

If it doesn’t benchmark horribly, I think we should do that.

@rvagg
Copy link
Member

rvagg commented Sep 23, 2016

any idea what the cost of permanently turning on noAssert are? that seems like reasonable behaviour to me but I don't really know the implications.

@bnoordhuis
Copy link
Member Author

It performs about the same. This is master with noAssert=true:

 Performance counter stats for 'out/Release/node -e var b = Buffer.alloc(8); for (var i = 0; i < 5e7; ++i) b.writeDoubleLE(1.0, 0, true)' (5 runs):

       2744.897728      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  1.04% )
                 0      context-switches:u        #    0.000 K/sec                  
                 0      cpu-migrations:u          #    0.000 K/sec                  
              2503      page-faults:u             #    0.912 K/sec                    ( +-  0.01% )

       2.745411566 seconds time elapsed                                          ( +-  1.05% )

Master with noAssert=false. Omitting noAssert benchmarks the same:

 Performance counter stats for 'out/Release/node -e var b = Buffer.alloc(8); for (var i = 0; i < 5e7; ++i) b.writeDoubleLE(1.0, 0, false)' (5 runs):

       3019.762714      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  1.01% )
                 0      context-switches:u        #    0.000 K/sec                  
                 0      cpu-migrations:u          #    0.000 K/sec                  
              2504      page-faults:u             #    0.829 K/sec                    ( +-  0.03% )

       3.020504516 seconds time elapsed                                          ( +-  1.02% )

And this is with noAssert removed, see patch below:

 Performance counter stats for 'out/Release/node -e var b = Buffer.alloc(8); for (var i = 0; i < 5e7; ++i) b.writeDoubleLE(1.0, 0)' (5 runs):

       2750.038828      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  0.84% )
                 0      context-switches:u        #    0.000 K/sec                  
                 0      cpu-migrations:u          #    0.000 K/sec                  
              2506      page-faults:u             #    0.911 K/sec                    ( +-  0.02% )

       2.751283658 seconds time elapsed                                          ( +-  0.85% )

Patch:

diff --git a/lib/buffer.js b/lib/buffer.js
index 876bdbe..d3e48a1 100644
--- a/lib/buffer.js
+++ b/lib/buffer.js
@@ -1241,13 +1241,8 @@ Buffer.prototype.writeInt32BE = function(value, offset, noAssert) {
 };


-Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset, noAssert) {
-  val = +val;
-  offset = offset >>> 0;
-  if (!noAssert)
-    binding.writeFloatLE(this, val, offset);
-  else
-    binding.writeFloatLE(this, val, offset, true);
+Buffer.prototype.writeFloatLE = function writeFloatLE(val, offset) {
+  binding.writeFloatLE(this, +val, offset >>> 0);
   return offset + 4;
 };

diff --git a/src/node_buffer.cc b/src/node_buffer.cc
index 4baa8d9..027b963 100644
--- a/src/node_buffer.cc
+++ b/src/node_buffer.cc
@@ -817,12 +817,7 @@ template <typename T, enum Endianness endianness>
 void WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);

-  bool should_assert = args.Length() < 4;
-
-  if (should_assert) {
-    THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
-  }
-
+  THROW_AND_RETURN_UNLESS_BUFFER(env, args[0]);
   Local<Uint8Array> ts_obj = args[0].As<Uint8Array>();
   ArrayBuffer::Contents ts_obj_c = ts_obj->Buffer()->GetContents();
   const size_t ts_obj_offset = ts_obj->ByteOffset();
@@ -837,10 +832,8 @@ void WriteFloatGeneric(const FunctionCallbackInfo<Value>& args) {

   size_t memcpy_num = sizeof(T);

-  if (should_assert) {
-    CHECK_NOT_OOB(offset + memcpy_num >= memcpy_num);
-    CHECK_NOT_OOB(offset + memcpy_num <= ts_obj_length);
-  }
+  CHECK_NOT_OOB(offset + memcpy_num >= memcpy_num);
+  CHECK_NOT_OOB(offset + memcpy_num <= ts_obj_length);

   if (offset + memcpy_num > ts_obj_length)
     memcpy_num = ts_obj_length - offset;

@seishun
Copy link
Contributor

seishun commented Feb 10, 2017

This is just about writing floats and doubles, right? noAssert=false causes a significant performance drop for ints: #11245

TimothyGu added a commit to TimothyGu/node that referenced this issue Mar 19, 2017
Also add test cases for partial writes and invalid indices.

Fixes: nodejs#8724
@bnoordhuis
Copy link
Member Author

I'm reopening this. I can't detect any measurable difference between noAssert=true and removing noAssert altogether, neither for integers nor floating-point numbers, so I think we should just get rid of the flag.

@bnoordhuis bnoordhuis reopened this Mar 27, 2017
@seishun
Copy link
Contributor

seishun commented Mar 27, 2017

@bnoordhuis could you post your benchmark results for ints? I still get a significant difference with noAssert=false, as seen in the last chart in #11245 (comment).

@bnoordhuis
Copy link
Member Author

bnoordhuis commented Mar 27, 2017

Okay, I'm able to reproduce with benchmark/buffers/buffer-write.js when I crank up the number of iterations to 100 million. The default of 1 million iterations finishes too quickly on my machine for the optimizer to really kick in, with the result that the numbers are pretty close.

So, I see about a 3.5x performance improvement with noAssert=true. With some clever open coding I can bring that back to 2.7x.

Something is fishy though: the open-coded version is only 5% slower than a direct this[offset] = value or this[offset >>> 0] = value >>> 0 so why the performance gap with noAssert=true?

I also can't reproduce the gap outside the benchmark with e.g. the test case below; in fact, it's a little slower with the current implementation. I'm starting to think the benchmark is suspect.

Current version:

Buffer.prototype.writeUInt8 = function(value, offset, noAssert) {
  value = +value;
  offset = offset >>> 0;
  if (!noAssert)
    checkInt(this, value, offset, 1, 0xff, 0);
  this[offset] = value;
  return offset + 1;
};

Open-coded version:

function failUInt8(that, value, offset) {
  if (value >= 256)
    throw new TypeError('"value" argument is out of bounds');
  if (offset >= that.length)
    throw new RangeError('Index out of range');
}

Buffer.prototype.writeUInt8 = function(value, offset) {
  value = value >>> 0;
  offset = offset >>> 0;
  if (value >= 256 || offset >= this.length)
    failUInt8(this, value, offset);
  this[offset] = value;
  return offset + 1;
};

Direct version:

Buffer.prototype.writeUInt8 = function(value, offset) {
  this[offset] = value;
};

Simple test:

$ time -p out/Release/node -e '(function() {
  "use strict";
  for (var b = Buffer.alloc(1), i = 0; i < 1e8; ++i)
    b.writeUInt8(i & 255, 0, true);
})()'

MylesBorins pushed a commit that referenced this issue Mar 28, 2017
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Apr 18, 2017
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Apr 19, 2017
Also add test cases for partial writes and invalid indices.

PR-URL: #11927
Fixes: #8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
andrew749 pushed a commit to michielbaird/node that referenced this issue Jul 19, 2017
Also add test cases for partial writes and invalid indices.

PR-URL: nodejs/node#11927
Fixes: nodejs/node#8724
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member

Trott commented Jul 30, 2017

Does TurboFan/Ignition change anything here in either direction as far as performance of our code that handles noAssert?

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Aug 9, 2017
Verify that the receiver is a buffer object before attempting to write
out a floating point value.

Fixes: nodejs#8724
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Aug 9, 2017
Check bounds and throw an exception when reading floating point values
from a buffer.  Before this commit, the process would fail a CHECK and
terminate when `noAssert=true`.

Fixes: nodejs#8724
BridgeAR added a commit to BridgeAR/node that referenced this issue Mar 2, 2018
This ports the Buffer#write(Double|Float)(B|L)E functions to JS.
This fixes a security issue concerning type confusion and fixes
another possible crash in combination with `noAssert`.
In addition to that it will also significantly improve the write
performance.

Fixes: nodejs#12179
Fixes: nodejs#8724
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
This ports the Buffer#write(Double|Float)(B|L)E functions to JS.
This fixes a security issue concerning type confusion and fixes
another possible crash in combination with `noAssert`.
In addition to that it will also significantly improve the write
performance.

Fixes: nodejs#12179
Fixes: nodejs#8724

PR-URL: nodejs#18395
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants