"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> The `shown` kh_str_t was freed with kh_release_str() at a point in
> the code only reachable in the non-trivial response path.  When the
> client receives a trivial response, the code jumps to the `cleanup`
> label, skipping the kh_release_str() call entirely and leaking the
> hash table.
>
> Fix this by initializing `shown` to NULL and moving the cleanup to the
> `cleanup` label using kh_destroy_str(), which is safe to call on NULL.
> This ensures the hash table is freed regardless of which code path is
> taken.

Makes perfect sense, the changes to the code matches the
description, and the difference between kh_release_* and
kh_destroy_* in khash.h is exactly as described in the log message.

Perfect.  I wish all the patches posted here are as easy to review
as this one ;-)

> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
>  builtin/fsmonitor--daemon.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 242c594646..bc4571938c 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -671,7 +671,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
>  	const struct fsmonitor_batch *batch;
>  	struct fsmonitor_batch *remainder = NULL;
>  	intmax_t count = 0, duplicates = 0;
> -	kh_str_t *shown;
> +	kh_str_t *shown = NULL;
>  	int hash_ret;
>  	int do_trivial = 0;
>  	int do_flush = 0;
> @@ -909,8 +909,6 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
>  		total_response_len += payload.len;
>  	}
>  
> -	kh_release_str(shown);
> -
>  	pthread_mutex_lock(&state->main_lock);
>  
>  	if (token_data->client_ref_count > 0)
> @@ -954,6 +952,7 @@ static int do_handle_client(struct fsmonitor_daemon_state *state,
>  	trace2_data_intmax("fsmonitor", the_repository, "response/count/duplicates", duplicates);
>  
>  cleanup:
> +	kh_destroy_str(shown);
>  	strbuf_release(&response_token);
>  	strbuf_release(&requested_token_id);
>  	strbuf_release(&payload);

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> The cookie wait in with_lock__wait_for_cookie() uses an infinite
> pthread_cond_wait() loop.  The existing comment notes the desire
> to switch to pthread_cond_timedwait(), but the routine was not
> available in git thread-utils.
>
> On certain container or overlay filesystems, inotify watches may
> succeed but events are never delivered.  In this case the daemon
> would hang indefinitely waiting for the cookie event, which in
> turn causes the client to hang.
>
> Replace the infinite wait with a one-second timeout using
> pthread_cond_timedwait().  If the timeout fires, report an
> error and let the client proceed with a trivial (full-scan)
> response rather than blocking forever.

I cannot convince myself if one-second interval is not too frequent
to force everybody, including those with working inotify, to poll.
I wonder if this is something that may want to be configurable (or
better yet, auto-detectable, but that may be wishing for moon).

> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
>  builtin/fsmonitor--daemon.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
> index 4d52622e24..f6c406ff12 100644
> --- a/builtin/fsmonitor--daemon.c
> +++ b/builtin/fsmonitor--daemon.c
> @@ -197,20 +197,31 @@ static enum fsmonitor_cookie_item_result with_lock__wait_for_cookie(
>  	unlink(cookie_pathname.buf);
>  
>  	/*
> -	 * Technically, this is an infinite wait (well, unless another
> -	 * thread sends us an abort).  I'd like to change this to
> -	 * use `pthread_cond_timedwait()` and return an error/timeout
> -	 * and let the caller do the trivial response thing, but we
> -	 * don't have that routine in our thread-utils.
> -	 *
> -	 * After extensive beta testing I'm not really worried about
> -	 * this.  Also note that the above open() and unlink() calls
> -	 * will cause at least two FS events on that path, so the odds
> -	 * of getting stuck are pretty slim.
> +	 * Wait for the listener thread to see the cookie file.
> +	 * Time out after a short interval so that the client
> +	 * does not hang forever if the filesystem does not deliver
> +	 * events (e.g., on certain container/overlay filesystems
> +	 * where inotify watches succeed but events never arrive).
>  	 */
> -	while (cookie->result == FCIR_INIT)
> -		pthread_cond_wait(&state->cookies_cond,
> -				  &state->main_lock);
> +	{
> +		struct timeval now;
> +		struct timespec ts;
> +		int err = 0;
> +
> +		gettimeofday(&now, NULL);
> +		ts.tv_sec = now.tv_sec + 1;
> +		ts.tv_nsec = now.tv_usec * 1000;
> +
> +		while (cookie->result == FCIR_INIT && !err)
> +			err = pthread_cond_timedwait(&state->cookies_cond,
> +						     &state->main_lock,
> +						     &ts);
> +		if (err == ETIMEDOUT && cookie->result == FCIR_INIT) {
> +			trace_printf_key(&trace_fsmonitor,
> +					 "cookie_wait timed out");
> +			cookie->result = FCIR_ERROR;
> +		}
> +	}
>  
>  done:
>  	hashmap_remove(&state->cookies, &cookie->entry, NULL);

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

> I cannot convince myself if one-second interval is not too frequent
> to force everybody, including those with working inotify, to poll.
> I wonder if this is something that may want to be configurable (or
> better yet, auto-detectable, but that may be wishing for moon).

The 1-second timeout only fires when the filesystem fails to deliver
the cookie event at all (e.g. overlayfs in containers where inotify
watches succeed but events never arrive). On a working filesystem
the cookie event comes back in well under a millisecond, so the
timeout never triggers. When it does fire, the client falls back to
a full scan, which is the safe default. Happy to make it
configurable if you think that's worth it, but the current behavior
seemed reasonable as a starting point.

Thanks,
Paul

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	 * Wait for the listener thread to see the cookie file.

Is this a complete sentence?  Or perhaps something like "see" ->
"check"?

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

> Is this a complete sentence?  Or perhaps something like "see" ->
> "check"?

Fixed, changed to "observe".

Thanks,
Paul

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> The IPC path logic for determining the Unix domain socket location is
> nearly identical between macOS and Linux.  Both need to check whether
> the .git directory is on a remote filesystem and, if so, fall back to
> a socket path under $HOME or a user-configured directory.
>
> Merge the two implementations into a single fsm-ipc-unix.c that is
> shared by both platforms.  The unified version includes the worktree
> NULL check (BUG guard) from the Linux implementation, which was missing
> in the macOS version.
>
> Update Makefile, meson.build, and CMakeLists.txt to use the new shared
> file for non-Windows platforms.

This sounds as if the patch started with two IPC path logic for
macOS and Linux, and the patch removes one of them and updates the
other one so that both platforms can use the surviving one.

But the patch seems to indicate somewhat different story.  The code
before this patch started with a single macOS (darwin) one, but
because it is mostly applicable to other UNIX variants as well, the
patch renames the existing macOS one for unix and makes a small
adjustment (namely, asserts that r->worktree is not NULL).

Perhaps you started with two separate implementations (possibly with
a new one called UNIX that was added by largely copying and pasting
from the macOS one) and unified them, but that history does not
exist in this 10-patch series, so the above story would need to be
rewritten to say what actually is happening in this series, e.g., we
realized macOS one is applicable generally for UNIX variants so we
are renaming it from darwin to unix, or something.

> @@ -2365,7 +2365,11 @@ ifdef FSMONITOR_DAEMON_BACKEND
>  	COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
>  	COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND).o
>  	COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o
> -	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o
> +ifeq ($(FSMONITOR_DAEMON_BACKEND),win32)
> +	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-win32.o
> +else
> +	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-unix.o
> +endif

Makes me wonder if doing

    #define FSMONITOR_DAEMON_BACKEND unix

for macOS and then keeping

	COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o

as-is would be cleaner.  

Later, we can add the same "#define FSMONITOR_DAEMON_BACKEND unix"
for Linux, perhaps?

This is doubly true when we look at the build recipe changes to the
meson one below ...

> diff --git a/meson.build b/meson.build
> index dd52efd1c8..8de795f9d4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1332,11 +1332,16 @@ if fsmonitor_backend != ''
>  
>    libgit_sources += [
>      'compat/fsmonitor/fsm-health-' + fsmonitor_backend + '.c',
> -    'compat/fsmonitor/fsm-ipc-' + fsmonitor_backend + '.c',
>      'compat/fsmonitor/fsm-listen-' + fsmonitor_backend + '.c',
>      'compat/fsmonitor/fsm-path-utils-' + fsmonitor_backend + '.c',
>      'compat/fsmonitor/fsm-settings-' + fsmonitor_backend + '.c',
>    ]
> +
> +  if fsmonitor_backend == 'win32'
> +    libgit_sources += 'compat/fsmonitor/fsm-ipc-win32.c'
> +  else
> +    libgit_sources += 'compat/fsmonitor/fsm-ipc-unix.c'
> +  endif
>  endif

... which would not be needed if fsmonitor_backend is set to 'unix'
instead of 'darwin'.

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

> But the patch seems to indicate somewhat different story.  The code
> before this patch started with a single macOS (darwin) one, but
> because it is mostly applicable to other UNIX variants as well, the
> patch renames the existing macOS one for unix and makes a small
> adjustment (namely, asserts that r->worktree is not NULL).

Good point. Restructured: FSMONITOR_OS_SETTINGS is now set to "unix"
for both macOS and Linux (and "win32" for Windows). The build files
use $(FSMONITOR_OS_SETTINGS) for fsm-ipc and fsm-settings, and
$(FSMONITOR_DAEMON_BACKEND) for the platform-specific files (listen,
health, path-utils). No more if/else blocks.

Also rewrote both commit messages to describe what's actually
happening (renaming darwin to unix) rather than "merging two
implementations".

Thanks,
Paul

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> The fsmonitor settings logic for checking Unix domain socket
> compatibility is nearly identical between macOS and Linux.  Both check
> whether the IPC socket directory resides on a remote filesystem or a
> filesystem that does not support Unix domain sockets (NTFS, FAT32, etc).
>
> Merge the two implementations into a single fsm-settings-unix.c shared
> by both platforms.  The unified version uses the safer xstrdup() +
> dirname() approach from the macOS implementation (avoiding strbuf
> mutation with dirname()) and includes the "vfat" filesystem check.
>
> Update Makefile, meson.build, and CMakeLists.txt to use the new shared
> file for non-Windows platforms.

I guess exactly the same comment as the one for 05/10 applies here
as well?

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

> I guess exactly the same comment as the one for 05/10 applies here
> as well?

Yep, same fix applied here. Also rewrote both commit messages to
describe what's actually happening (renaming darwin to unix) rather
than "merging two implementations".

Thanks,
Paul

"Paul Tarjan via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Paul Tarjan <github@paulisageek.com>
>
> Add a new option to struct child_process that closes file descriptors
> 3 and above in the child after forking but before exec.  This prevents
> long-running child processes from inheriting pipe endpoints or other
> descriptors from the parent environment.
>
> The upper bound for the fd scan comes from sysconf(_SC_OPEN_MAX),
> capped at 4096 to avoid excessive iteration when the limit is set
> very high.
>
> Signed-off-by: Paul Tarjan <github@paulisageek.com>
> ---
>  run-command.c | 11 +++++++++++
>  run-command.h |  9 +++++++++
>  2 files changed, 20 insertions(+)

All makes sense.

I somehow find it a bit surprising that we did not already have this
feature anywhere, as closing all except for the low file descriptors
connected to stdio by default is fairly a common thing to do.


> diff --git a/run-command.c b/run-command.c
> index e3e02475cc..cbadcf5ff8 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -832,6 +832,17 @@ fail_pipe:
>  			child_close(cmd->out);
>  		}
>  
> +		if (cmd->close_fd_above_stderr) {
> +			long max_fd = sysconf(_SC_OPEN_MAX);
> +			int fd;
> +			if (max_fd < 0 || max_fd > 4096)
> +				max_fd = 4096;
> +			for (fd = 3; fd < max_fd; fd++) {
> +				if (fd != child_notifier)
> +					close(fd);
> +			}
> +		}
> +
>  		if (cmd->dir && chdir(cmd->dir))
>  			child_die(CHILD_ERR_CHDIR);
>  
> diff --git a/run-command.h b/run-command.h
> index 0df25e445f..a1aa1b1069 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -141,6 +141,15 @@ struct child_process {
>  	unsigned stdout_to_stderr:1;
>  	unsigned clean_on_exit:1;
>  	unsigned wait_after_clean:1;
> +
> +	/**
> +	 * Close file descriptors 3 and above in the child after forking
> +	 * but before exec.  This prevents the long-running child from
> +	 * inheriting pipe endpoints or other descriptors from the parent
> +	 * environment (e.g., the test harness).
> +	 */
> +	unsigned close_fd_above_stderr:1;
> +
>  	void (*clean_on_exit_handler)(struct child_process *process);
>  };