buffer: fix segfaults when `this` is not a Buffer. #1486

monsanto · 2015-04-21T06:23:49Z

Return Object.prototype.toString(this) in such a case; this approach is inspired by Array.prototype.toString.

mscdex · 2015-04-21T06:30:49Z

Won't this technically be a problem for any of the prototype methods, not just toString()?

JacksonTian · 2015-04-21T07:26:39Z

> Array.prototype.toString()
''
> Object.prototype.toString()
'[object Object]'
> Date.prototype.toString()
'Invalid Date'
> RegExp.prototype.toString()
'/(?:)/'
> ArrayBuffer.prototype.toString()
'[object Object]'

If don't overwrite toString in prototype chain, the Object.prototype.toString will be used. Maybe just output "" is ok.

monsanto · 2015-04-21T07:34:47Z

@mscdex I would consider Buffer.prototype.toString a special case because it can be called implicitly, and should probably not throw if called.

But, yes, it's a problem, none of the C++ functions typecheck this. Also some of the typechecks that are already in node_buffer.cc are bugged out--for example, you can segfault by calling Buffer#copy w/ null or undefined as the first argument. I've fixed that case locally, I'm working on checking this on everything else.

monsanto · 2015-04-21T09:32:27Z

I think that's all of them!

It is not safe to check HasIndexedPropertiesInExternalArrayData() on an arbitrary value you have coerced with ToObject or .As<Object>. It will cause a segfault on null and undefined. To this end, ARGS_THIS now requires a value, which it will check for being an object before checking the indexed properties. It implicitly assumes that there is an environment called env in scope, and as a consequence a few functions gained an env.

Also, there were a few instances where there was a redundant ASSERT(obj->IsObject()); these are removed as ARGS_THIS subsumes their functionality.

bnoordhuis · 2015-04-21T10:21:20Z

src/node_buffer.cc

-  Local<Object> obj = argT;                                                 \
+  Handle<Value> _obj = argT;                                                \
+  if (!HasInstance(_obj)) {                                                 \
+    return env->ThrowTypeError("must be called on Buffer instance");        \


I'm not a fan of control flow in macros and I think that the buffer code in general is overdue for an overhaul.

I want to suggest a more structured approach where the JS prototype methods typecheck the this argument before passing it on to their C++ counterparts. I imagine it would look something like this (using CamelCase for dramatic effect):

const binding = process.binding('buffer'); const BindingIsBuffer = binding.IsBuffer; const BindingToString = binding.ToString; const BindingUtf8Slice = binding.Utf8Slice; // ... // keep a reference to Object#toString, the user may monkey-patch it const ObjectProtoToString = Object.prototype.toString; function IsBuffer(obj) { return typeof(obj) === 'object' && obj !== null && BindingIsBuffer(obj); } // ... Buffer.prototype.toString = function() { if (!IsBuffer(this)) return ObjectProtoToString(this); if (this.length === 0) return ''; return BindingToString(this); }; Buffer.prototype.utf8Slice = function(start, end) { if (!IsBuffer(this)) throw new TypeError('this is not an instance of Buffer'); // validate and coerce start and end if (this.length === 0 && start === 0 && end === 0) return ''; return BindingUtf8Slice(this, start, end); }; // etc.

In short, be paranoid and rigorous. It can be more paranoid still; for example, the user may have replaced this.length with a getter so it would be prudent to read it only once.

I think this is a good approach. setupBufferJS needs to go if we do this. It's weird that part of the methods are added in C++ land and some are added in JS land anyway.

@bnoordhuis' approach is the correct one. when I first rewrote Buffer I did it that way because the diff was already huge and using setupBufferJS was less diff noise in the PR. Plus it maintained how things had been done before.

@bnoordhuis you have plans to rewrite Buffer, or want me to? actually, um. do we just want to hold off on this since Buffer has to be completely rewritten anyway since 4.4 will break everything?

um. do we just want to hold off on this since Buffer has to be completely rewritten anyway since 4.4 will break everything?

If it's gona be re-written might as well re-write it to use ~~ArrayBuffers~~ TypedArrays by the sounds of it. :S

@Fishrock123 problem is that's a massive break. Don't even think nan can deal with it in a backwards compatible way. Since class won't be out until 2 (since we're increasing the major, i guess...). Are we going to require this be a 3 release?

Since class won't be out until 2 (since we're increasing the major, i guess...).

That is actually about now. Class-in-strict will be live in 2.0 due to 4.2, iirc.

Are we going to require this be a 3 release?

Well, v8 4.3 is already going to breaking enough to warrant a 3.0, right? :/

so stupid. whatever.

on a technical note, we'll need to wait until 2.0 until Buffers are implemented as Typed Arrays. That can be yet another breaking change added to the 3.x list.

OK, so what should I do with this PR? It is no problem for me to rewrite this in @bnoordhuis' style, but if that would not be useful due to the V8 4.4 breakage I can also not bother.

