[PATCH 0/4] midx: avoid potential rename-while-mapped on Windows

[PATCH 0/4] midx: avoid potential rename-while-mapped on Windows

[Taylor Blau]

(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.

In any case, we let's side step the issue entirely by only interacting with
MIDXs that can be found from the object store.

Taylor Blau (4):
  midx.c: extract MIDX lookup by object_dir
  midx.c: lookup MIDX by object directory during expire
  midx.c: lookup MIDX by object directory during repack
  midx.c: guard against commit_lock_file() failures

 midx.c | 42 +++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

-- 
2.33.0.96.g73915697e6

[PATCH 1/4] midx.c: extract MIDX lookup by object_dir

[Taylor Blau]

The first thing that write_midx_internal() does is load the MIDX
corresponding to the given object directory, if one is present.

Prepare for other functions in midx.c to do the same thing by extracting
that operation out to a small helper function.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/midx.c b/midx.c
index 18a4f25aa3..b66b75a3cd 100644
--- a/midx.c
+++ b/midx.c
@@ -1118,6 +1118,22 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,
 	return ret;
 }
 
+static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,
+							const char *object_dir)
+{
+	struct multi_pack_index *cur;
+
+	/* Ensure the given object_dir is local, or a known alternate. */
+	find_odb(r, object_dir);
+
+	for (cur = get_multi_pack_index(r); cur; cur = cur->next) {
+		if (!strcmp(object_dir, cur->object_dir))
+			return cur;
+	}
+
+	return NULL;
+}
+
 static int write_midx_internal(const char *object_dir,
 			       struct string_list *packs_to_include,
 			       struct string_list *packs_to_drop,
@@ -1131,15 +1147,11 @@ static int write_midx_internal(const char *object_dir,
 	struct hashfile *f = NULL;
 	struct lock_file lk;
 	struct write_midx_context ctx = { 0 };
-	struct multi_pack_index *cur;
 	int pack_name_concat_len = 0;
 	int dropped_packs = 0;
 	int result = 0;
 	struct chunkfile *cf;
 
-	/* Ensure the given object_dir is local, or a known alternate. */
-	find_odb(the_repository, object_dir);
-
 	midx_name = get_midx_filename(object_dir);
 	if (safe_create_leading_directories(midx_name))
 		die_errno(_("unable to create leading directories of %s"),
@@ -1151,12 +1163,7 @@ static int write_midx_internal(const char *object_dir,
 		 * packs to include, since all packs and objects are copied
 		 * blindly from an existing MIDX if one is present.
 		 */
-		for (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) {
-			if (!strcmp(object_dir, cur->object_dir)) {
-				ctx.m = cur;
-				break;
-			}
-		}
+		ctx.m = lookup_multi_pack_index(the_repository, object_dir);
 	}
 
 	if (ctx.m && !midx_checksum_valid(ctx.m)) {
-- 
2.33.0.96.g73915697e6

[PATCH 2/4] midx.c: lookup MIDX by object directory during expire

[Taylor Blau]

Before a new MIDX can be written, expire_midx_packs() first loads the
existing MIDX, figures out which packs can be expired, and then writes a
new MIDX based on that information.

In order to load the existing MIDX, it uses load_multi_pack_index(),
which mmaps the multi-pack-index file, but does not store the resulting
`struct multi_pack_index *` in the object store.

write_midx_internal() also needs to open the existing MIDX, and it does
so by iterating the results of get_multi_pack_index(), so that it reuses
the same pointer held by the object store. But before it can move the
new MIDX into place, it close_object_store() to munmap() the
multi-pack-index file to accommodate platforms like Windows which don't
allow overwriting files which are memory mapped.

That's where things get weird. Since expire_midx_packs has its own
*separate* memory mapped copy of the MIDX, the MIDX file is still memory
mapped! Interestingly, this doesn't seem to cause a problem in our
tests. (I believe that this has much more to do with my own lack of
familiarity with Windows than it does a lack of coverage in our tests).

In any case, we can side-step the whole issue by teaching
expire_midx_packs() to use the `struct multi_pack_index` pointer it
found via the object store instead of maintain its own copy. That way,
when write_midx_internal() calls `close_object_store()`, we know that
there are no memory mapped copies of the MIDX laying around.

A couple of other small notes about this patch:

  - As far as I can tell, passing `local == 1` to the call to
    load_multi_pack_index() was an error, since object_dir could be an
    alternate. But it doesn't matter, since even though we write
    `m->local = 1`, we never read that field back later on.

  - Setting `m = NULL` after write_midx_internal() was likely to prevent
    a double-free back from when that function took a `struct
    multi_pack_index *` that it called close_midx() on itself. We can
    rely on write_midx_internal() to call that for us now.

Finally, this enforces the same "the value of --object-dir must be the
local object store, or an alternate" rule from f57a739691 (midx: avoid
opening multiple MIDXs when writing, 2021-09-01) to the `expire`
sub-command, too.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
This does leak the MIDX write_midx_internal returns before calling
close_object_store(). We can't just blindly call close_object_store()
here, either, since it's susceptible to double-frees. I'll think about
improving this in a separate series.

 midx.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/midx.c b/midx.c
index b66b75a3cd..7f1addf4b6 100644
--- a/midx.c
+++ b/midx.c
@@ -1707,7 +1707,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla
 {
 	uint32_t i, *count, result = 0;
 	struct string_list packs_to_drop = STRING_LIST_INIT_DUP;
-	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
+	struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);
 	struct progress *progress = NULL;

 	if (!m)
@@ -1752,12 +1752,11 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla

 	free(count);

-	if (packs_to_drop.nr) {
+	if (packs_to_drop.nr)
 		result = write_midx_internal(object_dir, NULL, &packs_to_drop, NULL, NULL, flags);
-		m = NULL;
-	}

 	string_list_clear(&packs_to_drop, 0);
+
 	return result;
 }

--
2.33.0.96.g73915697e6

[PATCH 3/4] midx.c: lookup MIDX by object directory during repack

[Taylor Blau]

Apply similar treatment as in the last commit to the MIDX `repack`
operation.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/midx.c b/midx.c
index 7f1addf4b6..137af3fc67 100644
--- a/midx.c
+++ b/midx.c
@@ -1872,7 +1872,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	FILE *cmd_in;
 	struct strbuf base_name = STRBUF_INIT;
-	struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
+	struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);
 
 	/*
 	 * When updating the default for these configuration
@@ -1944,11 +1944,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
 	}
 
 	result = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);
-	m = NULL;
 
 cleanup:
-	if (m)
-		close_midx(m);
 	free(include_pack);
 	return result;
 }
-- 
2.33.0.96.g73915697e6

[PATCH 4/4] midx.c: guard against commit_lock_file() failures

[Taylor Blau]

When writing a MIDX, we atomically move the new MIDX into place via
commit_lock_file(), but do not check to see if that call was successful.

Make sure that we do check in order to prevent us from incorrectly
reporting that we wrote a new MIDX if we actually encountered an error.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/midx.c b/midx.c
index 137af3fc67..e79fb4427d 100644
--- a/midx.c
+++ b/midx.c
@@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir,
 	if (ctx.m)
 		close_object_store(the_repository->objects);
 
-	commit_lock_file(&lk);
+	if (commit_lock_file(&lk) < 0)
+		die_errno(_("could not write multi-pack-index"));
 
 	clear_midx_files_ext(object_dir, ".bitmap", midx_hash);
 	clear_midx_files_ext(object_dir, ".rev", midx_hash);
-- 
2.33.0.96.g73915697e6

Re: [PATCH 4/4] midx.c: guard against commit_lock_file() failures

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 08 2021, Taylor Blau wrote:

> When writing a MIDX, we atomically move the new MIDX into place via
> commit_lock_file(), but do not check to see if that call was successful.
>
> Make sure that we do check in order to prevent us from incorrectly
> reporting that we wrote a new MIDX if we actually encountered an error.
>
>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
>  midx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/midx.c b/midx.c
> index 137af3fc67..e79fb4427d 100644
> --- a/midx.c
> +++ b/midx.c
> @@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir,
>  	if (ctx.m)
>  		close_object_store(the_repository->objects);
>  
> -	commit_lock_file(&lk);
> +	if (commit_lock_file(&lk) < 0)
> +		die_errno(_("could not write multi-pack-index"));
>  
>  	clear_midx_files_ext(object_dir, ".bitmap", midx_hash);
>  	clear_midx_files_ext(object_dir, ".rev", midx_hash);

As an aside I've wondered with this API whether something like this
wouldn't make sense (obviously with a better message):

diff --git a/lockfile.c b/lockfile.c
index cc9a4b84283..6632a634f00 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -175,6 +175,7 @@ int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,
 					   long timeout_ms, int mode)
 {
 	int fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);
+	lk->flags = flags;
 	if (fd < 0) {
 		if (flags & LOCK_DIE_ON_ERROR)
 			unable_to_lock_die(path, errno);
@@ -209,6 +210,8 @@ int commit_lock_file(struct lock_file *lk)
 		int save_errno = errno;
 		free(result_path);
 		errno = save_errno;
+		if (lk->flags & LOCK_DIE_ON_ERROR)
+			die_errno("noes");
 		return -1;
 	}
 	free(result_path);
diff --git a/lockfile.h b/lockfile.h
index db93e6ba73e..40a9d91a6c5 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -119,6 +119,7 @@
 
 struct lock_file {
 	struct tempfile *tempfile;
+	int flags;
 };
 
 #define LOCK_INIT { NULL }

But a quick grep of the users reveals that some die on lock failure, but
are happy to ignore commit_lock_file() failures, or maybe those are also
bugs similar to the one you're fixing here & we could fix the API more
generally.

Re: [PATCH 2/4] midx.c: lookup MIDX by object directory during expire

[Derrick Stolee]

On 10/8/2021 5:46 PM, Taylor Blau wrote:
> Before a new MIDX can be written, expire_midx_packs() first loads the
> existing MIDX, figures out which packs can be expired, and then writes a
> new MIDX based on that information.
> 
> In order to load the existing MIDX, it uses load_multi_pack_index(),
> which mmaps the multi-pack-index file, but does not store the resulting
> `struct multi_pack_index *` in the object store.
> 
> write_midx_internal() also needs to open the existing MIDX, and it does
> so by iterating the results of get_multi_pack_index(), so that it reuses
> the same pointer held by the object store. But before it can move the
> new MIDX into place, it close_object_store() to munmap() the
> multi-pack-index file to accommodate platforms like Windows which don't
> allow overwriting files which are memory mapped.
> 
> That's where things get weird. Since expire_midx_packs has its own
> *separate* memory mapped copy of the MIDX, the MIDX file is still memory
> mapped! Interestingly, this doesn't seem to cause a problem in our
> tests. (I believe that this has much more to do with my own lack of
> familiarity with Windows than it does a lack of coverage in our tests).

You are fixing a bug in two ways:

1. This change ensures we use the 'struct multi_pack_index' that exists
   in the object store, ensuring it is closed before committing the lockfile.

2. Without this change, the change in patch 4 would cause the 'expire' tests
   to start failing on Windows, because the commands would die().

If our tests also verified that the .git/objects/pack/multi-pack-index.lock
file was missing at the end of the command, then we would have caught this
bug on Windows. I don't think that's a reasonable test, but instead we(I)
should have used the API correctly from the start.

The tests _do_ verify that the expired packs are deleted, but the new MIDX
probably refers to the old packs still. Since those packs are not actually
used (the necessary condition for expiring them), later Git commands do not
attempt to load them and hence do not fail. That is, until we try to expire
again and likely get warnings about missing pack-files.

Thanks,
-Stolee

Re: [PATCH 0/4] midx: avoid potential rename-while-mapped on Windows

[Derrick Stolee]

On 10/8/2021 5:46 PM, Taylor Blau wrote:
> (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