Skip to content

Buffer.allocUnsafe causes "Conditional jump..." on valgrind #26464

Description

@kotsss

Working with valgrind to clean a giant networking application I was getting "Conditional jump or move depends on uninitialised value(s)" with js stacks (see below), took a long time but managed to zero in on reason.

==28440== Conditional jump or move depends on uninitialised value(s)
==28440==    at 0xA7EBB08780A: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???
==28440==    by 0xA7EBB0118D4: ???

Running the following code through valgrind will trigger the error

const alloc = process.env.SAFE ? Buffer.alloc : Buffer.allocUnsafe;
var buf = alloc(20);
buf.writeInt32BE(123, 2);

>valgrind --gen-suppressions=yes --leak-check=full /var/lib/nave/global/10.13.0/bin/node a.js

I've managed to trace the cause to lib/internal/buffer.js:checkBounds accessing the buffer that isn't initialized.
Buffer.allocUnsafe used by ws npm module

Shachar

Activity

  1. added
    bufferIssues and PRs related to the buffer subsystem.
    on Mar 6, 2019
  2. bnoordhuis commented on Mar 7, 2019

    @bnoordhuis
    Member

    Thanks for the report. I'd say that's expected given that checkBounds() checks that buf[offset] === undefined, which has been observed to be a little faster than checking that offset < buf.length.

    V8's runtime loads the byte at buf[offset]. That's a random value if you use Buffer.allocUnsafe() because it doesn't zero the memory.

    I'm not sure if this is something we actually want to fix because there's no real bug.

    I suppose the strongest argument for addressing it is that it can't be suppressed because the comparison takes place in runtime-generated code - the address and even the exact layout of the code can change between invocations.

  3. kotsss commented on Mar 7, 2019

    @kotsss
    Author

    buf[offset] === undefined, which has been observed to be a little faster than checking that offset < buf.length.

    Is that really true? (I'm amazed that it amounts to something noticeable - I'm curious to test it myself)

    I'm not sure if this is something we actually want to fix because there's no real bug.

    I'm trying to understand why it's not a real bug, my best guess is that no matter what random memory value is read it won't cause Buffer [] operator to return undefined value as long as it's inside the array bounds. Am I getting this right?

    I suppose the strongest argument for addressing it is that it can't be suppressed because the comparison takes place in runtime-generated code - the address and even the exact layout of the code can change between invocations.

    IMHO, almost any fix to this will have, at least, the 'penalty' of changing to checking offset<buf.length.
    So if someone is already using allocUnsafe I can deduce 2 things about them: performance sensitive and know what they are doing (because working with uninitialized memory/buffer intentionally can lead to much worse bugs).
    So I would suggest not checking bounds if using allocUnsafe because as the name states it's 'unsafe' - you're on your own.

  4. reopened this on Mar 7, 2019
  5. BridgeAR commented on Mar 7, 2019

    @BridgeAR
    Member

    Is that really true? (I'm amazed that it amounts to something noticeable - I'm curious to test it myself)

    I was able to replace multiple error checks with buf[offset] === undefined and newer V8 versions have a nice OOB protection that works relatively fast. If buf[offset] is indeed undefined the original checks are executed to detect what exactly is wrong (e.g., wrong input value, out of bounds, float instead of integer...).

  6. bnoordhuis commented on Mar 7, 2019

    @bnoordhuis
    Member

    my best guess is that no matter what random memory value is read it won't cause Buffer [] operator to return undefined value as long as it's inside the array bounds. Am I getting this right?

    Yes, that's right.

  7. BridgeAR commented on Jun 11, 2019

    @BridgeAR
    Member

    I am going to close this since it does not really seem to be a bug.

    Please feel free to comment in case this should be reopened.

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

Metadata

Metadata

Assignees

No one assigned

    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