From 7b41272d226e74a3d68c4af6431380ae5b4e1485 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 | 18 ++++++++++-------- src/win/fs.c | 31 +++++++++++++++++-------------- test/task.h | 36 ++++++++++++++++++++---------------- test/test-fs.c | 38 ++++++++++++++++++++++++++++++++++++-- test/test-list.h | 2 ++ 5 files changed, 85 insertions(+), 40 deletions(-) diff --git a/src/unix/fs.c b/src/unix/fs.c index 48e2bc88392..38eb275885b 100644 --- a/src/unix/fs.c +++ b/src/unix/fs.c @@ -212,14 +212,22 @@ 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; + 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; return tv; } @@ -227,9 +235,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 +1015,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..83c14ccd0fb 100644 --- a/src/win/fs.c +++ b/src/win/fs.c @@ -92,17 +92,17 @@ 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_INT(filetime) \ + ((filetime) - 116444736 * BILLION) #define FILETIME_TO_TIME_T(filetime) \ - (FILETIME_TO_UINT(filetime) / (10u * MILLIONu)) + (FILETIME_TO_INT(filetime) / (10 * MILLION)) #define FILETIME_TO_TIME_NS(filetime, secs) \ - ((FILETIME_TO_UINT(filetime) - (secs * (uint64_t) 10 * MILLIONu)) * 100U) + ((FILETIME_TO_INT(filetime) - (secs) * 10 * MILLION) * 100U) #define FILETIME_TO_TIMESPEC(ts, filetime) \ do { \ @@ -112,10 +112,9 @@ #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 +1790,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); + FILETIME_TO_TIMESPEC(statbuf->st_atim, + file_info.BasicInformation.LastAccessTime.QuadPart); + FILETIME_TO_TIMESPEC(statbuf->st_ctim, + file_info.BasicInformation.ChangeTime.QuadPart); + FILETIME_TO_TIMESPEC(statbuf->st_mtim, + file_info.BasicInformation.LastWriteTime.QuadPart); + 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..c33790bc8cd 100644 --- a/test/test-fs.c +++ b/test/test-fs.c @@ -821,8 +821,11 @@ static void check_utime(const char* path, ASSERT(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); + ASSERT_DOUBLE_EQ(s->st_atim.tv_sec + (s->st_atim.tv_nsec / 1000000000.0), + atime); + + ASSERT_DOUBLE_EQ(s->st_mtim.tv_sec + (s->st_mtim.tv_nsec / 1000000000.0), + mtime); uv_fs_req_cleanup(&req); } @@ -2561,6 +2564,37 @@ TEST_IMPL(fs_utime) { } +TEST_IMPL(fs_utime_round) { + const char path[] = "test_file"; + double atime; + double mtime; + uv_fs_t req; + int r; + +#ifdef __MVS__ + RETURN_SKIP("futime on z/os does not like pre-epoch timestamps"); +#endif + + 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.0; /* 1969-07-20T02:56:00.00Z */ + ASSERT_EQ(0, uv_fs_utime(NULL, &req, path, atime, mtime, NULL)); + 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; diff --git a/test/test-list.h b/test/test-list.h index f85754a05c0..91b3975243d 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) @@ -995,6 +996,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)