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 write* silently sets 0 instead of failing for wrong values #1161

Closed
unbornchikken opened this issue Mar 16, 2015 · 3 comments
Closed
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@unbornchikken
Copy link

Repro:

> var b = new Buffer(6);
undefined
> b.writeUInt32LE(10, 0);
4
> b.readUInt32LE(0)
10
> b.writeUInt32LE("5", 0);
4
> b.readUInt32LE(0)
5
> b.writeUInt32LE({}, 0);
4
> b.readUInt32LE(0)
0
> b.writeUInt32LE({}, 0, false);
4
> b.readUInt32LE(0)
0

According to this: https://iojs.org/api/buffer.html#buffer_buf_writeint32le_value_offset_noassert, the last three writes should be failed, because value is not a valid integer.

@mathiask88
Copy link
Contributor

I think io.js should just check if the argument is a number instead of casting everything to number(or NaN for {}) (https://github.com/iojs/io.js/blob/v1.x/lib/buffer.js#L863) ?

@Fishrock123 Fishrock123 added the buffer Issues and PRs related to the buffer subsystem. label Mar 17, 2015
@trevnorris trevnorris self-assigned this Mar 17, 2015
@trevnorris
Copy link
Contributor

Related: #2668

This is because we auto coerce the value like so:

Buffer.prototype.writeUInt32LE = function(value, offset, noAssert) {
  value = +value;

I did this to improve performance, but forgot about the passing of non-numeric values. Was only considering out of range numeric values.

So current functionality doesn't match documentation. The probable best solution is to put the assert checks above the value coercion. We'll work on this in the aforementioned issue.

@trevnorris
Copy link
Contributor

On second thought I'm inclined to say that documentation should be updated. Would better read like so:

Note, value will be range checked then coerced to a valid signed 32 bit integer.

Basically, it should follow the same coercion rules as DataVew.

jasnell added a commit to jasnell/node that referenced this issue Mar 27, 2016
Per nodejs#1161, when the
buf.write*() methods are given anything other than what
they expect, indicate that the behavior is unspecified.

Fixes: nodejs#1161
@jasnell jasnell closed this as completed in 64bf4b3 Apr 3, 2016
MylesBorins pushed a commit that referenced this issue Apr 5, 2016
Per #1161, when the
buf.write*() methods are given anything other than what
they expect, indicate that the behavior is unspecified.

Fixes: #1161
PR-URL: #5925
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
MylesBorins pushed a commit that referenced this issue Apr 11, 2016
Per #1161, when the
buf.write*() methods are given anything other than what
they expect, indicate that the behavior is unspecified.

Fixes: #1161
PR-URL: #5925
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants