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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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:
>
> > From: Jeff Hostetler <jeffhost@microsoft.com>
> >
> > 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 <Johannes.Schindelin@gmx.de>
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 <johannes.schindelin@gmx.de>

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