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

src,fs: calculate times as unsigned long #13281

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented May 29, 2017

Fixes: #13255

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@nodejs-github-bot 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. labels May 29, 2017
assert.strictEqual(tests_ok, tests_run);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a newline at the end of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

@refack
Copy link
Contributor Author

refack commented May 29, 2017

@@ -164,7 +164,23 @@ runTest(new Date('1982-09-10 13:37'), new Date('1982-09-10 13:37'), function() {
});


if (common.isWindows) {
common.refreshTempDir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a call, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh yeah.

const path = `${common.tmpDir}/test-utimes-precision`;
fs.writeFileSync(path, '');

const truncate_mtime = 1713037251360;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would help with readability if you could add a comment explaining why these magic values are magic (I had to dig into the libuv sources, and I’m still not quite sure I got it right). ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tryied to hint with the name, but will add a comment.

src/node_file.cc Outdated
(s->st_##name.tv_nsec / 1e6); \
#define X(idx, name) \
fields[idx] = (static_cast<unsigned long>(s->st_##name.tv_sec) * 1e3) + \
(static_cast<unsigned long>(s->st_##name.tv_nsec) / 1e6); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a good idea if you could leave a comment here, explaining why this is needed.

And just so I get it right; this is needed because tv_sec overflows? I think the real problem in that case is that we don’t check for overflows in libuv during fs__utime_handle, which we probably should? Am I getting it totally wrong?

@refack
Copy link
Contributor Author

refack commented May 29, 2017

delint @ added comments
new "Dry Run" CI: https://ci.nodejs.org/job/node-test-commit/10223/

src/node_file.cc Outdated
fields[3] = s->st_uid;
fields[4] = s->st_gid;
fields[5] = s->st_rdev;
fields[0] = static_cast<double>(s->st_dev);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem unrelated, and I don’t think they are actually changing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They remove warnings, but I'll kick them out since I have a more "warning reducing" PR cooking

const truncate_stats = fs.statSync(path);
assert.strictEqual(truncate_mtime, truncate_stats.mtime.getTime());

// if this value is cast to `signed long` it gets converted to -2135622133469
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s not really a comment explaining what’s happening…

It’s not like (signed long) 2159345162531 == -2135622133469 – it’s more complicated than that, it definitely relates to how the Windows API deals with timestamps.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's almost that simple https://github.com/nodejs/node/blob/master/deps/uv/src/win/fs.c#L91
NTFS stores times in nanoseconds in a uint64_t but when uv converts it down to long we get 2's complement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Then maybe we should cast back to unsigned only on Windows?

But really, I’m not sure whether this is the right approach, or if we are better off throwing in utimes if the time is not representable as a long

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added to the comment why it's not a problem in POSIX, because there timestamps are stored in two fields struct {long t_sec, long t_usec}, and neither can overflow.
The problem is just how we convert from the NTFS uint64_t that hold nanoseconds, back to seconds.
IMHO This change is safe, since even the numbers from NTFS if properly converted to seconds fit in a long and in a double. (notice that the actual call to utimesSync divides these numbers by 1000). Also no FS will give us negative times, and the paper mentioned in the bug (interpretation-of-ntfs-timestamps), says we just need to interpret it right.

You might be right in that the calculations in https://github.com/nodejs/node/blob/master/deps/uv/src/win/fs.c#L91 should be done with uint

@refack refack force-pushed the more-for-12607 branch 2 times, most recently from 297a69c to 1e4c0c8 Compare May 29, 2017 17:49
@refack
Copy link
Contributor Author

refack commented May 29, 2017

more cleanup.
new "Dry Run" CI: https://ci.nodejs.org/job/node-test-commit/10226/

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! Looks good assuming we’re switching back to not doing 32-bit math.

src/node_file.cc Outdated
(s->st_##name.tv_nsec / 1e6); \
#define X(idx, name) \
fields[idx] = (static_cast<uint32_t>(s->st_##name.tv_sec) * 1e3) + \
(static_cast<uint32_t>(s->st_##name.tv_nsec) / 1e6); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Y2k38 bug? ;) I think doing the math in uint64_t (or double because it’s getting converted anyway) as you had it before was fine, is there any reason to switch?

Copy link
Contributor Author

@refack refack May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole fix turns Y2K38 to something like Y3K16
But there's a bug when doing static_cast<uint64_t> since the MSB is 1, it blows up to a huge number.
Also long (a.k.a. int32_t) to uint32_t is faster / stabler anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*static_cast<uint64_t>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole fix turns Y2K38 to something like Y3K16

Ah, right. (Still not great, but yes, it’s better.)

But there's a bug when doing static_cast<uint64_t> since the MSB is 1, it blows up to a huge number.

You have the overflow problem anyway, though? Also, this doesn’t happen if you cast to double directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since casting to double is a calculation is comes out a negative double.
Maybe a reinterpret_cast<uint64_t> or static_cast<uint64_t>(static_cast<uint32_t>()) first inner one to convince the compiler it's a positive number...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@refack The thing is, I’m not sure you can count on long being 32-bit on all those odd systems that Node supports. unsigned long is the right thing anyway if you are reading from a long.

Copy link
Contributor Author

@refack refack May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my initial thought but the linter was angry at me:
for
(static_cast<unsigned long>(s->st_##name.tv_sec) * 1e3)
it spet out
src/node_file.cc:483: Use int16/int64/etc, rather than the C type long [runtime/int] [4]
got the same for:
(unsigned long)(s->st_##name.tv_sec) * 1e3)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*angry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my initial thought

Good that we agree, though. :)

but the linter was angry at me:

// NOLINT(runtime/int) should help, although I’m not sure where exactly you need to place that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// This value if treaded as a `signed long` gets converted to -2135622133469.
// POSIX systems stores timestamps in {long t_sec, long t_usec}.
// NTFS stores times in nanoseconds in a single `uint64_t`, so when `uv`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/uv/libuv/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@refack
Copy link
Contributor Author

refack commented May 29, 2017

@refack
Copy link
Contributor Author

refack commented Jun 1, 2017

@sciolist This looks like the other half of #12607 now we're fixing Y2k38

@sciolist
Copy link
Contributor

sciolist commented Jun 1, 2017

fs dates on windows is a gift that keeps on giving.

Would it be bad to have the 2k38 test use 2159345163000 and let it run on non-windows as well?

@refack
Copy link
Contributor Author

refack commented Jun 1, 2017

Would it be bad to have the 2k38 test use 2159345163000 and let it run on non-windows as well?

good idea 👍 (even though there it's kept as seconds, not millis)

@refack
Copy link
Contributor Author

refack commented Jun 1, 2017

@sciolist
Copy link
Contributor

sciolist commented Jun 1, 2017

@refack yeah that's why i suggested increasing the number to a full second, but your method is nicer and a lot clearer what it's testing for. I was interested in seeing if all the other platforms would handle 2k38, seems like most of them did fine at least. :)

@refack
Copy link
Contributor Author

refack commented Jun 1, 2017

@refack
Copy link
Contributor Author

refack commented Jun 27, 2017

@refack
Copy link
Contributor Author

refack commented Jun 27, 2017

PR-URL: nodejs#13281
Fixes: nodejs#13255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@refack refack merged commit 0526f84 into nodejs:master Jun 27, 2017
addaleax pushed a commit that referenced this pull request Jun 29, 2017
PR-URL: #13281
Fixes: #13255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Jun 29, 2017
@refack refack deleted the more-for-12607 branch July 2, 2017 02:34
addaleax pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #13281
Fixes: #13255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13281
Fixes: #13255
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

Should this land in LTS?

Does it have any chance of changing behavior?

@refack
Copy link
Contributor Author

refack commented Aug 14, 2017

Does it have any chance of changing behavior?

Not No change in correct behaviour. It solves Y2K38 on linux POSIX, and enables higher precision times on Windows for use by #13173 (noop on current Stats otherwise)

@MylesBorins
Copy link
Contributor

This is not landing cleanly, ca you please backport with relevant commits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

utimes precision errors
7 participants