Re: [PATCH v2 04/28] fsmonitor-ipc: create client routines for git-fsmonitor--daemon

Hi Jeff,

On Wed, 2 Jun 2021, Johannes Schindelin wrote:

I know you're on vacation, therefore I would like to apologize for adding
to your post-vacation notification overload, but...

Now that you're back from vacation...

On Sat, 22 May 2021, Jeff Hostetler via GitGitGadget wrote:

quoted

From: Jeff Hostetler <redacted>

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
new file mode 100644
index 000000000000..e62901a85b5d
--- /dev/null
+++ b/fsmonitor-ipc.c

@@ -0,0 +1,179 @@

[...]
+
+int fsmonitor_ipc__send_query(const char *since_token,
+			      struct strbuf *answer)
+{
+	int ret = -1;
+	int tried_to_spawn = 0;
+	enum ipc_active_state state = IPC_STATE__OTHER_ERROR;
+	struct ipc_client_connection *connection = NULL;
+	struct ipc_client_connect_options options
+		= IPC_CLIENT_CONNECT_OPTIONS_INIT;
+
+	options.wait_if_busy = 1;
+	options.wait_if_not_found = 0;
+
+	trace2_region_enter("fsm_client", "query", NULL);
+
+	trace2_data_string("fsm_client", NULL, "query/command",
+			   since_token);
+
+try_again:
+	state = ipc_client_try_connect(fsmonitor_ipc__get_path(), &options,
+				       &connection);
+
+	switch (state) {
+	case IPC_STATE__LISTENING:
+		ret = ipc_client_send_command_to_connection(
+			connection, since_token, strlen(since_token), answer);

Here, `since_token` can be `NULL` (and hence the `strlen(since_token)` can
lead to a segmentation fault). I ran into this situation while `git rebase
-i --autostash` wanted to apply the stashed changes.

Since I picked up your v2 and included it in Git for Windows v2.32.0-rc2,
I needed this hotfix: https://github.com/git-for-windows/git/pull/3241

I actually noticed another similar issue and fixed it in time for Git for
Windows v2.32.0, but eventually figured out the actual culprit, with a
much better fix:

-- snip --
commit bc40a560d3c95040b55fd7be6fe5b7012d267f8f
Author: Johannes Schindelin [off-list ref]
Date:   Wed Jun 9 09:49:50 2021 +0200

    fixup! fsmonitor: introduce `core.useBuiltinFSMonitor` to call the daemon via IPC

    In FSMonitor v1, we made sure to only use a valid `since_token` when
    querying the FSMonitor. This condition was accidentally lost in v2, and
    caused segmentation faults uncovered by Scalar's Functional Tests.

    I had tried to fix this in https://github.com/git-for-windows/pull/3241,
    but the fix was incomplete, and I had to follow up with
    https://github.com/git-for-windows/pull/3258. However, it turns out that
    both of them were actually only work-arounds; I should have dug deeper
    to figure out _why_ the `since_token` was no longer guaranteed not to be
    `NULL`, and I finally did.

    Signed-off-by: Johannes Schindelin [off-list ref]

diff --git a/fsmonitor.c b/fsmonitor.c
index 22623fd228f..0b40643442e 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c

@@ -290,8 +290,9 @@ void refresh_fsmonitor(struct index_state *istate)
 	trace_printf_key(&trace_fsmonitor, "refresh fsmonitor");

 	if (r->settings.use_builtin_fsmonitor > 0) {
-		query_success = !fsmonitor_ipc__send_query(
-			istate->fsmonitor_last_update, &query_result);
+		query_success = istate->fsmonitor_last_update &&
+			!fsmonitor_ipc__send_query(istate->fsmonitor_last_update,
+						   &query_result);
 		if (query_success) {
 			/*
 			 * The response contains a series of nul terminated

-- snap --

Would you mind squashing this in when you re-roll?

Ciao,
Dscho