fs: fix handling of `struct stat` fields by addaleax · Pull Request #8515 · nodejs/node

addaleax · 2016-09-13T11:53:56Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included (how? I’m open to suggestions.)
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

FChown and Chown test that the uid and gid parameters they receive are unsigned integers, but Stat() and FStat() would return the corresponding fields of struct stat as signed integers. Applications which pass those these values directly to Chown may fail
(e.g. for nobody on OS X, who has an uid of -2, see e.g. nodejs/node-v0.x-archive#5890).

This patch changes the Integer::New() call for uid and gid to Integer::NewFromUnsigned().

All other fields are kept as they are, for performance, but strictly speaking the respective sizes of those fields aren’t specified, either.

Ref: npm/npm#13918

/cc @nodejs/fs

CI: https://ci.nodejs.org/job/node-test-commit/5026/

bnoordhuis · 2016-09-13T11:58:57Z

Escaping an empty handle isn't necessary. I realize this is copied from existing code, though.

I’ll remove that in all locations here, then

bnoordhuis · 2016-09-13T12:00:15Z

LGTM. Technically, it's uv_stat_t, not struct stat.

`FChown` and `Chown` test that the `uid` and `gid` parameters they receive are unsigned integers, but `Stat()` and `FStat()` would return the corresponding fields of `uv_stat_t` as signed integers. Applications which pass those these values directly to `Chown` may fail (e.g. for `nobody` on OS X, who has an `uid` of `-2`, see e.g. nodejs/node-v0.x-archive#5890). This patch changes the `Integer::New()` call for `uid` and `gid` to `Integer::NewFromUnsigned()`. All other fields are kept as they are, for performance, but strictly speaking the respective sizes of those fields aren’t specified, either. Ref: npm/npm#13918

addaleax · 2016-09-13T12:04:04Z

Updated with uv_stat_t in the commit message and escaping the empty handles removed

bnoordhuis · 2016-09-13T12:07:16Z

LGTM

thefourtheye · 2016-09-13T17:56:28Z


+ // Integers.
+#define X(name) \
+ Local<Value> name = Integer::NewFromUnsigned(env->isolate(), s->st_##name); \


This must be Integer::New, right?

… yeah, sorry.

addaleax · 2016-09-13T18:02:31Z

@thefourtheye updated! :)

thefourtheye · 2016-09-13T18:13:48Z

I am not sure, if I am looking in the right place. But if I am following the definition here, correctly, even dev, mode, nlink, and rdev are also unsigned it seems.

addaleax · 2016-09-13T18:22:59Z

@thefourtheye Yeah, that doesn’t look like the right place… usually, you’d want to take a peek at the public headers which define struct stat, but as Ben noted, libuv ships its own type for that. Either way, the result might be system-dependent, POSIX doesn’t seem to specify anything about size or signedness of the fields.

That being said… yes, I’ve looked into it, and it seems the fields you mentioned are unsigned on Linux, too. Worse, dev, ino, nlink and rdev are 64 bit wide on my system, so using Integer::New(FromUnsigned) isn’t really appropriate… calling Number::New may be more correct, but it does seem to come with a slight but noticeable performance impact due to the extra range checking it does (about -1 % for me).

bnoordhuis · 2016-09-13T18:27:25Z

Worse, dev, ino, nlink and rdev are 64 bit wide on my system, so using Integer::New(FromUnsigned) isn’t really appropriate… calling Number::New may be more correct

Pragmatically though, those are going to fit in a uint32_t for the foreseeable future (maybe, just maybe, with the exception of ino.)

thefourtheye · 2016-09-13T18:43:42Z

So, we are going to leave them as signed for the time being?

addaleax · 2016-09-13T18:48:00Z

So, we are going to leave them as signed for the time being?

I’d be okay with that, if only for the fear of unnecessarily breaking things… alternatively, I can update this PR with something that auto-detects signedness of the fields?

thefourtheye · 2016-09-13T18:54:32Z

@addaleax Nah, we can keep it, simple, as it is. LGTM. I wonder how our CITGM never got this.

addaleax · 2016-09-13T18:57:46Z

I wonder how our CITGM never got this.

I think one of the reasons is that it leaves quite a lot of code intact, it just blows up when you try to pass the uid/git to chown or fchown… but that’s not something you can’t usually do without root. As long as I’m not missing anything, doing an actual, working chown is basically untested by Node. :/

jasnell

LGTM but I wonder if there's a regression test that could be added for this.

addaleax · 2016-09-15T19:42:41Z

@jasnell Maybe I could get a cctest for this together… I’ll look into that as soon as I have the time, but I kind of don’t want to block this PR on that because this PR itself would be a blocker for updating to npm@3.10.8 in deps/.

addaleax · 2016-09-18T09:38:36Z

CI: https://ci.nodejs.org/job/node-test-commit/5118/

addaleax · 2016-09-19T12:59:40Z

Landed in c5545f2

`FChown` and `Chown` test that the `uid` and `gid` parameters they receive are unsigned integers, but `Stat()` and `FStat()` would return the corresponding fields of `uv_stat_t` as signed integers. Applications which pass those these values directly to `Chown` may fail (e.g. for `nobody` on OS X, who has an `uid` of `-2`, see e.g. nodejs/node-v0.x-archive#5890). This patch changes the `Integer::New()` call for `uid` and `gid` to `Integer::NewFromUnsigned()`. All other fields are kept as they are, for performance, but strictly speaking the respective sizes of those fields aren’t specified, either. Ref: npm/npm#13918 PR-URL: #8515 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> undo accidental change to other fields of uv_fs_stat

At the time of writing, all currently published versions of Node.js return signed 32-bit integers in their return values for the `uid` and `gid` fields of `fs.Stats` instances. This is problematic, because some of Node’s other `fs` methods like `chown` expect unsigned 32-bit integer input and throw when encountering negative integers; this has broken e.g. `sudo npm install -g` on `OS X`, where `nobody` has a UID that would be returned as `-2` by `fs.stat()`. Ref: nodejs/node#8515 Ref: npm/npm#13918

`FChown` and `Chown` test that the `uid` and `gid` parameters they receive are unsigned integers, but `Stat()` and `FStat()` would return the corresponding fields of `uv_stat_t` as signed integers. Applications which pass those these values directly to `Chown` may fail (e.g. for `nobody` on OS X, who has an `uid` of `-2`, see e.g. nodejs/node-v0.x-archive#5890). This patch changes the `Integer::New()` call for `uid` and `gid` to `Integer::NewFromUnsigned()`. All other fields are kept as they are, for performance, but strictly speaking the respective sizes of those fields aren’t specified, either. Ref: npm/npm#13918 PR-URL: #8515 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> undo accidental change to other fields of uv_fs_stat

MylesBorins · 2016-10-24T16:47:07Z

@addaleax should this be backported?

addaleax · 2016-10-24T20:45:01Z

@thealphanerd This seems to land cleanly on v4.x-staging with tests passing, and it makes sense to have this on LTS for me.

`FChown` and `Chown` test that the `uid` and `gid` parameters they receive are unsigned integers, but `Stat()` and `FStat()` would return the corresponding fields of `uv_stat_t` as signed integers. Applications which pass those these values directly to `Chown` may fail (e.g. for `nobody` on OS X, who has an `uid` of `-2`, see e.g. nodejs/node-v0.x-archive#5890). This patch changes the `Integer::New()` call for `uid` and `gid` to `Integer::NewFromUnsigned()`. All other fields are kept as they are, for performance, but strictly speaking the respective sizes of those fields aren’t specified, either. Ref: npm/npm#13918 PR-URL: #8515 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> undo accidental change to other fields of uv_fs_stat

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 13, 2016

addaleax added fs Issues and PRs related to the fs subsystem / file system. lts-watch-v4.x labels Sep 13, 2016

addaleax mentioned this pull request Sep 13, 2016
uid must be an unsigned int npm/npm#13918
Closed

bnoordhuis reviewed Sep 13, 2016
View reviewed changes

addaleax force-pushed the stat-unsigned branch from d309b1b to 28ccbf1Compare September 13, 2016 12:03

thefourtheye reviewed Sep 13, 2016
View reviewed changes

[squash] undo accidental change to other fields of uv_fs_stat
7f3eeb4

jasnell approved these changes Sep 15, 2016
View reviewed changes

addaleax closed this Sep 19, 2016

addaleax deleted the stat-unsigned branch September 19, 2016 12:59

othiym23 mentioned this pull request Sep 19, 2016
TypeError: uid must be an unsigned int npm/npm#4080
Closed

addaleax mentioned this pull request Sep 23, 2016
Always make fs.Stats’ .uid and .gid fields unsigned isaacs/node-graceful-fs#87
Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Oct 24, 2016

MylesBorins mentioned this pull request Oct 26, 2016
V4.6.2 proposal #9298
Closed

lawliet89 mentioned this pull request Nov 11, 2016
yarn install fails inside a Docker Container with --userns-remap enabled yarnpkg/yarn#1750
Closed

JLHwung mentioned this pull request Nov 3, 2017
backport: 11665 to v6 #16667
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

addaleax commented Sep 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuisSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

bnoordhuis commented Sep 13, 2016

Uh oh!

thefourtheyeSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxSep 13, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

thefourtheye commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

bnoordhuis commented Sep 13, 2016

Uh oh!

thefourtheye commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

thefourtheye commented Sep 13, 2016

Uh oh!

addaleax commented Sep 13, 2016

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 15, 2016

Uh oh!

addaleax commented Sep 18, 2016

Uh oh!

addaleax commented Sep 19, 2016

Uh oh!

MylesBorins commented Oct 24, 2016

Uh oh!

addaleax commented Oct 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

addaleax commented Sep 13, 2016•
edited
Loading