fs: fix stat dev unsigned cast overflow #16705

JLHwung · 2017-11-03T05:16:51Z

The dev_t is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

Closes #16667 since this PR is a Semver-Patch alternatives regarding to #16496 only.

According to the linux headers, (http://elixir.free-electrons.com/linux/v4.13.11/source/include/linux/types.h#L15, http://elixir.free-electrons.com/linux/v4.13.11/source/include/uapi/asm-generic/posix_types.h#L23), both dev_t, mode_t, nlink_t are unsigned. Using Integer::NewFromUnsigned is more appropriate.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

fs

The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow Fixes: nodejs#16496

mscdex · 2017-11-03T05:22:58Z

FWIW all of the values should probably just use Number::New() since libuv has all of the uv_stat_t fields as uint64_t. Yes, double can only give us at most 53 bits, but that's more than 32. In fact, this is what the master branch (and v8.x) already does.

JLHwung · 2017-11-03T05:30:59Z

@mscdex Fixed, thank you.
As long as we won't come across to a huge fs with more than 53 bit dev ID before the ending of v6 LTS, I think it is sufficient for pragmatical usage.

mscdex · 2017-11-03T05:35:11Z

I'm not sure how other @nodejs/collaborators feel about the semver-ness of the following suggestion: I think maybe we should just use -1 for the blksize and blocks fields on non-POSIX platforms, which matches the FillStatsArray() behavior.

bnoordhuis · 2017-11-03T05:36:35Z

src/node_file.cc

 #define X(name)                                                               \
-  Local<Value> name = Integer::NewFromUnsigned(env->isolate(), s->st_##name); \
+  Local<Value> name = Number::New(env->isolate(),                             \
+                                  static_cast<double>(s->st_##name));         \


~~You removed the name.IsEmpty() check but it should most definitely stay.~~

(edit: looking at the wrong diff.)

cjihrig · 2017-11-03T14:11:12Z

src/node_file.cc

-  // Unsigned integers. It does not actually seem to be specified whether
-  // uid and gid are unsigned or not, but in practice they are unsigned,
-  // and Node’s (F)Chown functions do check their arguments for unsignedness.
+  // Numbers.


This comment looks pretty out of place now. Care to just remove it?

These comments are added on e9ce8fc to accompany different X macro definition. I don't think it is out of place and we are good to let it be.

MylesBorins · 2017-11-14T19:17:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11434/

The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow PR-URL: #16705 Fixes: #16496 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #16705 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

MylesBorins · 2017-11-26T01:39:37Z

landed in 659c458...8ade20f

The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow PR-URL: #16705 Fixes: #16496 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

PR-URL: #16705 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary. # Conflicts: # test/scripts/box/file.js

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary.

* Revert "hotfix(file): cast dev to uint32" This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary. * test(appveyor): update to exact node.js version

fs: fix stat dev unsigned cast overflow

c623ad0

The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow Fixes: nodejs#16496

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. v6.x labels Nov 3, 2017

JLHwung mentioned this pull request Nov 3, 2017

backport: 11665 to v6 #16667

Closed

4 tasks

fs: use Number::New since all fields are uint64_t

4a88989

bnoordhuis reviewed Nov 3, 2017

View reviewed changes

bnoordhuis approved these changes Nov 3, 2017

View reviewed changes

cjihrig approved these changes Nov 3, 2017

View reviewed changes

jasnell approved these changes Nov 6, 2017

View reviewed changes

XadillaX approved these changes Nov 16, 2017

View reviewed changes

benjamingr approved these changes Nov 16, 2017

View reviewed changes

MylesBorins closed this Nov 26, 2017

MylesBorins mentioned this pull request Nov 28, 2017

v6.12.1 proposal #17180

Merged

JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 6, 2017

Revert "hotfix(file): cast dev to uint32"

42e7c57

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary. # Conflicts: # test/scripts/box/file.js

JLHwung mentioned this pull request Dec 6, 2017

Revert "hotfix(file): cast dev to uint32" hexojs/hexo#2889

Merged

2 tasks

JLHwung deleted the fix-16496 branch December 6, 2017 14:56

JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 7, 2017

Revert "hotfix(file): cast dev to uint32"

62df5a7

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary.

JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 8, 2017

Revert "hotfix(file): cast dev to uint32"

0b66c2e

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary.

JLHwung added a commit to hexojs/hexo that referenced this pull request Dec 10, 2017

Revert "hotfix(file): cast dev to uint32"

487c702

This reverts commit a218422. Since nodejs/node#16705 is merged, the hotfix is unecessary.

TimothyGu mentioned this pull request May 21, 2018

Suspect signed/unsigned bug in the fs module of Node 6 and up on Windows #16496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: fix stat dev unsigned cast overflow #16705

fs: fix stat dev unsigned cast overflow #16705

JLHwung commented Nov 3, 2017 •

edited

Loading

mscdex commented Nov 3, 2017 •

edited

Loading

JLHwung commented Nov 3, 2017 •

edited

Loading

mscdex commented Nov 3, 2017 •

edited

Loading

bnoordhuis Nov 3, 2017 •

edited

Loading

cjihrig Nov 3, 2017

JLHwung Nov 6, 2017

MylesBorins commented Nov 14, 2017

MylesBorins commented Nov 26, 2017

fs: fix stat dev unsigned cast overflow #16705

fs: fix stat dev unsigned cast overflow #16705

Conversation

JLHwung commented Nov 3, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mscdex commented Nov 3, 2017 • edited Loading

JLHwung commented Nov 3, 2017 • edited Loading

mscdex commented Nov 3, 2017 • edited Loading

bnoordhuis Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

cjihrig Nov 3, 2017

Choose a reason for hiding this comment

JLHwung Nov 6, 2017

Choose a reason for hiding this comment

MylesBorins commented Nov 14, 2017

MylesBorins commented Nov 26, 2017

JLHwung commented Nov 3, 2017 •

edited

Loading

mscdex commented Nov 3, 2017 •

edited

Loading

JLHwung commented Nov 3, 2017 •

edited

Loading

mscdex commented Nov 3, 2017 •

edited

Loading

bnoordhuis Nov 3, 2017 •

edited

Loading