Re: [PATCH v2 04/28] fsmonitor-ipc: create client routines for git-fsmonitor--daemon
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jun 14, 2021, 21:23 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2106142320070.57@tvgsbejvaqbjf.bet>
- In-Reply-To
- <nycvar.QRO.7.76.6.2106021320490.55@tvgsbejvaqbjf.bet>
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...
Show 45 quoted lines
> 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/3241I 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