[PATCH 0/2] use gettimeofday for current time

[PATCH 0/2] use gettimeofday for current time

[Paul Eggert]

In recent GNU/Linux versions on rare occasions the 'time' function
returns a value that is one second less than the seconds component of
the timestamp that would be returned by gettimeofday, clock_gettime
with CLOCK_REALTIME, 'stat' on a recently modified file, etc.

If a program calls both 'time' and any of these other functions, the
system clock will appear to jump backwards by a second temporarily.
This can lead to confusing or inconsistent output.

A simple workaround for Git is to use 'gettimeofday' instead of
'time'.  Although other options are possible (see the commit message
of patch 0002), simplest is probably best.

Paul Eggert (2):
  git-compat-util: time_now for current time
  git-compat-util: use gettimeofday for current time

 archive.c                          | 2 +-
 blame.c                            | 2 +-
 builtin/bugreport.c                | 2 +-
 builtin/credential-cache--daemon.c | 4 ++--
 builtin/diagnose.c                 | 2 +-
 builtin/fast-import.c              | 2 +-
 builtin/fsmonitor--daemon.c        | 4 ++--
 builtin/gc.c                       | 2 +-
 builtin/log.c                      | 2 +-
 builtin/receive-pack.c             | 2 +-
 builtin/reflog.c                   | 2 +-
 commit-graph.c                     | 4 ++--
 compat/nedmalloc/malloc.c.h        | 2 +-
 credential.c                       | 4 ++--
 date.c                             | 6 +++---
 git-compat-util.h                  | 8 ++++++++
 http-backend.c                     | 2 +-
 http-push.c                        | 6 +++---
 rerere.c                           | 2 +-
 run-command.c                      | 4 ++--
 t/helper/test-chmtime.c            | 4 ++--
 t/helper/test-simple-ipc.c         | 4 ++--
 22 files changed, 40 insertions(+), 32 deletions(-)

--
2.39.2

[PATCH 1/2] git-compat-util: time_now for current time

[Paul Eggert]

Add a function time_now to get the current time as a time_t value.
All uses of time(NULL) or time(&xxx) changed to use this new function.
This refactoring does not change behavior.

Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
---
 archive.c                          | 2 +-
 blame.c                            | 2 +-
 builtin/bugreport.c                | 2 +-
 builtin/credential-cache--daemon.c | 4 ++--
 builtin/diagnose.c                 | 2 +-
 builtin/fast-import.c              | 2 +-
 builtin/fsmonitor--daemon.c        | 4 ++--
 builtin/gc.c                       | 2 +-
 builtin/log.c                      | 2 +-
 builtin/receive-pack.c             | 2 +-
 builtin/reflog.c                   | 2 +-
 commit-graph.c                     | 4 ++--
 compat/nedmalloc/malloc.c.h        | 2 +-
 credential.c                       | 4 ++--
 date.c                             | 6 +++---
 git-compat-util.h                  | 5 +++++
 http-backend.c                     | 2 +-
 http-push.c                        | 6 +++---
 rerere.c                           | 2 +-
 run-command.c                      | 4 ++--
 t/helper/test-chmtime.c            | 4 ++--
 t/helper/test-simple-ipc.c         | 4 ++--
 22 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/archive.c b/archive.c
index 1c2ca78e52..5cdafc9b56 100644
--- a/archive.c
+++ b/archive.c
@@ -472,7 +472,7 @@ static void parse_treeish_arg(const char **argv,
 		archive_time = commit->date;
 	} else {
 		commit_oid = NULL;
-		archive_time = time(NULL);
+		archive_time = time_now();
 	}
 	if (ar_args->mtime_option)
 		archive_time = approxidate(ar_args->mtime_option);
diff --git a/blame.c b/blame.c
index e45d8a3bf9..df14c1dfe4 100644
--- a/blame.c
+++ b/blame.c
@@ -192,7 +192,7 @@ static struct commit *fake_working_tree_commit(struct repository *r,
 	struct strbuf msg = STRBUF_INIT;
 
 	repo_read_index(r);
-	time(&now);
+	now = time_now();
 	commit = alloc_commit_node(r);
 	commit->object.parsed = 1;
 	commit->date = now;
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 5bc254be80..bba5820787 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -98,7 +98,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	struct strbuf buffer = STRBUF_INIT;
 	struct strbuf report_path = STRBUF_INIT;
 	int report = -1;
-	time_t now = time(NULL);
+	time_t now = time_now();
 	struct tm tm;
 	enum diagnose_mode diagnose = DIAGNOSE_NONE;
 	char *option_output = NULL;
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 6e509d02c3..7a87df16d2 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -27,7 +27,7 @@ static void cache_credential(struct credential *c, int timeout)
 	/* take ownership of pointers */
 	memcpy(&e->item, c, sizeof(*c));
 	memset(c, 0, sizeof(*c));
-	e->expiration = time(NULL) + timeout;
+	e->expiration = time_now() + timeout;
 }
 
 static struct credential_cache_entry *lookup_credential(const struct credential *c)
