buffer: inconsistencies in readIntBE() and readIntLE()

[Trott]

• Version: 7.3.0
• Platform: OS X 10.12.2
• Subsystem: buffer

The documentation for readIntLE() and readIntBE() both indicate that the second argument (byteLength) is required. However, both functions will seem to operate without a byteLength provided. Unfortunately, byteLength will default to 0 in that case, which results in unpredictable behavior:

> var buf = Buffer.from([126, 127, 128, 129, 130])
undefined
> buf.readIntLE(0)
126
> buf.readIntBE(0)
undefined
> buf.readIntBE(4)
128
> buf.readIntLE(4)
129
>

Perhaps we should do one of the following?:

• have byteLength default to 1 rather than 0
• throw an error if byteLength is not provided
• warn in the documentation that if you fail to provide byteLength, then you will get a result but there are no guarantees even about the return value type

[Trott]

/cc @nodejs/buffer

[cjihrig]

If a value is required, but not provided, we should throw.

[sam-github]

The behaviour of signed int reads should be consistent with the other read functions, what do the unsigned variants do? I just tried, and they seem to have an offset that defaults to zero, which is what would I would expect if I was to guess (but not what is documented).

[sam-github]

Also, can you check whether when used with offsets larger than the buffer size, they are allowing reads of arbitrary memory? That could be the source of what looks "unpredictable", and could have security implications.

[Trott]

@sam-github I might be misunderstanding you or expressing myself in an unclear way, but I agree that offset defaulting to 0 makes sense and could be added to the documentation.

The problematic part is the way byteLength also defaults to 0. That's not even a valid byteLength value.

[sam-github]

Agreed.

I wish for a test for byteLength of zero, with and without noAssert, and that shows that that the readInt variants behave the same with or without noAssert. Ideally, they should throw, but if they do something else right now, that's worth writing a test for.... so that when we fix it we can doc it, change the test, and mark it semver-major.

[trevnorris]

Looks like this issue is open and known, just need someone to write the patch + test?