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

[kotsss]

• Version: v10.13.0
• Platform: Linux maint-ubuntu 3.13.0-123-generic test: split simple into io/simple, run in parallel #172-Ubuntu SMP Mon Jun 26 18:04:35 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: Buffer

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

[bnoordhuis]

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.

[kotsss]

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.

[BridgeAR]

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

[bnoordhuis]

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.

[BridgeAR]

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.