buffer: add a check for byteLength in readIntBE() and readIntLE()#11146
buffer: add a check for byteLength in readIntBE() and readIntLE()#11146seppevs wants to merge 3 commits intonodejs:masterfrom
Conversation
nodejs-github-bot
added
the
buffer
jasnell
added
the
semver-major
|
Added semver-major due to the addition of the new throw conditions. |
lib/buffer.js
Outdated
There was a problem hiding this comment.
Perhaps these can just be condensed into a single check
if (typeof byteLength !== 'number' || byteLength <= 0)
throw new TypeError('"byteLength" must be a positive number greater than zero');
There was a problem hiding this comment.
Thank you for your suggestion. I made the changes and amended them to the previous commit.
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
from
ca0c9be to
a8d1fa8
Compare
lib/buffer.js
Outdated
There was a problem hiding this comment.
byteLength <= 0 should be a RangeError I think.
There was a problem hiding this comment.
So should we throw 2 separate errors (TypeError when it's not a Number and RangeError when byteLength is <= 0) or just one RangeError?
There was a problem hiding this comment.
I think separate errors would be better, because a TypeError for byteLength <= 0 doesn't look right IMO.
According to the docs:
byteLength{Integer} How many bytes to read. Must satisfy:0 < byteLength <= 6
and
Supports up to 48 bits of accuracy.
So probably check byteLength > 6 too?
Also a doc update would be great :D
There was a problem hiding this comment.
good point about the RangeError. Yes, in that case keeping them separate is best.
There was a problem hiding this comment.
Thanks again for the feedback. I amended the proposed changes to the previous commit. I also updated the docs.
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
2 times, most recently
from
f3644da to
ed56860
Compare
|
Should a benchmark be run to ascertain the performance impact of this change? |
doc/api/buffer.md
Outdated
There was a problem hiding this comment.
Might as well adding TypeError: and RangeError: as the previous example, just to be a little bit more consistent.
|
@Trott +1 on running a benchmark. FYI: how to compare different implementations using the benchmark suite. The whole buffer benchmarks can take a long time IIRC, so probably running the benchmarks for these specific functions only (using |
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
from
ed56860 to
46ac4ad
Compare
|
@joyeecheung Thanks for advice on running the benchmark suite. I will look into it. |
|
I think the actual byte length range checking should go inside the
|
|
@mscdex so should both the RangeError and TypeError checks be moved inside the |
|
+1 on putting it in |
|
@seppevs I think putting both in there would be alright, plus it would allow us to avoid any potential performance regressions. |
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
from
46ac4ad to
25668ef
Compare
|
Code is updated. The checks are now inside an I also added a benchmark for the |
benchmark/buffers/buffer-read.js
Outdated
There was a problem hiding this comment.
The if branch should be moved outside new Function to avoid the runtime cost. Also would be easier to read if we use template literals here, something like:
var call;
if (fn === 'readIntLE' || conf.type === 'readIntBE') {
call = `buff.${fn}(0, 1, ${noAssert})`;
} else {
call = `buff.${fn}(0, ${noAssert})`;
}
var testFunction = new Function('buff',
` for (var i = 0; i !== ${len}; ++i) {
${call};
}`);
lib/buffer.js
Outdated
There was a problem hiding this comment.
Probably best moving checkByteLength and checkOffset in the same branch. Also not sure if the assertion should happen before or after offset and byteLength get converted into numbers, I am inclined to checking it before the conversion.
There was a problem hiding this comment.
The checkOffset uses the byteLength, and assumes it is defined and already converted to a number. The offset and byteLength are converted to numbers before the checkOffset function is invoked.
I think it's better to check the byteLength before it's converted to a number. If we do the conversion first, we can't trust the converted value in the checkByteLength function
console.log(undefined >>> 0); // -> 0
console.log(null >>> 0); // -> 0
console.log('a string' >>> 0); // -> 0
console.log(true >>> 0); // -> 1 .... this is in the expected range and would pass the check
So what do you suggest me to do?
There was a problem hiding this comment.
I agree that ideally we would combine the two, but there could easily be breakage if the current (prior to this PR) assertions are moved, so that would definitely be another semver-major change if we wanted to make that change at all. Thoughts @nodejs/collaborators?
There was a problem hiding this comment.
@seppevs No matter the order of the assertions, I the typechecking part of checkByteLength should be consistent with checkOffset. If checkOffset doesn't check the type, checkByteLength should not do it either. If we do want to check the type, then we need to add this to checkOffset too.
There was a problem hiding this comment.
Also if they are consistent, then they can be moved into the same branch I think? If we check the type, they can both be placed before the conversion. If we don't, they can both be placed after the conversion.
There was a problem hiding this comment.
So should I check the type, or not? Do you think the type check will affect the performance?
There was a problem hiding this comment.
@seppevs I can think of good arguments for both sides, so I think we can wait for thoughts from other collaborators.
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
from
25668ef to
d71fc82
Compare
|
What is the status on this one? @seppevs ... can you please rebase this? |
lib/buffer.js
Outdated
There was a problem hiding this comment.
You don't need the else here.
seppevs
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
2 times, most recently
from
8d4fbf6 to
10c78f4
Compare
|
@jasnell rebase done |
|
Ping @nodejs/buffer @nodejs/ctc ... thoughts on this? |
|
ping @nodejs/ctc |
Trott
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
3 times, most recently
from
64ff873 to
43e161e
Compare
@targos That would make sense. Will do... |
Oh, yes, probably! I'm intrigued, but I am inclined to save that for a subsequent PR. This PR is 11 months old at this point. I'd rather get the functionality landed and then have the performance vs. Crankshaft-script maintenance costs debate separately. |
|
Benchmark results with the additional changes: |
|
New benchmark results show an even smaller perf hit than before. 🎉 |
|
I imagine the |
|
Lone CI failure is a known flaky for which there is a fix pending (#17958). CI is good. |
|
Previous reviewers, a re-review is probably in order. Sorry about that. On the upside, I think this is probably good to go at this point. PTAL |
BridgeAR
added
the
author ready
The 'byteLength' argument should be required and of type 'number'. It should have a value between 1 and 6. This is a fix for issue: nodejs#10515
Split them into their own benhmark file and use different byteLength values.
Trott
force-pushed
the
buffer_add_a_check_for_byteLength_in_readIntBE_and_readIntLE
branch
from
4a5d043 to
710d1dc
Compare
The 'byteLength' argument should be required and of type 'number'. It should have a value between 1 and 6. PR-URL: nodejs#11146 Fixes: nodejs#10515 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Split them into their own benhmark file and use different byteLength values. PR-URL: nodejs#11146 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#11146 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
TimothyGu
removed
the
author ready
14 participants
The 'byteLength' argument should be required and of type 'number'. It should have a value between 1 and 6.
This is a fix for issue: #10515
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
buffer