-
Notifications
You must be signed in to change notification settings - Fork 28.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs: do basic arg validation of truncate in js #2498
Conversation
len = 0; | ||
} else if (typeof len !== 'number' || !isFinite(len)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Number.isFinite does both checks in one call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@targos Ah, you are correct. I updated now. PTAL.
Out of curiousity, what was the motivation behind these changes? |
b93d681
to
9910ba2
Compare
len = 0; | ||
} else if (!Number.isFinite(len) || len < 0) { | ||
throw new TypeError('length must be a positive integer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just seeing the error message. If you want to ensure it is an integer, you can drop Number.isFinite
and use Number.isInteger
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@targos Man, that's wonderful. I didn't know about that function and that is exactly what we need here. Thanks :-) Updated the PR.
@bnoordhuis Ping! |
@@ -688,14 +688,19 @@ fs.renameSync = function(oldPath, newPath) { | |||
}; | |||
|
|||
fs.truncate = function(path, len, callback) { | |||
if (typeof path === 'number') { | |||
if (Number.isInteger(path) && path >= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change in functionality. Here's an approximate test:
if (typeof path === 'number' && !Number.isInteger(path))
throw new TypeError('Not an integer');
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or are you just short-circuiting for the fast path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a legacy functionality it seems. If we pass an fd to truncate it internally delegates it to ftruncate. So we cannot throw error here. We are just checking if it is a positive integer and move on to the next check if is not.
Are you suggesting to not to call ftruncate at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just followed the logic paths for current and this patch, and there seemed to be a discrepancy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trevnorris Agree, but wouldn't this check be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My assumption was to land most these changes on a minor, then making any breaking changes, that push this to semver-major, to another PR. If you're cool with this staying on major then I'll have another look from that perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trevnorris I am okay with this being in major :-) Let's get this done right. Please suggest improvements as well :-)
Bump! |
!IsInt64(len_v->NumberValue())) { | ||
return env->ThrowTypeError("Not an integer"); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this and the below line to:
CHECK(args[1]->IsNumber());
const int64_t = args[1]->IntegerValue();
Reason for the CHECK()
is because we're now making an assumption about the incoming value. Always want to catch that if incorrect, because at this point if the value is incorrect then we've screwed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait. nm. read this as the fd value, not the len value.
EDIT: meaning, no need for the CHECK()
. can still collapse the other.
to recap
|
ping @thefourtheye |
ping @thefourtheye ;) |
@targos Ah, I have this open for a loooong time. I'll get to it this week. Thanks for reminding :-) Last week @benjamingr also reminded me of this. |
000eee8
to
8233f9a
Compare
@trevnorris Sorry for the delay in getting your review comments addressed. PTAL. |
Added semver-major tag based on the discussion thread. Feel free to change that if it turns out not to be major after all. |
function sanitizeFD(inputFD) { | ||
const fd = +inputFD; | ||
if (!Number.isInteger(fd) || fd < 0 || fd > 0xFFFFFFFF) { | ||
throw new Error('file descriptor must be a positive 32-bit integer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to a TypeError
. Also I'd replace "positive" with "unsigned".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
6036668
to
3aab026
Compare
@trevnorris Changed positive to unsigned now. |
3aab026
to
f0b3208
Compare
Included another commit now, which just doesn't coerce the fds. |
Thanks for the change, and test. LGTM. |
7a953f9
to
f19c403
Compare
Rebased and improved the |
This patch moves the basic validation of arguments to `truncate` family of functions to the JavaScript from the C++ layer.
f19c403
to
693f14e
Compare
Rebased, after landing #7168. CI Run: https://ci.nodejs.org/job/node-test-pull-request/3378/ |
Landed in c86c1ee. |
This patch 1. moves the basic validation of arguments to `truncate` family of functions to the JavaScript layer from the C++ layer. 2. makes sure that the File Descriptors are validated strictly. PR-URL: #2498 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This reverts commit c86c1ee. original commit message: This patch 1. moves the basic validation of arguments to `truncate` family of functions to the JavaScript layer from the C++ layer. 2. makes sure that the File Descriptors are validated strictly. PR-URL: nodejs#2498 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This reverts commit c86c1ee. original commit message: This patch 1. moves the basic validation of arguments to `truncate` family of functions to the JavaScript layer from the C++ layer. 2. makes sure that the File Descriptors are validated strictly. PR-URL: nodejs#2498 Reviewed-By: Trevor Norris <trev.norris@gmail.com> PR-URL: nodejs#7950 Reviewed-By: Julien Gilli <jgilli@nodejs.org> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch moves the basic validation of arguments to
truncate
familyof functions to the JavaScript from the C++ layer.
cc @bnoordhuis
CI Run: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/131/