@@ -54,7 +54,7 @@ static timestamp_t check_expirations(void)
 {
 	static timestamp_t wait_for_entry_until;
 	int i = 0;
-	timestamp_t now = time(NULL);
+	timestamp_t now = time_now();
 	timestamp_t next = TIME_MAX;
 
 	/*
diff --git a/builtin/diagnose.c b/builtin/diagnose.c
index d52015c67a..c502096e6c 100644
--- a/builtin/diagnose.c
+++ b/builtin/diagnose.c
@@ -11,7 +11,7 @@ static const char * const diagnose_usage[] = {
 int cmd_diagnose(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf zip_path = STRBUF_INIT;
-	time_t now = time(NULL);
+	time_t now = time_now();
 	struct tm tm;
 	enum diagnose_mode mode = DIAGNOSE_STATS;
 	char *option_output = NULL;
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index f7548ff4a3..9096f52075 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -335,7 +335,7 @@ static void write_crash_report(const char *err)
 	fprintf(rpt, "fast-import crash report:\n");
 	fprintf(rpt, "    fast-import process: %"PRIuMAX"\n", (uintmax_t) getpid());
 	fprintf(rpt, "    parent process     : %"PRIuMAX"\n", (uintmax_t) getppid());
-	fprintf(rpt, "    at %s\n", show_date(time(NULL), 0, DATE_MODE(ISO8601)));
+	fprintf(rpt, "    at %s\n", show_date(time_now(), 0, DATE_MODE(ISO8601)));
 	fputc('\n', rpt);
 
 	fputs("fatal: ", rpt);
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index cae804a190..45cc8134cc 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -415,7 +415,7 @@ static struct fsmonitor_token_data *fsmonitor_new_token_data(void)
 	 * events to accumulate.
 	 */
 	if (test_env_value)
-		batch->pinned_time = time(NULL);
+		batch->pinned_time = time_now();
 
 	return token;
 }
@@ -772,7 +772,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
 	 */
 	token_data = state->current_token_data;
 	batch_head = token_data->batch_head;
-	((struct fsmonitor_batch *)batch_head)->pinned_time = time(NULL);
+	((struct fsmonitor_batch *)batch_head)->pinned_time = time_now();
 
 	/*
 	 * FSMonitor Protocol V2 requires that we send a response header
diff --git a/builtin/gc.c b/builtin/gc.c
index c58fe8c936..029bd2ba5c 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -450,7 +450,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)
 			 * after the user verifies that no gc is
 			 * running.
 			 */
