Repository navigation
buffer: inconsistencies in readIntBE() and readIntLE() #10515
Description
- 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
byteLengthdefault to1rather than0 - throw an error if
byteLengthis 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
Activity
Trott commentedon Dec 29, 2016
/cc @nodejs/buffer
cjihrig commentedon Dec 29, 2016
If a value is required, but not provided, we should throw.
sam-github commentedon Dec 29, 2016
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 commentedon Dec 29, 2016
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 commentedon Dec 29, 2016
@sam-github I might be misunderstanding you or expressing myself in an unclear way, but I agree that
offsetdefaulting to0makes sense and could be added to the documentation.The problematic part is the way
byteLengthalso defaults to0. That's not even a validbyteLengthvalue.sam-github commentedon Dec 29, 2016
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 commentedon Jan 11, 2017
Looks like this issue is open and known, just need someone to write the patch + test?
Trott commentedon Jul 15, 2017
#11146 seems really close. Head over there and help push it over the finish line?
buffer: check byteLength in readInt(B|L)E