-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: prevent abort on bad proto #2012
Conversation
}, TypeError); | ||
|
||
assert.throws(function() { | ||
+Buffer.prototype; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 blah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neither was I before this PR. Previously I would have agreed with you on just coercing the call.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name could be better, IMO. Maybe THROW_AND_RETURN_UNLESS_BUFFER(...)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like it.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/