Skip to content

Commit

Permalink
fs: fix utime/futime timestamp rounding errors
Browse files Browse the repository at this point in the history 
`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: nodejs/node#32369 (partially)
PR-URL: libuv#2747
Co-authored-by: Jameson Nash <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
@bnoordhuis @vtjnash
bnoordhuis and vtjnash committed Nov 10, 2020
1 parent 17b4156 commit 80893ed
Show file tree
Hide file tree
Showing 5 changed files with 111 additions and 56 deletions.
26 changes: 18 additions & 8 deletions src/unix/fs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,24 +212,37 @@ 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;
}

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);
Expand Down Expand Up @@ -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);
Expand Down
46 changes: 22 additions & 24 deletions src/win/fs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'/')
Expand Down Expand Up @@ -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;

Expand Down
36 changes: 20 additions & 16 deletions test/task.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
57 changes: 49 additions & 8 deletions test/test-fs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -821,8 +821,13 @@ 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_GE(s->st_atim.tv_nsec, 0);
ASSERT_DOUBLE_GE(s->st_mtim.tv_nsec, 0);
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);
}
Expand Down Expand Up @@ -2530,8 +2535,8 @@ TEST_IMPL(fs_utime) {
* 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 */
#if defined(_WIN32) || defined(__linux__)
mtime += 0.25; /* 1982-09-10 11:22:33.25 */
#endif

r = uv_fs_utime(NULL, &req, path, atime, mtime, NULL);
Expand Down Expand Up @@ -2561,6 +2566,42 @@ 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; /* 1969-07-20T02:56:00.00Z */

#if defined(_WIN32) || defined(__linux__)
mtime += 0.25; /* 1969-07-20T02:56:00.25Z */
#endif

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;
Expand Down Expand Up @@ -2621,8 +2662,8 @@ TEST_IMPL(fs_futime) {
* 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 */
#if defined(_WIN32) || defined(__linux__)
mtime += 0.25; /* 1982-09-10 11:22:33.25 */
#endif

r = uv_fs_open(NULL, &req, path, O_RDWR, 0, NULL);
Expand Down Expand Up @@ -2702,8 +2743,8 @@ TEST_IMPL(fs_lutime) {
/* 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 */
#if defined(_WIN32) || defined(__linux__)
mtime += 0.25; /* 1982-09-10 11:22:33.25 */
#endif

checkme.atime = atime;
Expand Down
2 changes: 2 additions & 0 deletions test/test-list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 80893ed

Please sign in to comment.