Skip to content

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

Closed
Closed
Unreachable code in buffer.js and unsigned bitwise shifts.#2668
Assignees
trevnorrisChALkeR
Labels
bufferIssues and PRs related to the buffer subsystem.

Description

@ChALkeR
ChALkeR
opened on 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

Activity

  1. trevnorris
    added
    bufferIssues and PRs related to the buffer subsystem.
    on Sep 2, 2015

  2. thefourtheye commented on Sep 3, 2015

    @thefourtheye
    thefourtheye
    on Sep 3, 2015
    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.

  3. trevnorris commented on Sep 3, 2015

    @trevnorris
    trevnorris
    on Sep 3, 2015
    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.

  4. trevnorris commented on Sep 3, 2015

    @trevnorris
    trevnorris
    on Sep 3, 2015
    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.

  5. trevnorris
    closed this as completedin ec83654on Dec 7, 2015
  6. rvagg
    added a commit that references this issue on Dec 8, 2015

    buffer: fix range checking for slowToString

    08a3f29

  7. ChALkeR commented on Dec 15, 2015

    @ChALkeR
    ChALkeR
    on Dec 15, 2015
    MemberAuthor

    This is fixed only partially.

  8. ChALkeR
    reopened this on Dec 15, 2015
  9. trevnorris
    self-assigned this
    on Jan 15, 2016
  10. scovetta
    added a commit that references this issue on Apr 2, 2016

    buffer: fix range checking for slowToString

    949af3d

  11. ChALkeR commented on Jun 2, 2016

    @ChALkeR
    ChALkeR
    on Jun 2, 2016
    MemberAuthor

    I will re-check this in a few days.

  12. ChALkeR
    self-assigned this
    on Jun 2, 2016

  13. Fishrock123 commented on Aug 3, 2016

    @Fishrock123
    Fishrock123
    on Aug 3, 2016
    Contributor

    @ChALkeR Any progress?

  14. ChALkeR commented on Aug 4, 2016

    @ChALkeR
    ChALkeR
    on Aug 4, 2016
    MemberAuthor

    @Fishrock123 This is still partially applicable.

    https://github.com/nodejs/node/blob/master/lib/buffer.js#L725-L726 — offset < 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.

  15. bnoordhuis commented on Feb 7, 2017

    @bnoordhuis
    bnoordhuis
    on Feb 7, 2017
    Member

    @ChALkeR Status?

  16. ChALkeR commented on Feb 8, 2017

    @ChALkeR
    ChALkeR
    on Feb 8, 2017
    MemberAuthor

    @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.

  17. seishun commented on Feb 8, 2017

    @seishun
    seishun
    on Feb 8, 2017
    Contributor

    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.

  18. Trott commented on Mar 23, 2017

    @Trott
    Trott
    on Mar 23, 2017
    Member

    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.

  19. Trott
    closed this as completedon Mar 23, 2017
  20. sjarmak
    added a commit that references this issue on Jul 27, 2026

    buffer: fix range checking for slowToString

    ec83654
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bufferIssues and PRs related to the buffer subsystem.

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions