Skip to content
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

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

Closed
erikkemperman opened this issue Oct 25, 2017 · 4 comments
Closed
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.

Comments

@erikkemperman
Copy link

  • Version: Node 6 and higher
  • Platform: Windows
  • Subsystem: fs

There seems to be a signed/unsigned bug in the fs module of Node 6 and higher on Windows.

Specifically, I see different values in fs.Stats.dev for the synchronous and asynchronous versions fs.stat() and fs.statSync() (for the same file, obviously).

This only happens on Windows (well, technically Appveyor, I am unable to try it locally because I don't have a Windows machine at hand) and only in Node 6 and up.

I've created a minimal repository demonstrating this bug, here.

The assertion that fails on Appveyor Node >= 6 but passes on Node < 6, is here:
https://github.com/erikkemperman/windows-stats-quirk/blob/master/test/quirk.js#L11

As you see in the logs, it got stat.dev == -726974396 from the async call, but it got stat.dev == 3567992900 from the sync call!

This happens to be what you get if you cast the former, in C, from int32_t to uint32_t, so I suspect something like that must be going wrong in Node/LibUV.

#include <stdint.h>
#include <stdio.h>
int main() {
  printf("%u\n", (uint32_t) -726974396); // 3567992900
}

The older Node versions, up to 5, are fine, and so are all of the versions on Travis.

Thanks for taking the time, hopefully this bug report will be somewhat helpful in improving an already pretty awesome platform!

@mscdex mscdex added fs Issues and PRs related to the fs subsystem / file system. v6.x windows Issues and PRs related to the Windows platform. labels Oct 25, 2017
@mscdex
Copy link
Contributor

mscdex commented Oct 25, 2017

Probably need something like #12614 for v6.x since it could be that an incomplete backport was done to v6.x at some point.

@erikkemperman
Copy link
Author

erikkemperman commented Oct 26, 2017

FYI it also happens with lstat and lstatSync, as well as fstat and fstatSync. Updated my repo to demonstrate this:

https://github.com/erikkemperman/windows-stats-quirk/blob/master/test/quirk.js#L23
https://github.com/erikkemperman/windows-stats-quirk/blob/master/test/quirk.js#L35

All of these fail on Appveyor but pass on Travis.

Also, I think it might be possible for something like this to happen for other fields than dev but those are probably just less likely to overflow like this.

JLHwung added a commit to JLHwung/node that referenced this issue Nov 1, 2017
Including:

* Move async *stat() functions to FillStatsArray() now used by the
sync *stat() functions

* Avoid creating fs.Stats instances for implicit async/sync *stat()
calls used in various fs functions

* Store reference to Float64Array data on C++ side for easier/faster
access, instead of passing from JS to C++ on every async/sync *stat()
call

Backport-PR-URL: nodejs#11665
Fixes: nodejs#16496
@JLHwung JLHwung mentioned this issue Nov 1, 2017
4 tasks
JLHwung added a commit to JLHwung/node that referenced this issue Nov 3, 2017
The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

Fixes: nodejs#16496
MylesBorins pushed a commit that referenced this issue Nov 26, 2017
The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

PR-URL: #16705
Fixes: #16496
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 28, 2017
The `dev_t` is unsigned on Linux, use Integer::NewFromUnsigned to fix cast overflow

PR-URL: #16705
Fixes: #16496
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@gireeshpunathil
Copy link
Member

link #12419

@TimothyGu
Copy link
Member

I believe this has been fixed by #16705.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

4 participants