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

Unreachable code in buffer.js and unsigned bitwise shifts. #2668

Closed
ChALkeR opened this issue Sep 2, 2015 · 11 comments
Closed

Unreachable code in buffer.js and unsigned bitwise shifts. #2668

ChALkeR opened this issue Sep 2, 2015 · 11 comments
Assignees
Labels
buffer Issues and PRs related to the buffer subsystem.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Sep 2, 2015

Thing to note here: A >>> B converts both arguments to unsigned int: http://www.ecma-international.org/ecma-262/6.0/#sec-unsigned-right-shift-operator.

  1. buffer.js#L314 is not reachable. If argument start is a number that is less than 0, then start >>> 0 makes it a (large) positive number. Fixed.
  2. buffer.js#L498 has offset < 0 check that is not reachable, because above it was either set to 0 or offset >>> 0 or length >>> 0.
  3. buffer.js#L498 has length < 0 check that is reachable only through XXX legacy write(string, encoding, offset, length) - remove in v0.13 case, because else it was set to either 0, undefined, or this.length (which should be probably not less than 0 I guess). On a side note: sould that legacy path be removed in 4.0 or not? Btw, in the «legacy write(» case there is no type checking or casting for length.

Also, on many other lines, offset = offset >>> 0 and a succeeding checkInt(this, value, offset is done.

Note that -30064771000 >>> 0 === 72, which makes -30064771000 a valid offset input, which could be a bit unexpected. Also, checkInt doesn't check for offset (or offset + ext) to be not less than zero. Such check would be unreachable now (because offset is always cast to unsigned), but it could be usable once this is fixed.

var x = new Buffer(10).fill(0);
x.writeUInt32BE(0xdeadbeef, -30064771070);
x

<Buffer 00 00 de ad be ef 00 00 00 00>

Btw, with these negative values the same happens also when >> or | (or any other cast-to-int) is used: -30064771070 >> 0 === 2, -30064771070 | 0 === 0. All checks should be probably done before int casting.

Maybe it's worth splitting this into two issues — unreachable code and casting before checks.

@trevnorris

@trevnorris trevnorris added the buffer Issues and PRs related to the buffer subsystem. label Sep 2, 2015
@thefourtheye
Copy link
Contributor

Glad that you brought this. I was mentioning something like this to trevnorris in a comment.

I say let's not do coercing and validate the values as they are and throw error if they are invalid. It would keep the code straightforward and easily maintainable.

@trevnorris
Copy link
Contributor

As I mentioned in the referenced issue, the conditional and check should be above the value coercion. In the case of .write*(). That is a bug, as it does not follow current documentation.

We shouldn't throw any more exceptions ATM so that the patch can land in v4. Otherwise it'll have to wait for v5. And there are legitimate bugs here that should be addressed which should propagate to v4. Any other changes should be referenced against documentation. Though I know in several cases where it's just not documented.

@trevnorris
Copy link
Contributor

Here's one potential solution for https://github.com/nodejs/node/blob/v3.3.0/lib/buffer.js#L314:
Replace the above start = start >>> 0; with:

if (start < 0) start = 0;
if (start > 0x7fffffff) start = 0x7fffffff;

The reason for this is because in src/node_internal.h the ParseArrayIndex() function is used to extract the values. This uses Int32Value(). Though ironically we store that into a size_t.

As you can see, this mess goes deeper. We should do a full assessment of the code and how all these cases are handled. I'm the one responsible for switching to use the >>> on incoming arguments for .write*() functions because by skipping argument checks it allowed many of these operations to be done in a couple nanoseconds. Which kicks the crap out of DataView. Is also very useful for node modules that work with image processing, video, etc. to have that kind of performance. Since doing that many writes on that scale can quickly kill performance.

Addressing throwing more often, any place we currently don't throw, we won't start for the time being. First we'll have to make an assessment of how much community breakage it will introduce. Its APIs are in massive use and possibly breaking them for anything short of a security issue would be difficult.

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Sep 22, 2015
If `start` is not a valid number in the range, then the default value
zero will be used. Same way, if `end` is not a valid number in the
accepted range, then, by default, the length of the buffer is assumed.

Ref: nodejs#2668

PR-URL: nodejs#2919
rvagg pushed a commit that referenced this issue Dec 8, 2015
If `start` is not a valid number in the range, then the default value
zero will be used. Same way, if `end` is not a valid number in the
accepted range, then, by default, the length of the buffer is assumed.

Fixes: #2668
Ref: #2919
PR-URL: #4019
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@ChALkeR
Copy link
Member Author

ChALkeR commented Dec 15, 2015

This is fixed only partially.

@ChALkeR ChALkeR reopened this Dec 15, 2015
@trevnorris trevnorris self-assigned this Jan 15, 2016
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
If `start` is not a valid number in the range, then the default value
zero will be used. Same way, if `end` is not a valid number in the
accepted range, then, by default, the length of the buffer is assumed.

Fixes: nodejs#2668
Ref: nodejs#2919
PR-URL: nodejs#4019
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@ChALkeR
Copy link
Member Author

ChALkeR commented Jun 2, 2016

I will re-check this in a few days.

@ChALkeR ChALkeR self-assigned this Jun 2, 2016
@Fishrock123
Copy link
Contributor

@ChALkeR Any progress?

@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 4, 2016

@Fishrock123 This is still partially applicable.

https://github.com/nodejs/node/blob/master/lib/buffer.js#L725-L726offset < 0 check is unreachable. We might want to keep it there as a safeguard in case if the surrounding code would change, though. Not sure.

@bnoordhuis
Copy link
Member

@ChALkeR Status?

@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 8, 2017

@bnoordhuis There are still some unreachable lines of code there, but those can be viewed as safeguards. I will take another look shortly to confirm that everything is fine.

Perhaps I will file a PR to add some comments there.

@seishun
Copy link
Contributor

seishun commented Feb 8, 2017

The offset check should either be removed, or a comment should be added that this is a safeguard. Otherwise it will confuse people reading the code and waste their time.

@Trott
Copy link
Member

Trott commented Mar 23, 2017

The offset < 0 is now reachable. Buffer.from('1234').write('100', 1) will not result in an error. Buffer.from('1234').write('100', -1) will result in an error. The difference in behavior is due to that check.

coverage.nodejs.org indicates that there is 100% code coverage for buffer.js, so assuming no issues with the coverage reports, all code in buffer.js is reachable now and all conditions are evaluated as both true and false in the course of tests..

Closing. Feel free to re-open if you think that's premature or incorrect.

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

7 participants