Re: [PATCH] fsmonitor: avoid global-buffer-overflow READ when checking trivial response

On 3/15/21 12:39 PM, Andrzej Hunt via GitGitGadget wrote:

From: Andrzej Hunt <redacted>

query_result can be be an empty strbuf (STRBUF_INIT) - in that case
trying to read 3 bytes triggers a buffer overflow read (as
query_result.buf = '\0').

Therefore we need to check query_result's length before trying to read 3
bytes.

This overflow was introduced in:
   940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
It was found when running the test-suite against ASAN, and can be most
easily reproduced with the following command:

[...]

     fsmonitor: fix overflow read
     
     This patch fixes a buffer overflow read in
     fsmonitor_is_trivial_response().
     
     I'm not super familiar with fsmonitor, so I'm not 100% sure what the
     empty response actually means. Based on my reading of the docs below,
     this can happen with fsmonitor-watchman v1 when no files have changed.
     But it could also happen for v2 if the implementation is broken (in
     which case we also shouldn't overflow)? Either way, I'm guessing the
     empty response doesn't count as trivial:
     https://git-scm.com/docs/githooks#_fsmonitor_watchman
     
     The other question I had is: can watchman V1 return "/\0" as the trivial
     response (as it has no token header) - and should we be recognising that
     too?
     
     ATB,
     
     Andrzej

[...]

Looks good to me.  And thanks for catching this.

WRT your questions:

An empty response means no files have changed since the last query.
The client can assume all cache-entries are valid and doesn't need
to scan.

A "trivial" response means that the monitor doesn't have enough
information to answer the question. The client should assume that
everything is invalid and do a full scan (as if no monitor were
present).

I added the `fsmonitor_is_trivial_response()` function with the
tracing that I added in [1] in preparation for adding a builtin
fsmonitor service (and currently only my tracing uses that function),
but the concept of a trivial "/" response line has been present since
the initial fsmonitor implementation [2].   See [3] and [4].

[1] 940b94f35c (fsmonitor: log invocation of FSMonitor hook to trace2, 2021-02-03)
[2] 883e248b8a (fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files., 2017-09-22)
[3] https://github.com/git/git/blob/a5828ae6b52137b913b978e16cd2334482eb4c1f/fsmonitor.c#L304
[4] https://github.com/git/git/blob/a5828ae6b52137b913b978e16cd2334482eb4c1f/fsmonitor.c#L320

Thanks again for the review.
Jeff