-			time(NULL) - st.st_mtime <= 12 * 3600 &&
+			time_now() - st.st_mtime <= 12 * 3600 &&
 			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
 			/* be gentle to concurrent "gc" on remote hosts */
 			(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
diff --git a/builtin/log.c b/builtin/log.c
index b62e629ba8..bdc62d9b9e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1168,7 +1168,7 @@ static void gen_message_id(struct rev_info *info, char *base)
 {
 	struct strbuf buf = STRBUF_INIT;
 	strbuf_addf(&buf, "%s.%"PRItime".git.%s", base,
-		    (timestamp_t) time(NULL),
+		    (timestamp_t) time_now(),
 		    git_committer_info(IDENT_NO_NAME|IDENT_NO_DATE|IDENT_STRICT));
 	info->message_id = strbuf_detach(&buf, NULL);
 }
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fe68c79ee3..3fedeea65c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -2504,7 +2504,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
 
 	git_config(receive_pack_config, NULL);
 	if (cert_nonce_seed)
-		push_cert_nonce = prepare_push_cert_nonce(service_dir, time(NULL));
+		push_cert_nonce = prepare_push_cert_nonce(service_dir, time_now());
 
 	if (0 <= transfer_unpack_limit)
 		unpack_limit = transfer_unpack_limit;
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 270681dcdf..013673e768 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -237,7 +237,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 {
 	struct cmd_reflog_expire_cb cmd = { 0 };
-	timestamp_t now = time(NULL);
+	timestamp_t now = time_now();
 	int i, status, do_all, all_worktrees = 1;
 	unsigned int flags = 0;
 	int verbose = 0;
diff --git a/commit-graph.c b/commit-graph.c
index 5e6098ff35..4c75d96fc4 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2196,7 +2196,7 @@ static void merge_commit_graphs(struct write_commit_graph_context *ctx)
 static void mark_commit_graphs(struct write_commit_graph_context *ctx)
 {
 	uint32_t i;
-	time_t now = time(NULL);
+	time_t now = time_now();
 
 	for (i = ctx->num_commit_graphs_after - 1; i < ctx->num_commit_graphs_before; i++) {
 		struct stat st;
@@ -2217,7 +2217,7 @@ static void expire_commit_graphs(struct write_commit_graph_context *ctx)
 	DIR *dir;
 	struct dirent *de;
 	size_t dirnamelen;
-	timestamp_t expire_time = time(NULL);
+	timestamp_t expire_time = time_now();
 
 	if (ctx->opts && ctx->opts->expire_time)
 		expire_time = ctx->opts->expire_time;
diff --git a/compat/nedmalloc/malloc.c.h b/compat/nedmalloc/malloc.c.h
index 814845d4b3..4c30a49e60 100644
--- a/compat/nedmalloc/malloc.c.h
+++ b/compat/nedmalloc/malloc.c.h
@@ -3085,7 +3085,7 @@ static int init_mparams(void) {
 #ifdef WIN32
 	magic = (size_t)(GetTickCount() ^ (size_t)0x55555555U);
 #else
-      magic = (size_t)(time(0) ^ (size_t)0x55555555U);
+      magic = (size_t)(time_now() ^ (size_t)0x55555555U);
 #endif
       magic |= (size_t)8U;    /* ensure nonzero */
       magic &= ~(size_t)7U;   /* improve chances of fault for bad values */
diff --git a/credential.c b/credential.c
index ea40a2a410..dc750fa502 100644
--- a/credential.c
+++ b/credential.c
@@ -356,7 +356,7 @@ void credential_fill(struct credential *c)
 
 	for (i = 0; i < c->helpers.nr; i++) {
 		credential_do(c, c->helpers.items[i].string, "get");
-		if (c->password_expiry_utc < time(NULL)) {
+		if (c->password_expiry_utc < time_now()) {
 			/* Discard expired password */
 			FREE_AND_NULL(c->password);
 			/* Reset expiry to maintain consistency */
@@ -380,7 +380,7 @@ void credential_approve(struct credential *c)
 
 	if (c->approved)
 		return;
-	if (!c->username || !c->password || c->password_expiry_utc < time(NULL))
+	if (!c->username || !c->password || c->password_expiry_utc < time_now())
 		return;
 
 	credential_apply_config(c);
diff --git a/date.c b/date.c
index 6f45eeb356..16717be397 100644
--- a/date.c
+++ b/date.c
@@ -597,7 +597,7 @@ static int match_multi_number(timestamp_t num, char c, const char *date,
 	case '/':
 	case '.':
 		if (!now)
-			now = time(NULL);
+			now = time_now();
 		refuse_future = NULL;
 		if (gmtime_r(&now, &now_tm))
 			refuse_future = &now_tm;
@@ -712,7 +712,7 @@ static int match_digit(const char *date, struct tm *tm, int *offset, int *tm_gmt
 		unsigned int num2 = (num % 10000) / 100;
 		unsigned int num3 = num % 100;
 		if (n == 8)
-			set_date(num1, num2, num3, NULL, time(NULL), tm);
+			set_date(num1, num2, num3, NULL, time_now(), tm);
 		else if (n == 6 && set_time(num1, num2, num3, tm) == 0 &&
 			 *end == '.' && isdigit(end[1]))
 			strtoul(end + 1, &end, 10);
@@ -1041,7 +1041,7 @@ void datestamp(struct strbuf *out)
 	int offset;
 	struct tm tm = { 0 };
 
-	time(&now);
+	now = time_now();
 
 	offset = tm_to_time_t(localtime_r(&now, &tm)) - now;
 	offset /= 60;
diff --git a/git-compat-util.h b/git-compat-util.h
index 1e6592624d..d05f4bac22 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -339,6 +339,11 @@ static inline const char *precompose_string_if_needed(const char *in)
 int compat_mkdir_wo_trailing_slash(const char*, mode_t);
 #endif
 
+static inline time_t time_now(void)
+{
+	return time(NULL);
+}
+
 #ifdef NO_STRUCT_ITIMERVAL
 struct itimerval {
 	struct timeval it_interval;
diff --git a/http-backend.c b/http-backend.c
index 9cfc6f2541..d533989e32 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -113,7 +113,7 @@ static void hdr_nocache(struct strbuf *hdr)
 
 static void hdr_cache_forever(struct strbuf *hdr)
 {
-	timestamp_t now = time(NULL);
+	timestamp_t now = time_now();
 	hdr_date(hdr, "Date", now);
 	hdr_date(hdr, "Expires", now + 31536000);
 	hdr_str(hdr, "Cache-Control", "public, max-age=31536000");
diff --git a/http-push.c b/http-push.c
index 88aa045ecb..d8158667a0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -459,7 +459,7 @@ static int refresh_lock(struct remote_lock *lock)
 			fprintf(stderr, "LOCK HTTP error %ld\n",
 				results.http_code);
 		} else {
-			lock->start_time = time(NULL);
+			lock->start_time = time_now();
 			rc = 1;
 		}
 	}
@@ -473,7 +473,7 @@ static int refresh_lock(struct remote_lock *lock)
 static void check_locks(void)
 {
 	struct remote_lock *lock = repo->locks;
-	time_t current_time = time(NULL);
+	time_t current_time = time_now();
 	int time_remaining;
 
 	while (lock) {
@@ -933,7 +933,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		FREE_AND_NULL(lock);
 	} else {
 		lock->url = url;
-		lock->start_time = time(NULL);
+		lock->start_time = time_now();
 		lock->next = repo->locks;
 		repo->locks = lock;
 	}
diff --git a/rerere.c b/rerere.c
index a67abaab07..ac36a9c234 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1179,7 +1179,7 @@ void rerere_gc(struct repository *r, struct string_list *rr)
 	DIR *dir;
 	struct dirent *e;
 	int i;
-	timestamp_t now = time(NULL);
+	timestamp_t now = time_now();
 	timestamp_t cutoff_noresolve = now - 15 * 86400;
 	timestamp_t cutoff_resolve = now - 60 * 86400;
 
diff --git a/run-command.c b/run-command.c
index ba617655b2..fbf9afdb27 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1864,7 +1864,7 @@ enum start_bg_result start_bg_command(struct child_process *cmd,
 		goto done;
 	}
 
-	time(&time_limit);
+	time_limit = time_now();
 	time_limit += timeout_sec;
 
 wait:
@@ -1891,7 +1891,7 @@ enum start_bg_result start_bg_command(struct child_process *cmd,
 			 */
 			time_t now;
 
-			time(&now);
+			now = time_now();
 			if (now < time_limit)
 				goto wait;
 
diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index dc28890a18..f48a07f6f7 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c
@@ -7,7 +7,7 @@
  *
  *	test-tool chmtime =<seconds> file...
  *
- * Relative to the current time as returned by time(3):
+ * Relative to the current time as returned by time_now():
  *
  *	test-tool chmtime =+<seconds> (or =-<seconds>) file...
  *
@@ -60,7 +60,7 @@ static int timespec_arg(const char *arg, long int *set_time, int *set_eq)
 		return 0;
 	}
 	if ((*set_eq && *set_time < 0) || *set_eq == 2) {
-		time_t now = time(NULL);
+		time_t now = time_now();
 		*set_time += now;
 	}
 	return 1;
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index 3d1436da59..2267cd9fac 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -411,7 +411,7 @@ static int client__stop_server(void)
 	time_t time_limit, now;
 	enum ipc_active_state s;
 
-	time(&time_limit);
+	time_limit = time_now();
 	time_limit += cl_args.max_wait_sec;
 
 	cl_args.token = "quit";
@@ -434,7 +434,7 @@ static int client__stop_server(void)
 			return 0;
 		}
 
-		time(&now);
+		now = time_now();
 		if (now > time_limit)
 			return error("daemon has not shutdown yet");
 	}
-- 
2.39.2

[PATCH 2/2] git-compat-util: use gettimeofday for current time

[Paul Eggert]

Use gettimeofday instead of time(NULL) to get current time.
This avoids clock skew on glibc 2.31+ on Linux, where in the
first 1 to 2.5 ms of every second, time(NULL) returns a
value that is one less than the tv_sec part of
higher-resolution timestamps such as those returned by
gettimeofday or timespec_get, or those in the file system.
There are similar clock skew problems on AIX and MS-Windows,
which have problems in the first 5 ms of every second.

Without this patch, users can observe Git issuing a
timestamp T+1 before it issues timestamp T, because Git
sometimes uses time(NULL) or time(&t) and sometimes uses
higher-res methods like gettimeofday.  Although strictly
speaking users should tolerate this behavuior because a
superuser can always change the clock back, this is a
quality of implementation issue and users naturally expect
Git to issue timestamps in increasing order unless the
superuser has fiddled with the system clock.

This patch always uses gettimeofday(...) instead of time(...),
and I have verified that the resulting .o files never refer
to the name 'time'.  A trickier patch would change only
those calls for which timestamp monotonicity is user-visible.
Such a patch would require more expertise about Git internals,
though, and would be harder to maintain later.

Another possibility would be to change Git's documentation
to warn users that Git does not always issue timestamps in
increasing order.  However, Git users would likely be either
dismayed by this possibility, or confused by the level of
detail that any such documentation would require.

Yet another possibility would be to fix the Linux kernel so
that the time syscall is consistent with the other timestamp
syscalls.  I suppose this has not been done due to
performance implications.  (Git's use of timestamps is rare
enough that performance is not a significant consideration
for git.)  However, this wouldn't fix Git's problem on older
Linux kernels, or on AIX or MS-Windows.

Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
---
 git-compat-util.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index d05f4bac22..dcd9e5ca65 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -341,7 +341,10 @@ int compat_mkdir_wo_trailing_slash(const char*, mode_t);
 
 static inline time_t time_now(void)
 {
-	return time(NULL);
+	/* Avoid time(NULL), which can disagree with gettimeofday etc.  */
+	struct timeval tv;
+	gettimeofday(&tv, NULL);
+	return tv.tv_sec;
 }
 
 #ifdef NO_STRUCT_ITIMERVAL
-- 
2.39.2

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Eric Wong]

Paul Eggert <eggert@cs.ucla.edu> wrote:
> Use gettimeofday instead of time(NULL) to get current time.
> This avoids clock skew on glibc 2.31+ on Linux, where in the
> first 1 to 2.5 ms of every second, time(NULL) returns a
> value that is one less than the tv_sec part of
> higher-resolution timestamps such as those returned by
> gettimeofday or timespec_get, or those in the file system.
> There are similar clock skew problems on AIX and MS-Windows,
> which have problems in the first 5 ms of every second.

Wow, this is enlightening... and unfortunate :<

I decided to check glibc archives to find more discussion on it.
So far, I've found:

  https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/

and the original bug:
  https://sourceware.org/bugzilla/show_bug.cgi?id=30200

And this is due to the time64 changes in glibc 2.31+?
(<= 2.30 isn't affected?)

<snip>

> Yet another possibility would be to fix the Linux kernel so
> that the time syscall is consistent with the other timestamp
> syscalls.  I suppose this has not been done due to
> performance implications.  (Git's use of timestamps is rare
> enough that performance is not a significant consideration
> for git.)  However, this wouldn't fix Git's problem on older
> Linux kernels, or on AIX or MS-Windows.

Agreed on the older kernels and other OSes part.

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> Paul Eggert <eggert@cs.ucla.edu> wrote:
>> Use gettimeofday instead of time(NULL) to get current time.
>> This avoids clock skew on glibc 2.31+ on Linux, where in the
>> first 1 to 2.5 ms of every second, time(NULL) returns a
>> value that is one less than the tv_sec part of
>> higher-resolution timestamps such as those returned by
>> gettimeofday or timespec_get, or those in the file system.
>> There are similar clock skew problems on AIX and MS-Windows,
>> which have problems in the first 5 ms of every second.
>
> Wow, this is enlightening... and unfortunate :<
>
> I decided to check glibc archives to find more discussion on it.
> So far, I've found:
>
>   https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/
>
> and the original bug:
>   https://sourceware.org/bugzilla/show_bug.cgi?id=30200
>
> And this is due to the time64 changes in glibc 2.31+?
> (<= 2.30 isn't affected?)
>
> <snip>
>
>> Yet another possibility would be to fix the Linux kernel so
>> that the time syscall is consistent with the other timestamp
>> syscalls.  I suppose this has not been done due to
>> performance implications.  (Git's use of timestamps is rare
>> enough that performance is not a significant consideration
>> for git.)  However, this wouldn't fix Git's problem on older
>> Linux kernels, or on AIX or MS-Windows.
>
> Agreed on the older kernels and other OSes part.

Yeah, this is interesting.  I however wonder if we should follow our
usual pattern of implementing git_time() with the identical function
signature as what we replace (i.e. system's time()), and #undef/#define
the symbol we replace with git_time, though.  Wouldn't it make [1/2]
a lot smaller and future-proof?

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Eric Wong <e@80x24.org> writes:
> ...
>> I decided to check glibc archives to find more discussion on it.
>> So far, I've found:
>>
>>   https://inbox.sourceware.org/libc-alpha/20230306160321.2942372-1-adhemerval.zanella@linaro.org/T/
>>
>> and the original bug:
>>   https://sourceware.org/bugzilla/show_bug.cgi?id=30200
>>
>> And this is due to the time64 changes in glibc 2.31+?
>> (<= 2.30 isn't affected?)
>>
> ...
>
> Yeah, this is interesting.  I however wonder if we should follow our
> usual pattern of implementing git_time() with the identical function
> signature as what we replace (i.e. system's time()), and #undef/#define
> the symbol we replace with git_time, though.  Wouldn't it make [1/2]
> a lot smaller and future-proof?

And here is how such an approach may look like.  The second part is
more or less the same as Paul's version so I kept his authorship
there (these are mostly for discussion and not for application).

----- >8 ----- [PATCH 1/2] ----- >8 -----
Subject: [PATCH] time(): allow OVERRIDE_TIME to redefine time(2)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile          | 11 +++++++++++
 compat/time.c     | 11 +++++++++++
 config.mak.uname  |  1 +
 git-compat-util.h |  8 ++++++++
 4 files changed, 31 insertions(+)
 create mode 100644 compat/time.c

diff --git a/Makefile b/Makefile
index 50ee51fde3..aa6fcd6e04 100644
--- a/Makefile
+++ b/Makefile
@@ -286,6 +286,12 @@ include shared.mak
 # crashes due to allocation and free working on different 'heaps'.
 # It's defined automatically if USE_NED_ALLOCATOR is set.
 #
+# Define OVERRIDE_TIME to override time(2) and replace it with an
+# implementation based on gettimeofday(2).  THis is necessary when
+# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of
+# every second, time(NULL) returns a value that is one less than the
+# tv_sec part of higher-resolution timestamps used in the file system.
+#
 # Define NO_REGEX if your C library lacks regex support with REG_STARTEND
 # feature.
 #
@@ -2061,6 +2067,11 @@ ifdef OVERRIDE_STRDUP
 	COMPAT_OBJS += compat/strdup.o
 endif
 
+ifdef OVERRIDE_TIME
+	COMPAT_CFLAGS += -DOVERRIDE_TIME
+	COMPAT_OBJS += compat/time.o
+endif
+
 ifdef GIT_TEST_CMP_USE_COPIED_CONTEXT
 	export GIT_TEST_CMP_USE_COPIED_CONTEXT
 endif
diff --git a/compat/time.c b/compat/time.c
new file mode 100644
index 0000000000..a3ef5c0e98
--- /dev/null
+++ b/compat/time.c
@@ -0,0 +1,11 @@
+#include "../git-compat-util.h"
+
+#undef time
+time_t git_time(time_t *tloc)
+{
+	time_t t = time(NULL);
+
+	if (tloc)
+		*tloc = t;
+	return t;
+}
diff --git a/config.mak.uname b/config.mak.uname
index 64c44db805..29398919e8 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -64,6 +64,7 @@ ifeq ($(uname_S),Linux)
 	PROCFS_EXECUTABLE_PATH = /proc/self/exe
 	HAVE_PLATFORM_PROCINFO = YesPlease
 	COMPAT_OBJS += compat/linux/procinfo.o
+	OVERRIDE_TIME = YesPlease
 	# centos7/rhel7 provides gcc 4.8.5 and zlib 1.2.7.
 	ifneq ($(findstring .el7.,$(uname_R)),)
 		BASIC_CFLAGS += -std=c99
diff --git a/git-compat-util.h b/git-compat-util.h
index 1e6592624d..2279f3c90c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -917,6 +917,14 @@ void *gitmemmem(const void *haystack, size_t haystacklen,
 char *gitstrdup(const char *s);
 #endif
 
+#ifdef OVERRIDE_TIME
+#ifdef time
+#undef time
+#endif
+#define time git_time
+extern time_t git_time(time_t *);
+#endif
+
 #ifdef NO_GETPAGESIZE
 #define getpagesize() sysconf(_SC_PAGESIZE)
 #endif
-- 
2.40.0-115-ge25cabbf6b


----- >8 ----- [PATCH 2/2] ----- >8 -----
From: Paul Eggert <eggert@cs.ucla.edu>
From: Paul Eggert <eggert@cs.ucla.edu>
Subject: [PATCH] time(): use gettimeofday()

Use gettimeofday instead of time(NULL) to get current time.
This avoids clock skew on glibc 2.31+ on Linux, where in the
first 1 to 2.5 ms of every second, time(NULL) returns a
value that is one less than the tv_sec part of
higher-resolution timestamps such as those returned by
gettimeofday or timespec_get, or those in the file system.
There are similar clock skew problems on AIX and MS-Windows,
which have problems in the first 5 ms of every second.

Without this patch, users can observe Git issuing a
timestamp T+1 before it issues timestamp T, because Git
sometimes uses time(NULL) or time(&t) and sometimes uses
higher-res methods like gettimeofday.  Although strictly
speaking users should tolerate this behavuior because a
superuser can always change the clock back, this is a
quality of implementation issue and users naturally expect
Git to issue timestamps in increasing order unless the
superuser has fiddled with the system clock.

This patch always uses gettimeofday(...) instead of time(...),
and I have verified that the resulting .o files never refer
to the name 'time'.  A trickier patch would change only
those calls for which timestamp monotonicity is user-visible.
Such a patch would require more expertise about Git internals,
though, and would be harder to maintain later.

Another possibility would be to change Git's documentation
to warn users that Git does not always issue timestamps in
increasing order.  However, Git users would likely be either
dismayed by this possibility, or confused by the level of
detail that any such documentation would require.

Yet another possibility would be to fix the Linux kernel so
that the time syscall is consistent with the other timestamp
syscalls.  I suppose this has not been done due to
performance implications.  (Git's use of timestamps is rare
enough that performance is not a significant consideration
for git.)  However, this wouldn't fix Git's problem on older
Linux kernels, or on AIX or MS-Windows.

Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 compat/time.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/compat/time.c b/compat/time.c
index a3ef5c0e98..758a160ee7 100644
--- a/compat/time.c
+++ b/compat/time.c
@@ -3,9 +3,15 @@
 #undef time
 time_t git_time(time_t *tloc)
 {
-	time_t t = time(NULL);
+	/*
+	 * Avoid time(NULL), which can disagree with gettimeofday()
+	 * and filesystem timestamps.
+	 */
+	struct timeval tv;
+
+	gettimeofday(&tv, NULL);
 
 	if (tloc)
-		*tloc = t;
-	return t;
+		*tloc = tv.tv_sec;
+	return tv.tv_sec;
 }
-- 
2.40.0-115-ge25cabbf6b

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Paul Eggert]

On 3/20/23 10:01, Junio C Hamano wrote:

>> I however wonder if we should follow our
>> usual pattern of implementing git_time() with the identical function
>> signature as what we replace (i.e. system's time()), and #undef/#define
>> the symbol we replace with git_time, though.  Wouldn't it make [1/2]
>> a lot smaller and future-proof?

Yes, something like that would work too. (Sorry, I didn't know about the 
usual pattern.)


> +# Define OVERRIDE_TIME to override time(2) and replace it with an
> +# implementation based on gettimeofday(2).  THis is necessary when
> +# glibc 2.31+ on Linux is used, where in the first 1 to 2.5 ms of
> +# every second, time(NULL) returns a value that is one less than the
> +# tv_sec part of higher-resolution timestamps used in the file system.

THis -> This

It might be simpler to use the gettimeofday workaround on all platforms, 
rather than having an OVERRIDE_TIME flag and complicating 
config.mak.uname. gettimeofday should be portable, as it's already used 
elsewhere in Git without configury.

If we're going with the more-complicated solution, config.mak.uname will 
need changes in its AIX and MS-Windows sections because the problem is 
known to occur there too. Although AIX configuration is simple, I'm not 
sure how to go about the MS-Windows part as there seem to be a lot of 
ifdefs there. Also, because the time problem does not occur with musl 
libc (used in Alpine Linux 3.17), on the Linux side the workaround could 
be employed only if glibc 2.31+ is in use.

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Junio C Hamano]

Paul Eggert <eggert@cs.ucla.edu> writes:

> It might be simpler to use the gettimeofday workaround on all
> platforms, rather than having an OVERRIDE_TIME flag and complicating
> config.mak.uname. gettimeofday should be portable, as it's already
> used elsewhere in Git without configury.

That is an excellent point.

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Taylor Blau]

On Mon, Mar 20, 2023 at 09:33:01AM -0700, Junio C Hamano wrote:
> Yeah, this is interesting.

Definitely so. You learn something new every day ;-).

> I however wonder if we should follow our usual pattern of implementing
> git_time() with the identical function signature as what we replace
> (i.e. system's time()), and #undef/#define the symbol we replace with
> git_time, though.  Wouldn't it make [1/2] a lot smaller and
> future-proof?

Yeah, I agree here, too. It was my first thought when I started reading
Paul's patches here. I think that your approach is sound and I would be
happy to see you queue it.

Thanks,
Taylor

Re: [PATCH 2/2] git-compat-util: use gettimeofday for current time

[Taylor Blau]

On Mon, Mar 20, 2023 at 12:40:07PM -0700, Junio C Hamano wrote:
> Paul Eggert <eggert@cs.ucla.edu> writes:
>
> > It might be simpler to use the gettimeofday workaround on all
> > platforms, rather than having an OVERRIDE_TIME flag and complicating
> > config.mak.uname. gettimeofday should be portable, as it's already
> > used elsewhere in Git without configury.
>
> That is an excellent point.

I'd be happy to assume OVERRIDE_TIME is set everywhere and just drop it
entirely (using gettimeofday() unconditionally within git_time()).

An alternative approach might be to leave OVERRIDE_TIME time in place,
but treat it as opt-out instead of opt-in. I can imagine that some
exotic platform might want to use time() instead of gettimeofday() for
one reason or another.

Thanks,
Taylor

[PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Junio C Hamano]

From: Paul Eggert <eggert@cs.ucla.edu>

Use gettimeofday instead of time(NULL) to get current time.
This avoids clock skew on glibc 2.31+ on Linux, where in the
first 1 to 2.5 ms of every second, time(NULL) returns a
value that is one less than the tv_sec part of
higher-resolution timestamps such as those returned by
gettimeofday or timespec_get, or those in the file system.
There are similar clock skew problems on AIX and MS-Windows,
which have problems in the first 5 ms of every second.

Without this patch, users can observe Git issuing a
timestamp T+1 before it issues timestamp T, because Git
sometimes uses time(NULL) or time(&t) and sometimes uses
higher-res methods like gettimeofday.  Although strictly
speaking users should tolerate this behavuior because a
superuser can always change the clock back, this is a
quality of implementation issue and users naturally expect
Git to issue timestamps in increasing order unless the
superuser has fiddled with the system clock.

This patch always uses gettimeofday(...) instead of time(...),
and I have verified that the resulting .o files never refer
to the name 'time'.  A trickier patch would change only
those calls for which timestamp monotonicity is user-visible.
Such a patch would require more expertise about Git internals,
though, and would be harder to maintain later.

Another possibility would be to change Git's documentation
to warn users that Git does not always issue timestamps in
increasing order.  However, Git users would likely be either
dismayed by this possibility, or confused by the level of
detail that any such documentation would require.

Yet another possibility would be to fix the Linux kernel so
that the time syscall is consistent with the other timestamp
syscalls.  I suppose this has not been done due to
performance implications.  (Git's use of timestamps is rare
enough that performance is not a significant consideration
for git.)  However, this wouldn't fix Git's problem on older
Linux kernels, or on AIX or MS-Windows.

Signed-off-by: Paul Eggert <eggert@cs.ucla.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This one is closer to v1 than my "how about" illustration, in
   that we use gettimeofday() everywhere, so I kept the authorship
   ident and timestamp from the original.

 git-compat-util.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 4f0028ce60..d6fbe9311d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -339,6 +339,25 @@ static inline const char *precompose_string_if_needed(const char *in)
 int compat_mkdir_wo_trailing_slash(const char*, mode_t);
 #endif
 
+#ifdef time
+#undef time
+#endif
+static inline time_t git_time(time_t *tloc)
+{
+	struct timeval tv;
+
+	/*
+	 * Avoid time(NULL), which can disagree with gettimeofday(2)
+	 * and filesystem timestamps.
+	 */
+	gettimeofday(&tv, NULL);
+
+	if (tloc)
+		*tloc = tv.tv_sec;
+	return tv.tv_sec;
+}
+#define time(x) git_time(x)
+
 #ifdef NO_STRUCT_ITIMERVAL
 struct itimerval {
 	struct timeval it_interval;
-- 
2.40.0-115-ge25cabbf6b

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Paul Eggert]

Thanks, this looks good. As a matter of fact it almost precisely matches 
what I was about to email you. The only significant difference is that 
yours has "#define time(x) git_time(x)" whereas mine had "#define time 
git_time". Since Git never takes the address of 'time' the two macro 
definitions should have equivalent effects when used in Git.

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Junio C Hamano]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Thanks, this looks good. As a matter of fact it almost precisely
> matches what I was about to email you. The only significant difference
> is that yours has "#define time(x) git_time(x)" whereas mine had
> "#define time git_time". Since Git never takes the address of 'time'
> the two macro definitions should have equivalent effects when used in
> Git.

That is a valid concern.  Writing &time would not be caught by
compilers, and you would not notice such a mistake until you run "nm
-ug" on the result.

On the other hand, straight token replacement will risk renaming
variables and structure members, and I was not sure if we have such
use of the identifier "time".  As long as people do not use "time"
and "git_time" at the same time as such identifiers, that would not
be an issue (except for perhaps expecting to see them in debuggers).
Writing "git_time" and "time" at the same time for identifiers not
related to the time(2) function would not be caught by compilers,
either, but it feels much less likely mistake we would make in the
future, so let me drop (x) from the macro.

Thanks.

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Konstantin Khomoutov]

On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote:

[...]
> higher-res methods like gettimeofday.  Although strictly
> speaking users should tolerate this behavuior because a

A small typo: it should probably read "behavior" or "behaviour".

[...]

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Junio C Hamano]

Konstantin Khomoutov <kostix@bswap.ru> writes:

> On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote:
>
> [...]
>> higher-res methods like gettimeofday.  Although strictly
>> speaking users should tolerate this behavuior because a
>
> A small typo: it should probably read "behavior" or "behaviour".
>
> [...]

Thanks.

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Jeff King]

On Mon, Mar 20, 2023 at 04:05:07PM -0700, Junio C Hamano wrote:

> +#ifdef time
> +#undef time
> +#endif
> +static inline time_t git_time(time_t *tloc)
> +{
> +	struct timeval tv;
> +
> +	/*
> +	 * Avoid time(NULL), which can disagree with gettimeofday(2)
> +	 * and filesystem timestamps.
> +	 */
> +	gettimeofday(&tv, NULL);
> +
> +	if (tloc)
> +		*tloc = tv.tv_sec;
> +	return tv.tv_sec;
> +}
> +#define time(x) git_time(x)

This looks good to me, but I wanted to mention one alternative. If we
are declaring that time() sucks and gettimeofday() is how to do it, then
we could just use gettimeofday() everywhere, and add time() to banned.h
to catch stragglers.

It has two mild advantages:

  1. gettimeofday() gives the callers extra resolution if they want it
     (though in practice I guess none of them really do)

  2. It more directly describes what's going on, and we'd play fewer
     games with macros (though we may end up with a git_gettimeofday()
     wrapper if somebody doesn't support it; I really wonder about
     Windows here).

The disadvantage is that it's longer to type, and that you have to
declare a timeval in the caller. So maybe it's a dumb idea.

-Peff

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Taylor Blau]

On Tue, Mar 21, 2023 at 02:22:52PM -0400, Jeff King wrote:
> The disadvantage is that it's longer to type, and that you have to
> declare a timeval in the caller. So maybe it's a dumb idea.

I don't think it's a dumb idea per-se, but I think that being able to
pass `time(NULL)` around without having to create a timeval and pass a
pointer to *it* before then giving that timeval to some other function
is a nice advantage.

So, yeah, we probably should just avoid calling time() altogether, but
in practice I like the solution of redefining time() to do the right
thing and implement it by calling gettimeofday().

...Which is a long way of saying that I agree with you that this
approach looks good, and that I'd like to avoid putting time() in the
list of banned functions.

Thanks,
Taylor

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This looks good to me, but I wanted to mention one alternative. If we
> are declaring that time() sucks and gettimeofday() is how to do it, then
> we could just use gettimeofday() everywhere, and add time() to banned.h
> to catch stragglers.
>
> It has two mild advantages:
>
>   1. gettimeofday() gives the callers extra resolution if they want it
>      (though in practice I guess none of them really do)

True.  Many of our data structures do not have room to store the
extra resolution right now.

>   2. It more directly describes what's going on, and we'd play fewer
>      games with macros (though we may end up with a git_gettimeofday()
>      wrapper if somebody doesn't support it; I really wonder about
>      Windows here).

I think they already have a compat/ function for their use in
compat/mingw.c so that may not be an issue.

> The disadvantage is that it's longer to type, and that you have to
> declare a timeval in the caller. So maybe it's a dumb idea.

Yes, you have to declare and use a timeval, but you are already
declaring and using time_t in today's code, so if we were writing
like so from scratch, the result wouldn't have been so bad.  We just
do nto use time_t and use timeval instead consistently.

The patch noise to go from here to there may not be worth it,
though.

Also, unless we are going to actively take advantage of extra
resolution, I am not sure if it is wise to ask for extra resolution
in the first place.  If time(2), at least on some platforms whose
maintainers care, gets corrected to avoid going backwards or being
inconsistent with filesystem timestamp in the future, converting
everything that calls time(2) to call gettimeofday(2) would lose
information---we will want to know which ones need only seconds
resolution and convert them back when the world advances.

So, I am not quite sure I am sold on the idea of rewriting
everything to use gettimeofday(2).

Thanks.

Re: [PATCH v2] git-compat-util: use gettimeofday(2) for time(2)

[Jeff King]

On Tue, Mar 21, 2023 at 01:04:49PM -0700, Junio C Hamano wrote:

> So, I am not quite sure I am sold on the idea of rewriting
> everything to use gettimeofday(2).

I'm not sure I am either. :) All your points are fair. Let's forget
about my suggestion.

-Peff