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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxx>
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@xxxxxx>
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
