From 8d5af5e70e7e894a4c93e752add75bac2df12ba8 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 24 Mar 2020 21:13:14 +0100 Subject: [PATCH] fs: fix utime/futime timestamp rounding errors `uv_fs_utime()` and `uv_fs_futime()` receive the timestamp as a `double` and then convert it to `struct timeval` or `struct timespec` where necessary but the calculation for the sub-second part exhibited rounding errors for dates in the deep past or the far-flung future, causing the timestamps to be off by sometimes over half a second on unix, or to be reinterpreted as unsigned and end up off by more than just sign but many also decades. Fixes: https://github.com/nodejs/node/issues/32369 (partially) PR-URL: https://github.com/libuv/libuv/pull/2747 Co-authored-by: Jameson Nash Reviewed-By: Colin Ihrig --- src/unix/fs.c | 26 ++++++++---- src/win/fs.c | 46 ++++++++++---------- test/task.h | 36 +++++++++------- test/test-fs.c | 108 ++++++++++++++++++++++++++++++++++------------- test/test-list.h | 2 + 5 files changed, 140 insertions(+), 78 deletions(-) diff --git a/src/unix/fs.c b/src/unix/fs.c index 48e2bc88392..57ad40ef26d 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -212,14 +212,30 @@ static ssize_t uv__fs_fdatasync(uv_fs_t* req) { UV_UNUSED(static struct timespec uv__fs_to_timespec(double time)) { struct timespec ts; ts.tv_sec = time; - ts.tv_nsec = (uint64_t)(time * 1000000) % 1000000 * 1000; + ts.tv_nsec = (time - ts.tv_sec) * 1e9; + + /* TODO(bnoordhuis) Remove this. utimesat() has nanosecond resolution but we + * stick to microsecond resolution for the sake of consistency with other + * platforms. I'm the original author of this compatibility hack but I'm + * less convinced it's useful nowadays. + */ + ts.tv_nsec -= ts.tv_nsec % 1000; + + if (ts.tv_nsec < 0) { + ts.tv_nsec += 1e9; + ts.tv_sec -= 1; + } return ts; } UV_UNUSED(static struct timeval uv__fs_to_timeval(double time)) { struct timeval tv; tv.tv_sec = time; - tv.tv_usec = (uint64_t)(time * 1000000) % 1000000; + tv.tv_usec = (time - tv.tv_sec) * 1e6; + if (tv.tv_usec < 0) { + tv.tv_usec += 1e6; + tv.tv_sec -= 1; + } return tv; } @@ -227,9 +243,6 @@ static ssize_t uv__fs_futime(uv_fs_t* req) { #if defined(__linux__) \ || defined(_AIX71) \ || defined(__HAIKU__) - /* utimesat() has nanosecond resolution but we stick to microseconds - * for the sake of consistency with other platforms. - */ struct timespec ts[2]; ts[0] = uv__fs_to_timespec(req->atime); ts[1] = uv__fs_to_timespec(req->mtime); @@ -1010,9 +1023,6 @@ static ssize_t uv__fs_utime(uv_fs_t* req) { || defined(_AIX71) \ || defined(__sun) \ || defined(__HAIKU__) - /* utimesat() has nanosecond resolution but we stick to microseconds - * for the sake of consistency with other platforms. - */ struct timespec ts[2]; ts[0] = uv__fs_to_timespec(req->atime); ts[1] = uv__fs_to_timespec(req->mtime); diff --git a/src/win/fs.c b/src/win/fs.c index 8a801749d47..3306b8dadd6 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -92,30 +92,24 @@ return; \ } -#define MILLIONu (1000U * 1000U) -#define BILLIONu (1000U * 1000U * 1000U) +#define MILLION ((int64_t) 1000 * 1000) +#define BILLION ((int64_t) 1000 * 1000 * 1000) -#define FILETIME_TO_UINT(filetime) \ - (*((uint64_t*) &(filetime)) - (uint64_t) 116444736 * BILLIONu) - -#define FILETIME_TO_TIME_T(filetime) \ - (FILETIME_TO_UINT(filetime) / (10u * MILLIONu)) - -#define FILETIME_TO_TIME_NS(filetime, secs) \ - ((FILETIME_TO_UINT(filetime) - (secs * (uint64_t) 10 * MILLIONu)) * 100U) - -#define FILETIME_TO_TIMESPEC(ts, filetime) \ - do { \ - (ts).tv_sec = (long) FILETIME_TO_TIME_T(filetime); \ - (ts).tv_nsec = (long) FILETIME_TO_TIME_NS(filetime, (ts).tv_sec); \ - } while(0) +static void uv__filetime_to_timespec(uv_timespec_t *ts, int64_t filetime) { + filetime -= 116444736 * BILLION; + ts->tv_sec = (long) (filetime / (10 * MILLION)); + ts->tv_nsec = (long) ((filetime - ts->tv_sec * 10 * MILLION) * 100U); + if (ts->tv_nsec < 0) { + ts->tv_sec -= 1; + ts->tv_nsec += 1e9; + } +} #define TIME_T_TO_FILETIME(time, filetime_ptr) \ do { \ - uint64_t bigtime = ((uint64_t) ((time) * (uint64_t) 10 * MILLIONu)) + \ - (uint64_t) 116444736 * BILLIONu; \ - (filetime_ptr)->dwLowDateTime = bigtime & 0xFFFFFFFF; \ - (filetime_ptr)->dwHighDateTime = bigtime >> 32; \ + int64_t bigtime = ((time) * 10 * MILLION + 116444736 * BILLION); \ + (filetime_ptr)->dwLowDateTime = (uint64_t) bigtime & 0xFFFFFFFF; \ + (filetime_ptr)->dwHighDateTime = (uint64_t) bigtime >> 32; \ } while(0) #define IS_SLASH(c) ((c) == L'\\' || (c) == L'/') @@ -1791,10 +1785,14 @@ INLINE static int fs__stat_handle(HANDLE handle, uv_stat_t* statbuf, statbuf->st_mode |= (_S_IREAD | _S_IWRITE) | ((_S_IREAD | _S_IWRITE) >> 3) | ((_S_IREAD | _S_IWRITE) >> 6); - FILETIME_TO_TIMESPEC(statbuf->st_atim, file_info.BasicInformation.LastAccessTime); - FILETIME_TO_TIMESPEC(statbuf->st_ctim, file_info.BasicInformation.ChangeTime); - FILETIME_TO_TIMESPEC(statbuf->st_mtim, file_info.BasicInformation.LastWriteTime); - FILETIME_TO_TIMESPEC(statbuf->st_birthtim, file_info.BasicInformation.CreationTime); + uv__filetime_to_timespec(&statbuf->st_atim, + file_info.BasicInformation.LastAccessTime.QuadPart); + uv__filetime_to_timespec(&statbuf->st_ctim, + file_info.BasicInformation.ChangeTime.QuadPart); + uv__filetime_to_timespec(&statbuf->st_mtim, + file_info.BasicInformation.LastWriteTime.QuadPart); + uv__filetime_to_timespec(&statbuf->st_birthtim, + file_info.BasicInformation.CreationTime.QuadPart); statbuf->st_ino = file_info.InternalInformation.IndexNumber.QuadPart; diff --git a/test/task.h b/test/task.h index 8250f949b2b..2145744b18f 100644 --- a/test/task.h +++ b/test/task.h @@ -196,22 +196,26 @@ typedef enum { } \ } while (0) -#define ASSERT_INT_BASE(a, operator, b, type, conv) \ - ASSERT_BASE(a, operator, b, type, conv) - -#define ASSERT_EQ(a, b) ASSERT_INT_BASE(a, ==, b, int64_t, PRId64) -#define ASSERT_GE(a, b) ASSERT_INT_BASE(a, >=, b, int64_t, PRId64) -#define ASSERT_GT(a, b) ASSERT_INT_BASE(a, >, b, int64_t, PRId64) -#define ASSERT_LE(a, b) ASSERT_INT_BASE(a, <=, b, int64_t, PRId64) -#define ASSERT_LT(a, b) ASSERT_INT_BASE(a, <, b, int64_t, PRId64) -#define ASSERT_NE(a, b) ASSERT_INT_BASE(a, !=, b, int64_t, PRId64) - -#define ASSERT_UINT64_EQ(a, b) ASSERT_INT_BASE(a, ==, b, uint64_t, PRIu64) -#define ASSERT_UINT64_GE(a, b) ASSERT_INT_BASE(a, >=, b, uint64_t, PRIu64) -#define ASSERT_UINT64_GT(a, b) ASSERT_INT_BASE(a, >, b, uint64_t, PRIu64) -#define ASSERT_UINT64_LE(a, b) ASSERT_INT_BASE(a, <=, b, uint64_t, PRIu64) -#define ASSERT_UINT64_LT(a, b) ASSERT_INT_BASE(a, <, b, uint64_t, PRIu64) -#define ASSERT_UINT64_NE(a, b) ASSERT_INT_BASE(a, !=, b, uint64_t, PRIu64) +#define ASSERT_EQ(a, b) ASSERT_BASE(a, ==, b, int64_t, PRId64) +#define ASSERT_GE(a, b) ASSERT_BASE(a, >=, b, int64_t, PRId64) +#define ASSERT_GT(a, b) ASSERT_BASE(a, >, b, int64_t, PRId64) +#define ASSERT_LE(a, b) ASSERT_BASE(a, <=, b, int64_t, PRId64) +#define ASSERT_LT(a, b) ASSERT_BASE(a, <, b, int64_t, PRId64) +#define ASSERT_NE(a, b) ASSERT_BASE(a, !=, b, int64_t, PRId64) + +#define ASSERT_UINT64_EQ(a, b) ASSERT_BASE(a, ==, b, uint64_t, PRIu64) +#define ASSERT_UINT64_GE(a, b) ASSERT_BASE(a, >=, b, uint64_t, PRIu64) +#define ASSERT_UINT64_GT(a, b) ASSERT_BASE(a, >, b, uint64_t, PRIu64) +#define ASSERT_UINT64_LE(a, b) ASSERT_BASE(a, <=, b, uint64_t, PRIu64) +#define ASSERT_UINT64_LT(a, b) ASSERT_BASE(a, <, b, uint64_t, PRIu64) +#define ASSERT_UINT64_NE(a, b) ASSERT_BASE(a, !=, b, uint64_t, PRIu64) + +#define ASSERT_DOUBLE_EQ(a, b) ASSERT_BASE(a, ==, b, double, "f") +#define ASSERT_DOUBLE_GE(a, b) ASSERT_BASE(a, >=, b, double, "f") +#define ASSERT_DOUBLE_GT(a, b) ASSERT_BASE(a, >, b, double, "f") +#define ASSERT_DOUBLE_LE(a, b) ASSERT_BASE(a, <=, b, double, "f") +#define ASSERT_DOUBLE_LT(a, b) ASSERT_BASE(a, <, b, double, "f") +#define ASSERT_DOUBLE_NE(a, b) ASSERT_BASE(a, !=, b, double, "f") #define ASSERT_STR_EQ(a, b) \ ASSERT_BASE_STR(strcmp(a, b) == 0, a, == , b, char*, "s") diff --git a/test/test-fs.c b/test/test-fs.c index 89d00f2a3a6..64b622d7262 100644 --- a/test/test-fs.c +++ b/test/test-fs.c @@ -816,13 +816,44 @@ static void check_utime(const char* path, else r = uv_fs_stat(loop, &req, path, NULL); - ASSERT(r == 0); + ASSERT_EQ(r, 0); - ASSERT(req.result == 0); + ASSERT_EQ(req.result, 0); s = &req.statbuf; - ASSERT(s->st_atim.tv_sec + (s->st_atim.tv_nsec / 1000000000.0) == atime); - ASSERT(s->st_mtim.tv_sec + (s->st_mtim.tv_nsec / 1000000000.0) == mtime); + if (s->st_atim.tv_nsec == 0 && s->st_mtim.tv_nsec == 0) { + /* + * Test sub-second timestamps only when supported (such as Windows with + * NTFS). Some other platforms support sub-second timestamps, but that + * support is filesystem-dependent. Notably OS X (HFS Plus) does NOT + * support sub-second timestamps. But kernels may round or truncate in + * either direction, so we may accept either possible answer. + */ +#ifdef _WIN32 + ASSERT_DOUBLE_EQ(atime, (long) atime); + ASSERT_DOUBLE_EQ(mtime, (long) atime); +#endif + if (atime > 0 || (long) atime == atime) + ASSERT_EQ(s->st_atim.tv_sec, (long) atime); + if (mtime > 0 || (long) mtime == mtime) + ASSERT_EQ(s->st_mtim.tv_sec, (long) mtime); + ASSERT_GE(s->st_atim.tv_sec, (long) atime - 1); + ASSERT_GE(s->st_mtim.tv_sec, (long) mtime - 1); + ASSERT_LE(s->st_atim.tv_sec, (long) atime); + ASSERT_LE(s->st_mtim.tv_sec, (long) mtime); + } else { + double st_atim; + double st_mtim; +#ifndef __APPLE__ + /* TODO(vtjnash): would it be better to normalize this? */ + ASSERT_DOUBLE_GE(s->st_atim.tv_nsec, 0); + ASSERT_DOUBLE_GE(s->st_mtim.tv_nsec, 0); +#endif + st_atim = s->st_atim.tv_sec + s->st_atim.tv_nsec / 1e9; + st_mtim = s->st_mtim.tv_sec + s->st_mtim.tv_nsec / 1e9; + ASSERT_DOUBLE_EQ(st_atim, atime); + ASSERT_DOUBLE_EQ(st_mtim, mtime); + } uv_fs_req_cleanup(&req); } @@ -2523,16 +2554,7 @@ TEST_IMPL(fs_utime) { uv_fs_req_cleanup(&req); uv_fs_close(loop, &req, r, NULL); - atime = mtime = 400497753; /* 1982-09-10 11:22:33 */ - - /* - * Test sub-second timestamps only on Windows (assuming NTFS). Some other - * platforms support sub-second timestamps, but that support is filesystem- - * dependent. Notably OS X (HFS Plus) does NOT support sub-second timestamps. - */ -#ifdef _WIN32 - mtime += 0.444; /* 1982-09-10 11:22:33.444 */ -#endif + atime = mtime = 400497753.25; /* 1982-09-10 11:22:33.25 */ r = uv_fs_utime(NULL, &req, path, atime, mtime, NULL); ASSERT(r == 0); @@ -2541,7 +2563,7 @@ TEST_IMPL(fs_utime) { check_utime(path, atime, mtime, /* test_lutime */ 0); - atime = mtime = 1291404900; /* 2010-12-03 20:35:00 - mees <3 */ + atime = mtime = 1291404900.25; /* 2010-12-03 20:35:00.25 - mees <3 */ checkme.path = path; checkme.atime = atime; checkme.mtime = mtime; @@ -2561,6 +2583,45 @@ TEST_IMPL(fs_utime) { } +TEST_IMPL(fs_utime_round) { + const char path[] = "test_file"; + double atime; + double mtime; + uv_fs_t req; + int r; + + loop = uv_default_loop(); + unlink(path); + r = uv_fs_open(NULL, &req, path, O_RDWR | O_CREAT, S_IWUSR | S_IRUSR, NULL); + ASSERT_GE(r, 0); + ASSERT_GE(req.result, 0); + uv_fs_req_cleanup(&req); + ASSERT_EQ(0, uv_fs_close(loop, &req, r, NULL)); + + atime = mtime = -14245440.25; /* 1969-07-20T02:56:00.25Z */ + + r = uv_fs_utime(NULL, &req, path, atime, mtime, NULL); +#if !defined(__linux__) && \ + !defined(_WIN32) && \ + !defined(__APPLE__) && \ + !defined(__FreeBSD__) && \ + !defined(__sun) + if (r != 0) { + ASSERT_EQ(r, UV_EINVAL); + RETURN_SKIP("utime on some OS (z/OS, IBM i PASE, AIX) or filesystems may reject pre-epoch timestamps"); + } +#endif + ASSERT_EQ(0, r); + ASSERT_EQ(0, req.result); + uv_fs_req_cleanup(&req); + check_utime(path, atime, mtime, /* test_lutime */ 0); + unlink(path); + + MAKE_VALGRIND_HAPPY(); + return 0; +} + + #ifdef _WIN32 TEST_IMPL(fs_stat_root) { int r; @@ -2614,16 +2675,7 @@ TEST_IMPL(fs_futime) { uv_fs_req_cleanup(&req); uv_fs_close(loop, &req, r, NULL); - atime = mtime = 400497753; /* 1982-09-10 11:22:33 */ - - /* - * Test sub-second timestamps only on Windows (assuming NTFS). Some other - * platforms support sub-second timestamps, but that support is filesystem- - * dependent. Notably OS X (HFS Plus) does NOT support sub-second timestamps. - */ -#ifdef _WIN32 - mtime += 0.444; /* 1982-09-10 11:22:33.444 */ -#endif + atime = mtime = 400497753.25; /* 1982-09-10 11:22:33.25 */ r = uv_fs_open(NULL, &req, path, O_RDWR, 0, NULL); ASSERT(r >= 0); @@ -2700,11 +2752,7 @@ TEST_IMPL(fs_lutime) { uv_fs_req_cleanup(&req); /* Test the synchronous version. */ - atime = mtime = 400497753; /* 1982-09-10 11:22:33 */ - -#ifdef _WIN32 - mtime += 0.444; /* 1982-09-10 11:22:33.444 */ -#endif + atime = mtime = 400497753.25; /* 1982-09-10 11:22:33.25 */ checkme.atime = atime; checkme.mtime = mtime; diff --git a/test/test-list.h b/test/test-list.h index d2de4009e33..2b139aace66 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -360,6 +360,7 @@ TEST_DECLARE (fs_open_flags) TEST_DECLARE (fs_fd_hash) #endif TEST_DECLARE (fs_utime) +TEST_DECLARE (fs_utime_round) TEST_DECLARE (fs_futime) TEST_DECLARE (fs_lutime) TEST_DECLARE (fs_file_open_append) @@ -996,6 +997,7 @@ TASK_LIST_START #endif TEST_ENTRY (fs_chown) TEST_ENTRY (fs_utime) + TEST_ENTRY (fs_utime_round) TEST_ENTRY (fs_futime) TEST_ENTRY (fs_lutime) TEST_ENTRY (fs_readlink)