[0/2] Fix memory corruption with FSMonitor-enabled unpack_trees()

Message

Jean-Noël Avila via GitGitGadget March 17, 2021, 3:30 p.m. UTC

While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really
obscure segmentation fault during one of my epic Git for Windows rebases.
Turns out that this bug is quite old.

Johannes Schindelin (2):
  fsmonitor: fix memory corruption in some corner cases
  fsmonitor: do not forget to release the token in `discard_index()`

 read-cache.c   | 1 +
 unpack-trees.c | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)


base-commit: dfaed028620c2dca08d24583c7fc8b0aef9b6d0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-891%2Fdscho%2Ffix-fsmonitor-crash-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-891/dscho/fix-fsmonitor-crash-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/891

Comments

Derrick Stolee March 17, 2021, 8:21 p.m. UTC | #1

On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote:
> While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really
> obscure segmentation fault during one of my epic Git for Windows rebases.

Thanks for dogfooding!

> Turns out that this bug is quite old.

A little over a year, yes, since the v2 hook was committed. It's old
enough that these could be applied to 'maint'.
 
> Johannes Schindelin (2):
>   fsmonitor: fix memory corruption in some corner cases
>   fsmonitor: do not forget to release the token in `discard_index()`

The patches themselves are correct and describe the problem well.
They only show up during non-trivial uses of FS Monitor and index
updates, so I understand your hesitance to write tests that trigger
these problems.

Thanks,
-Stolee

Johannes Schindelin March 19, 2021, 2:49 p.m. UTC | #2

Hi Stolee,

On Wed, 17 Mar 2021, Derrick Stolee wrote:

> On 3/17/2021 11:30 AM, Johannes Schindelin via GitGitGadget wrote:
> > While dog-fooding Jeff Hostetler's FSMonitor patches, I ran into a really
> > obscure segmentation fault during one of my epic Git for Windows rebases.
>
> Thanks for dogfooding!

I'm completely selfish here, as I want to benefit from the speed myself,
and that's also the reason why I added this as an experimental option to
Git for Windows v2.31.0: That way, I can test it everywhere (and so can
others).

> > Turns out that this bug is quite old.
>
> A little over a year, yes, since the v2 hook was committed. It's old
> enough that these could be applied to 'maint'.

Indeed. Even better: if you look closely at the GitGitGadget PR, you will
see that I based it on `kw/fsmonitor-watchman-racefix`.

> > Johannes Schindelin (2):
> >   fsmonitor: fix memory corruption in some corner cases
> >   fsmonitor: do not forget to release the token in `discard_index()`
>
> The patches themselves are correct and describe the problem well.
> They only show up during non-trivial uses of FS Monitor and index
> updates, so I understand your hesitance to write tests that trigger
> these problems.

Right. For me, I ran into them only in one specific instance, when
rebasing Git for Windows' patch thicket onto `seen`, and then only when
merging a topic branch with a rather big diff.

Thanks,
Dscho