Re: [PATCH 0/4] midx: avoid potential rename-while-mapped on Windows
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Oct 13, 2021, 13:29 UTC
- Message-ID
- <42a8d2ef-3a67-ca2c-4334-b79975d45da7@gmail.com>
- In-Reply-To
- <cover.1633729502.git.me@ttaylorr.com>
On 10/8/2021 5:46 PM, Taylor Blau wrote:
Show 23 quoted lines
> (This topic is based on tb/repack-write-midx). > > While looking through some failures in t5319 when Git is compiled with > SANITIZE=leak, I noticed a curious pattern that basically looks like: > > struct multi_pack_index *m = load_multi_pack_index(object_dir, 0); > /* ... */ > write_midx_internal(); > > This can cause problems on platforms where a memory mapped file cannot be > overridden via a rename, because load_multi_pack_index() builds a new MIDX from > scratch each time it is called without adding that MIDX to the object store. But > write_midx_internal() loads a MIDX from the object store, causing the same > region to be mapped twice. > > The details are fully spelled out in the second patch, but the gist is that > while write_midx_internal() unmaps *its* copy of the MIDX, the caller's copy is > still mapped in memory. > > Strangely, I could not get this to produce a failure on Windows, even though I > would have expected one. I asked around off-list, and it sounds like it's > possible this behavior has changed, or Windows' mmap-equivalent may not lock > files that fit within a single page.
That's a good possibility. Once everything is loaded into memory, there is no need for a file handle.
> In any case, we let's side step the issue entirely by only interacting with > MIDXs that can be found from the object store.
I think this is a good strategy, and the patch series goes above and beyond this one failure by making it easier to get the MIDX from a specific alternate.
Thanks, -Stolee