If there is going to be another patch release before V8 4.4 & the rewrite I recommend at least taking my first two commits (the .toString() fix and the .copy() fix). They are small, don't touch the ARGS_THIS macro, and fix problems that are easy to trigger. The copy bug is especially bad because anyBuffer.copy(obj.misspelledProperty) segfaults.

mscdex · 2015-04-21T19:23:45Z

I'm not 100% sure about what the current status is on LTS releases, but I would imagine that even though Buffer is going to be rewritten/heavily modified because of upcoming V8 changes, that these changes could at least land in 1.x?

trevnorris · 2015-04-21T20:20:45Z

@iojs/tc Thoughts on making sure this change gets in to v1.x regardless of whether a v2 is coming out soon?

Fishrock123 · 2015-04-21T20:21:57Z

@trevnorris land in master and backport?

trevnorris · 2015-04-22T15:03:06Z

Fine by me. Will need the changes @bnoordhuis explained though.

monsanto · 2015-04-24T19:28:02Z

It looks like the copy() part of this PR was reported, and a fix was landed immediately. I asked here 3 days ago if people would cherry pick this fix, but I was ignored, and instead asked to rewrite a good bit of Buffer. It isn't a big deal to me since it was such a small amount of code, but it still feels a bit rude since I know @trevnorris was aware of this PR, and I now feel like I've been led on a bit of a wild goose chase wrt the rewrite.

trevnorris · 2015-04-24T20:20:11Z

@monsanto I'd like to apologize for the confusion. This PR popped up the same day I learned V8 was removing the API that Buffer currently uses, and so was heavily on my mind. And my comments about the changes @bnoordhuis suggested were mainly about the technique he was explaining.

