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 doesn't throw error for invalid parameter type #210

Closed
ghostoy opened this issue Dec 26, 2014 · 4 comments
Closed

Buffer doesn't throw error for invalid parameter type #210

ghostoy opened this issue Dec 26, 2014 · 4 comments

Comments

@ghostoy
Copy link

ghostoy commented Dec 26, 2014

In Node.js 0.10, invalid parameter type String will cause some Buffer APIs, like writeUint32LE('some string', 0), throwing an "value is out of bounds" error. The parameter is checked by checkInt function:

function checkInt(buffer, value, offset, ext, max, min) {
  if ((value % 1) !== 0 || value > max || value < min)
    throw TypeError('value is out of bounds');
  if ((offset % 1) !== 0 || offset < 0)
    throw TypeError('offset is not uint');
  if (offset + ext > buffer.length || buffer.length + offset < 0)
    throw RangeError('Trying to write outside buffer length');
}

However in Node.js / io.js 0.11 & 0.12, the check function is changed into following, which doesn't throw any errors for invalid values:

function checkInt(buffer, value, offset, ext, max, min) {
  if (!(buffer instanceof Buffer))
    throw new TypeError('buffer must be a Buffer instance');
  if (value > max || value < min)
    throw new TypeError('value is out of bounds');
  if (offset + ext > buffer.length)
    throw new RangeError('index out of range');
}

This is an inconsistent behavior, which should be fixed.

@bnoordhuis
Copy link
Member

I believe the motivation was to make it more aligned with the implicit type coercion that you get in most places; e.g. typed arrays: new Int32Array(['0x1badbabe'])

@trevnorris I believe you were responsible for that change? Can you comment?

ghostoy added a commit to ghostoy/node-ffi that referenced this issue Dec 29, 2014
* `NanCallback` throws errors to global, instead of current scope.
 So it can't be caught by `TryCatch` within `CallbackInfo::DispatchToV8`.
* `Buffer` in node.js 0.11 failed to detecting invalid parameters
 as 0.10 did nodejs/node#210
@trevnorris
Copy link
Contributor

Exactly what @bnoordhuis said.

@ghostoy
Copy link
Author

ghostoy commented Dec 31, 2014

In other word, this is a fix rather than a bug, right?

@chrisdickinson
Copy link
Contributor

Closing this as "working as intended." If I'm incorrect, please let me know and I can reopen.

jasongin added a commit to jasongin/nodejs that referenced this issue Mar 29, 2017
- Check that an internal field value is actually an External before casting.
   This prevents a crash if the wrong object is passed to napi_unwrap.
 - Wrap some macro parameters in parentheses.
boingoing pushed a commit to boingoing/node that referenced this issue Apr 6, 2017
- Check that an internal field value is actually an External before casting.
   This prevents a crash if the wrong object is passed to napi_unwrap.
 - Wrap some macro parameters in parentheses.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants