buffer: prevent failed assertions #1922

brendanashworth · 2015-06-09T06:28:35Z

This commit adds assert() calls in JS, which prevents non-buffer
instances from triggering failed C++ assertions (killing the process).

They will now die with an AssertionError. Buffer.prototype.copy had to
be wrapped in JS to allow for the assertion, with a modification to the
arguments passed.

Fixes: #1485

Follows #1486.

This commit adds assert() calls in JS, which prevents non-buffer instances from triggering failed C++ assertions (killing the process). They will now die with an AssertionError. Buffer.prototype.copy had to be wrapped in JS to allow for the assertion, with a modification to the arguments passed. Fixes: nodejs#1485

domenic · 2015-06-09T06:30:07Z

lib/buffer.js

@@ -335,6 +336,14 @@ Buffer.byteLength = byteLength;

 // toString(encoding, start=0, end=buffer.length)
 Buffer.prototype.toString = function(encoding, start, end) {
+  // .toString() can be called automatically on non-Buffers.


+Buffer.prototype, it isn't "supported" but it was decided that it shouldn't kill the process. I can add that to the comment, if you'd like.

Yes, and some more detail on what "automatically" means would be great...

The fix seems overcomplicated in any case. Why throw for arguments.length !== 0?

To get here you'd have to call toString.{call,apply}() or to copy it to the prototype of another constructor, etc. Either way you're hosed. Like @domenic mentions, just throw a TypeError if the instanceof check fails.

domenic · 2015-06-09T06:30:44Z

TypeErrors are more conventional here (used in ES and the web for the same purpose). I'd suggest that instead of AssertionErrors.

That also avoids any crazy back-compat breaks that could possibly occur from your change of some of the errors from TypeErrors to AssertionErrors.

trevnorris · 2015-06-09T07:13:36Z

Since you're trying to be overly safe and all, it's worth mentioning that to get past these checks you just have to mess with __proto__. You'd also have to check for external memory to make sure it still works. But even then I can mess with the length property, which will differ from the external array length property retrieved natively. All of these would cause an abort.

What I'm getting at is it's almost always possible to get around the checks in place. We just have to determine where we draw the line, and I'm not sure checking the this of every method call falls within that line.

brendanashworth · 2015-06-09T07:17:11Z

@trevnorris I can stop the type checking at toString() if you'd like. I too don't see so much of a point in checking all the methods, I just figure it's better than a dead process and a confused developer.

trevnorris · 2015-06-09T07:29:52Z

I completely agree that the application should not abort, but I have to draw the line at usage matching or relatively close to the blessed API. Only because there are many ways I know of to cause an abort by doing strange things with the blessed API, and checking all of them would be near impossible. My gauge is the level of effort necessary to cause it. Doing funky things with the prototype then making an explicit call to a custom method is outside that scope.

With that said, I agree that the toString patch is a good one. Because the user doesn't need to explicitly call it in order for it to fire. After looking at your linked fixes I see why the argument length 0 thing. Feel free to leave that in. Just swap it with a type error.

also add comments

brendanashworth · 2015-06-09T07:59:06Z

Thank you @trevnorris & @domenic, I have pushed another commit to refine it to just toString() and change to TypeError, please take another look.

trevnorris · 2015-06-09T16:37:57Z

LGTM. Guess we should go ahead and run CI for consistency.

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

domenic · 2015-06-09T17:23:01Z

lib/buffer.js

@@ -335,6 +335,14 @@ Buffer.byteLength = byteLength;

 // toString(encoding, start=0, end=buffer.length)
 Buffer.prototype.toString = function(encoding, start, end) {
+  // .toString() can be called by JS on prototype (Buffer.prototype + '').
+  if (!(this instanceof Buffer)) {
+    if (arguments.length === 0)


I still don't understand the purpose of this check at all. Why not always return O.p.toString when used on a non-buffer.

domenic · 2015-06-09T17:25:04Z

I completely agree that the application should not abort, but I have to draw the line at usage matching or relatively close to the blessed API. Only because there are many ways I know of to cause an abort by doing strange things with the blessed API, and checking all of them would be near impossible. My gauge is the level of effort necessary to cause it. Doing funky things with the prototype then making an explicit call to a custom method is outside that scope.

If we can get away with it without performance costs, I'd still prefer to avoid crashes in favor of TypeErrors, even for bizarre abuses. I think this should be doable though---just have the C++ code that extracts the char* throw a TypeError if it's not there. Or maybe wait for the Uint8Array changes to land and then throw a TypeError if IsTypedArray is false.

Not a big deal though.

trevnorris · 2015-06-09T17:38:19Z

@domenic Was thinking something similar. The instanceof checks from JS aren't sufficient. Instead I think the best approach would be to throw if HasInstance() returns false. That also has the advantage of being forward compatible with the typed array changes.

I am planning on doing this in another PR so I can make sure to maintain forwards compatibility.

domenic · 2015-06-09T17:43:56Z

Will HasInstance() work with subclassing? I.e. will subclass instances pass the test correctly? (I don't know how HasInstance works really.)

What you really want to verify is "are the internal structures that C++ is going to rely on present." That is why I was thinking IsTypedArray. But HasInstance might be just as good or better.

trevnorris · 2015-06-09T17:47:20Z

Currently HasInstance() only checks if the object has external array data. The future implementation checks the prototype specifically to make sure it matches Buffer's. We don't check the prototype chain because we've never supported subclassing Buffers. Also need to use IsUint8Array() instead of IsTypedArray(), since that's the only type supported.

brendanashworth · 2015-06-09T23:18:54Z

@trevnorris should I continue with this PR then? or would you like to just keep it with yours?

trevnorris · 2015-06-10T04:12:17Z

@brendanashworth feel bad about doing that and scrapping all the work you've done here. Wasn't until I was considering the implementation consequences of your patch that I realized what needs to happen. If you're really alright with it, I'll create a patch. If you can C++ I think the change would be pretty straight forward.

brendanashworth · 2015-06-10T04:17:27Z

@trevnorris no, please, feel free. I like your approach better anyways. I can C, but I can neither C++ nor v8, so you'd probably do a much better job than I would. I'll close this for now, thanks.

trevnorris · 2015-06-10T04:19:46Z

Thanks much for your work here. Wouldn't have realized what was necessary otherwise.

In that case I'll throw for all improper usages except toString().

brendanashworth · 2015-06-19T04:40:27Z

cross-ref: #2012

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 <[email protected]>

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 <[email protected]>

domenic reviewed Jun 9, 2015
View reviewed changes

brendanashworth added the buffer Issues and PRs related to the buffer subsystem. label Jun 9, 2015

!fixup remove checks besides toString()

ece2b4e

also add comments

domenic reviewed Jun 9, 2015
View reviewed changes

brendanashworth closed this Jun 10, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: prevent failed assertions #1922

buffer: prevent failed assertions #1922

brendanashworth commented Jun 9, 2015

domenic Jun 9, 2015

brendanashworth Jun 9, 2015

domenic Jun 9, 2015

trevnorris Jun 9, 2015

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 9, 2015

domenic Jun 9, 2015

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 10, 2015

brendanashworth commented Jun 10, 2015

trevnorris commented Jun 10, 2015

brendanashworth commented Jun 19, 2015

buffer: prevent failed assertions #1922

buffer: prevent failed assertions #1922

Conversation

brendanashworth commented Jun 9, 2015

domenic Jun 9, 2015

Choose a reason for hiding this comment

brendanashworth Jun 9, 2015

Choose a reason for hiding this comment

domenic Jun 9, 2015

Choose a reason for hiding this comment

trevnorris Jun 9, 2015

Choose a reason for hiding this comment

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 9, 2015

domenic Jun 9, 2015

Choose a reason for hiding this comment

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

domenic commented Jun 9, 2015

trevnorris commented Jun 9, 2015

brendanashworth commented Jun 9, 2015

trevnorris commented Jun 10, 2015

brendanashworth commented Jun 10, 2015

trevnorris commented Jun 10, 2015

brendanashworth commented Jun 19, 2015