buffer: prevent abort on bad proto #2012
Conversation
| }, TypeError); | ||
|
|
||
| assert.throws(function() { | ||
| +Buffer.prototype; |
There was a problem hiding this comment.
I don't think this should fail, but rather coerce as usual with Object.prototype.toString.call(Buffer.prototype). Thoughts?
> '' + net.Socket.prototype
'[object Object]'
> '' + Buffer.prototype
TypeError: blah blah blahThere was a problem hiding this comment.
This operation calls the toString method on the object. Which is a custom implementation. It seems standard to throw in such cases.
There was a problem hiding this comment.
I disagree and think that regardless of how the operation works, we shouldn't throw an error on simple coercion.
There was a problem hiding this comment.
V8 throws the 'incompatible_method_receiver' TypeError for any custom defined toString. For example:
> Symbol.prototype.toString.call({})
TypeError: Method Symbol.prototype.toString called on incompatible receiver #<Object>
This type of behavior is defined by the ES spec: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-symbol.prototype.tostring
I think it would be smart for us to follow the same pattern.
There was a problem hiding this comment.
I wasn't aware of that before, thank you, failing with an error now makes sense to me.
There was a problem hiding this comment.
Neither was I before this PR. Previously I would have agreed with you on just coercing the call.
trevnorris
force-pushed
the
buffer-throw-not-abort
branch
from
cae5883
to
5bfea5a
Compare
| @@ -18,6 +18,12 @@ | |||
| if (!(r)) return env->ThrowRangeError("out of range index"); \ | |||
| } while (0) | |||
|
|
|||
| #define CHECK_IS_BUFFER(env, obj) \ | |||
There was a problem hiding this comment.
The name could be better, IMO. Maybe THROW_AND_RETURN_UNLESS_BUFFER(...)?
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.
trevnorris
force-pushed
the
buffer-throw-not-abort
branch
from
5bfea5a
to
2a4ec45
Compare
|
@bnoordhuis Suggestions complete. Had to make one change on CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/83/ |
| @@ -380,8 +380,6 @@ function slowToString(encoding, start, end) { | |||
|
|
|||
| Buffer.prototype.toString = function() { | |||
| const length = this.length | 0; | |||
There was a problem hiding this comment.
length is only used when arguments.length === 0 now.
|
The change to |
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>
|
Sorry about the Landed 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>
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.
Replaces: #1486
R=@bnoordhuis
CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/48/