The reason the copy() fix landed was because someone pinged me about that specific case, and TBH I forgot about this PR (I forget about my own PRs constantly, so please don't be offended). That individual fix was very straight forward. The change here is a little more in depth.

If there's a quick fix to prevent a user from being able to segfault from JS then it should land. The user should never be able to segfault from JS using blessed API. I'll finish doing my review now.

trevnorris · 2015-04-24T20:20:29Z

lib/buffer.js

@@ -302,6 +302,10 @@ Buffer.byteLength = byteLength;

 // toString(encoding, start=0, end=buffer.length)
 Buffer.prototype.toString = function(encoding, start, end) {
+  if (!(this instanceof Buffer)) {
+    return Object.prototype.toString.call(this);
+  }


style: no need for the { }.

monsanto · 2015-04-24T20:44:31Z

@trevnorris it's fine, and don't worry about reviewing the changes, it's changed up a lot locally due to moving the checks to JS land as requested. I didn't have time to finish up that work yesterday but I can try to get it out today.

monsanto · 2015-04-25T10:25:30Z

@trevnorris @bnoordhuis I have moved all type checks (and a few others) to JS land.

I have less free time than I expected so the code is not as paranoid as requested. For example, it uses b instanceof Buffer (the established norm in the file) instead of a more involved C++ land check. There are also some validations in C++ land that could have been moved to JS land. Since the code is slated to be rewritten it is hard to be motivated about covering all of these bases, I am just interested in fixing the segfaults in the most straightforward way possible.

Hopefully this is useful, and the extra paranoia/etc can be added in a follow up PR. Otherwise I am going to have to bow out. Sorry!

Fishrock123 · 2015-04-25T13:39:49Z

~~Some CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/~~

trevnorris · 2015-04-27T15:50:56Z

lib/buffer.js

+    if (arguments.length === 0)
+      return Object.prototype.toString.call(this);
+    else
+      throw new TypeError('this is not an instance of Buffer');


Even though this case is insane, it inevitably will break someone in the wild. Because of that this is a sem-minor patch.

which part will break? if this is not a Buffer the process will crash

hehe. okay true enough. in that case I'd consider this a bug fix patch. :)

Fishrock123 · 2015-04-27T15:56:54Z

Oops, I just realized my link didn't link to the actual CI run. :S

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/590/

trevnorris · 2015-04-27T16:00:54Z

src/node_buffer.cc

@@ -19,7 +19,7 @@
  } while (0)

 #define ARGS_THIS(argT)                                                     \
-  Local<Object> obj = argT;                                                 \
+  Local<Object> obj = argT.As<Object>();                                    \


can't exclude the proper checks from C++. example:

function F() { } F.prototype.__proto__ = Buffer.prototype; var f = new F(); f.write(); // ABORT

The above passes the instanceof check. The only think you can ensure is that it's an Object, but not that it's been allocated external memory. Still run the Buffer::HasInstance() check on all incoming objects.

Fishrock123 · 2015-05-12T16:57:45Z

ping @monsanto

monsanto · 2015-05-20T04:32:28Z

sorry @Fishrock123, this PR went from a 5 min fix to something much larger and I don't have the time for it anymore esp. considering this will all be moot for the Buffer rewrite. If someone would like to take the code I have here as their own feel free.

trevnorris · 2015-06-09T07:34:06Z

@monsanto Thanks for all your work on this. You plan on picking it back up, or mind if I make a PR implementing some similar changes you have here?

monsanto · 2015-06-09T08:23:40Z

@trevnorris it's all yours!

If an object's prototype is munged it's possible to bypass the instanceof check and cause the application to abort. Instead now use HasInstance() to verify that the object is a Buffer, and throw if not. This check will not work for JS only methods. So while the application won't abort, it also won't throw. In order to properly throw in all cases with toString() the JS optimization of checking that length is zero has been removed. In its place the native methods will now return early if a zero length string is detected. Ref: #1486 Ref: #1922 Fixes: #1485 PR-URL: #2012 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

trevnorris · 2015-06-25T19:10:54Z

Done in 1cd9eeb.

If an object's prototype is munged it's possible to bypass the instanceof check and cause the application to abort. Instead now use HasInstance() to verify that the object is a Buffer, and throw if not. This check will not work for JS only methods. So while the application won't abort, it also won't throw. In order to properly throw in all cases with toString() the JS optimization of checking that length is zero has been removed. In its place the native methods will now return early if a zero length string is detected. Ref: nodejs#1486 Ref: nodejs#1922 Fixes: nodejs#1485 PR-URL: nodejs#2012 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

mscdex added the buffer Issues and PRs related to the buffer subsystem. label Apr 21, 2015

monsanto changed the title ~~buffer: fix toString when this is not a Buffer.~~ buffer: fix segfaults when this is not a Buffer. Apr 21, 2015

bnoordhuis reviewed Apr 21, 2015
View reviewed changes

trevnorris reviewed Apr 24, 2015
View reviewed changes

monsanto added 2 commits April 25, 2015 06:01

move all prototype initialization to JS

9f61af1

validate buffer instance consistently

00da460

trevnorris reviewed Apr 27, 2015
View reviewed changes

bnoordhuis force-pushed the master branch from 311c3e4 to b926718 Compare June 2, 2015 17:25

brendanashworth mentioned this pull request Jun 9, 2015

buffer: prevent failed assertions #1922

Closed

trevnorris mentioned this pull request Jun 18, 2015

buffer: prevent abort on bad proto #2012

Closed

rvagg force-pushed the master branch from 9d21135 to 628a3ab Compare June 25, 2015 09:18

trevnorris closed this Jun 25, 2015

snyk-bot mentioned this pull request Jul 14, 2021

[Snyk] Upgrade protobufjs from 6.8.8 to 6.11.2 XirdigH/node-22#27

Open

snyk-bot mentioned this pull request Feb 12, 2022

[Snyk] Upgrade protobufjs from 6.8.8 to 6.11.2 Iceymann18777/node#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: fix segfaults when `this` is not a Buffer. #1486

buffer: fix segfaults when `this` is not a Buffer. #1486

monsanto commented Apr 21, 2015

mscdex commented Apr 21, 2015

JacksonTian commented Apr 21, 2015

monsanto commented Apr 21, 2015

monsanto commented Apr 21, 2015

bnoordhuis Apr 21, 2015

monsanto Apr 21, 2015

trevnorris Apr 21, 2015

Fishrock123 Apr 21, 2015

trevnorris Apr 21, 2015

Fishrock123 Apr 21, 2015

trevnorris Apr 21, 2015

monsanto Apr 21, 2015

mscdex commented Apr 21, 2015

trevnorris commented Apr 21, 2015

Fishrock123 commented Apr 21, 2015

trevnorris commented Apr 22, 2015

monsanto commented Apr 24, 2015

trevnorris commented Apr 24, 2015

trevnorris Apr 24, 2015

monsanto commented Apr 24, 2015

monsanto commented Apr 25, 2015

Fishrock123 commented Apr 25, 2015

trevnorris Apr 27, 2015

monsanto Apr 27, 2015

trevnorris Apr 27, 2015

Fishrock123 commented Apr 27, 2015

trevnorris Apr 27, 2015

Fishrock123 commented May 12, 2015

monsanto commented May 20, 2015

trevnorris commented Jun 9, 2015

monsanto commented Jun 9, 2015

trevnorris commented Jun 25, 2015

buffer: fix segfaults when this is not a Buffer. #1486

buffer: fix segfaults when this is not a Buffer. #1486

Conversation

monsanto commented Apr 21, 2015

mscdex commented Apr 21, 2015

JacksonTian commented Apr 21, 2015

monsanto commented Apr 21, 2015

monsanto commented Apr 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Apr 21, 2015

trevnorris commented Apr 21, 2015

Fishrock123 commented Apr 21, 2015

trevnorris commented Apr 22, 2015

monsanto commented Apr 24, 2015

trevnorris commented Apr 24, 2015

Choose a reason for hiding this comment

monsanto commented Apr 24, 2015

monsanto commented Apr 25, 2015

Fishrock123 commented Apr 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Apr 27, 2015

Choose a reason for hiding this comment

Fishrock123 commented May 12, 2015

monsanto commented May 20, 2015

trevnorris commented Jun 9, 2015

monsanto commented Jun 9, 2015

trevnorris commented Jun 25, 2015

buffer: fix segfaults when `this` is not a Buffer. #1486

buffer: fix segfaults when `this` is not a Buffer. #1486