Buffer.alloc(): A TypeError will be thrown if size is not a number.

[jorangreef]

Buffer.alloc(size) and friends (allocUnsafe(), allocUnsafeSlow()) promise that:

A TypeError will be thrown if size is not a number.

However, this contract is broken when size is NaN:

> Buffer.alloc(123+undefined)
<Buffer >

The reason is that assertSize(), which validates the size argument, does the check using typeof size !== 'number', however:

> typeof (123+undefined)
'number'

Number.isInteger() would be better:

> Number.isInteger(123+undefined)
false

Alternatively, a faster test for NaN would be swapping the range check around to throw for anything not in range (as opposed to throw for anything before 0 or after kMaxLength which is what it currently does):

if (!(size >= 0 && size <= kMaxLength)) {
  err = new ERR_INVALID_OPT_VALUE.RangeError('size', size);
}

The above snippet catches NaN.

[jorangreef]

The reason the NaN check is important, is that a user might do this:

ThirdPartyModule = { HEADERSIZE: 64 };

let size = 1000; // Pretend the user has validated this already.
Buffer.alloc(ThirdPartyModule.HEADER_SIZE + size);

And then pass this buffer to a native addon, assuming size has been validated and that HEADER_SIZE does not need to be validated since it's a third-party module constant.

Since the user miss-typed ThirdPartyModule.HEADERSIZE, it's undefined which when added to size gives NaN.

If the native addon then fails to check the buffer.length correctly, it's a buffer overflow waiting to happen.

Granted, this is alot of "ifs", but NaN is not an integer and the broken contract of Buffer.alloc() is dangerous.

[jorangreef]

As an aside, and just to highlight this again, the range check in assertSize doesn't actually check that an integer is within range, throwing if not:

size < 0 || size > kMaxLength

There are too many possible values of size which are not in range and yet which pass. The current range check is too weak. Are there other places in the Node api where checks like this need to be swapped around?

This actually checks the range:

!(size >= 0 && size <= kMaxLength)

[Fishrock123]

I think this is a bug.

[addaleax]

@Fishrock123 Does that mean you think #26162 shouldn’t be semver-major? I’d still say that it is…