{"thread":{"id":"56669","subject":"[PATCH 0/4] midx: avoid potential rename-while-mapped on Windows","startedAt":"2021-10-08T21:46:30Z","lastAt":"2021-10-13T13:30:02Z","messageCount":8,"participants":["Taylor Blau","Ævar Arnfjörð Bjarmason","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"438363","messageId":"cover.1633729502.git.me@ttaylorr.com","threadId":"56669","inReplyTo":null,"subject":"[PATCH 0/4] midx: avoid potential rename-while-mapped on Windows","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-08T21:46:25Z","receivedAt":"2021-10-08T21:46:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"(This topic is based on tb/repack-write-midx).\n\nWhile looking through some failures in t5319 when Git is compiled with\nSANITIZE=leak, I noticed a curious pattern that basically looks like:\n\n    struct multi_pack_index *m = load_multi_pack_index(object_dir, 0);\n    /* ... */\n    write_midx_internal();\n\nThis can cause problems on platforms where a memory mapped file cannot be\noverridden via a rename, because load_multi_pack_index() builds a new MIDX from\nscratch each time it is called without adding that MIDX to the object store. But\nwrite_midx_internal() loads a MIDX from the object store, causing the same\nregion to be mapped twice.\n\nThe details are fully spelled out in the second patch, but the gist is that\nwhile write_midx_internal() unmaps *its* copy of the MIDX, the caller's copy is\nstill mapped in memory.\n\nStrangely, I could not get this to produce a failure on Windows, even though I\nwould have expected one. I asked around off-list, and it sounds like it's\npossible this behavior has changed, or Windows' mmap-equivalent may not lock\nfiles that fit within a single page.\n\nIn any case, we let's side step the issue entirely by only interacting with\nMIDXs that can be found from the object store.\n\nTaylor Blau (4):\n  midx.c: extract MIDX lookup by object_dir\n  midx.c: lookup MIDX by object directory during expire\n  midx.c: lookup MIDX by object directory during repack\n  midx.c: guard against commit_lock_file() failures\n\n midx.c | 42 +++++++++++++++++++++++-------------------\n 1 file changed, 23 insertions(+), 19 deletions(-)\n\n-- \n2.33.0.96.g73915697e6\n"},{"id":"438364","messageId":"ecb454233a4fcc88e1ca142b633a5785b6776a2e.1633729502.git.me@ttaylorr.com","threadId":"56669","inReplyTo":"cover.1633729502.git.me@ttaylorr.com","subject":"[PATCH 1/4] midx.c: extract MIDX lookup by object_dir","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-08T21:46:29Z","receivedAt":"2021-10-08T21:46:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The first thing that write_midx_internal() does is load the MIDX\ncorresponding to the given object directory, if one is present.\n\nPrepare for other functions in midx.c to do the same thing by extracting\nthat operation out to a small helper function.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 27 +++++++++++++++++----------\n 1 file changed, 17 insertions(+), 10 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 18a4f25aa3..b66b75a3cd 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1118,6 +1118,22 @@ static int write_midx_bitmap(char *midx_name, unsigned char *midx_hash,\n \treturn ret;\n }\n \n+static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,\n+\t\t\t\t\t\t\tconst char *object_dir)\n+{\n+\tstruct multi_pack_index *cur;\n+\n+\t/* Ensure the given object_dir is local, or a known alternate. */\n+\tfind_odb(r, object_dir);\n+\n+\tfor (cur = get_multi_pack_index(r); cur; cur = cur->next) {\n+\t\tif (!strcmp(object_dir, cur->object_dir))\n+\t\t\treturn cur;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n static int write_midx_internal(const char *object_dir,\n \t\t\t       struct string_list *packs_to_include,\n \t\t\t       struct string_list *packs_to_drop,\n@@ -1131,15 +1147,11 @@ static int write_midx_internal(const char *object_dir,\n \tstruct hashfile *f = NULL;\n \tstruct lock_file lk;\n \tstruct write_midx_context ctx = { 0 };\n-\tstruct multi_pack_index *cur;\n \tint pack_name_concat_len = 0;\n \tint dropped_packs = 0;\n \tint result = 0;\n \tstruct chunkfile *cf;\n \n-\t/* Ensure the given object_dir is local, or a known alternate. */\n-\tfind_odb(the_repository, object_dir);\n-\n \tmidx_name = get_midx_filename(object_dir);\n \tif (safe_create_leading_directories(midx_name))\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n@@ -1151,12 +1163,7 @@ static int write_midx_internal(const char *object_dir,\n \t\t * packs to include, since all packs and objects are copied\n \t\t * blindly from an existing MIDX if one is present.\n \t\t */\n-\t\tfor (cur = get_multi_pack_index(the_repository); cur; cur = cur->next) {\n-\t\t\tif (!strcmp(object_dir, cur->object_dir)) {\n-\t\t\t\tctx.m = cur;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t}\n+\t\tctx.m = lookup_multi_pack_index(the_repository, object_dir);\n \t}\n \n \tif (ctx.m && !midx_checksum_valid(ctx.m)) {\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"438365","messageId":"84e95aacbdfb092082d0ca467892552982134774.1633729502.git.me@ttaylorr.com","threadId":"56669","inReplyTo":"cover.1633729502.git.me@ttaylorr.com","subject":"[PATCH 2/4] midx.c: lookup MIDX by object directory during expire","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-08T21:46:32Z","receivedAt":"2021-10-08T21:46:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Before a new MIDX can be written, expire_midx_packs() first loads the\nexisting MIDX, figures out which packs can be expired, and then writes a\nnew MIDX based on that information.\n\nIn order to load the existing MIDX, it uses load_multi_pack_index(),\nwhich mmaps the multi-pack-index file, but does not store the resulting\n`struct multi_pack_index *` in the object store.\n\nwrite_midx_internal() also needs to open the existing MIDX, and it does\nso by iterating the results of get_multi_pack_index(), so that it reuses\nthe same pointer held by the object store. But before it can move the\nnew MIDX into place, it close_object_store() to munmap() the\nmulti-pack-index file to accommodate platforms like Windows which don't\nallow overwriting files which are memory mapped.\n\nThat's where things get weird. Since expire_midx_packs has its own\n*separate* memory mapped copy of the MIDX, the MIDX file is still memory\nmapped! Interestingly, this doesn't seem to cause a problem in our\ntests. (I believe that this has much more to do with my own lack of\nfamiliarity with Windows than it does a lack of coverage in our tests).\n\nIn any case, we can side-step the whole issue by teaching\nexpire_midx_packs() to use the `struct multi_pack_index` pointer it\nfound via the object store instead of maintain its own copy. That way,\nwhen write_midx_internal() calls `close_object_store()`, we know that\nthere are no memory mapped copies of the MIDX laying around.\n\nA couple of other small notes about this patch:\n\n  - As far as I can tell, passing `local == 1` to the call to\n    load_multi_pack_index() was an error, since object_dir could be an\n    alternate. But it doesn't matter, since even though we write\n    `m->local = 1`, we never read that field back later on.\n\n  - Setting `m = NULL` after write_midx_internal() was likely to prevent\n    a double-free back from when that function took a `struct\n    multi_pack_index *` that it called close_midx() on itself. We can\n    rely on write_midx_internal() to call that for us now.\n\nFinally, this enforces the same \"the value of --object-dir must be the\nlocal object store, or an alternate\" rule from f57a739691 (midx: avoid\nopening multiple MIDXs when writing, 2021-09-01) to the `expire`\nsub-command, too.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\nThis does leak the MIDX write_midx_internal returns before calling\nclose_object_store(). We can't just blindly call close_object_store()\nhere, either, since it's susceptible to double-frees. I'll think about\nimproving this in a separate series.\n\n midx.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex b66b75a3cd..7f1addf4b6 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1707,7 +1707,7 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n {\n \tuint32_t i, *count, result = 0;\n \tstruct string_list packs_to_drop = STRING_LIST_INIT_DUP;\n-\tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n+\tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n \tstruct progress *progress = NULL;\n\n \tif (!m)\n@@ -1752,12 +1752,11 @@ int expire_midx_packs(struct repository *r, const char *object_dir, unsigned fla\n\n \tfree(count);\n\n-\tif (packs_to_drop.nr) {\n+\tif (packs_to_drop.nr)\n \t\tresult = write_midx_internal(object_dir, NULL, &packs_to_drop, NULL, NULL, flags);\n-\t\tm = NULL;\n-\t}\n\n \tstring_list_clear(&packs_to_drop, 0);\n+\n \treturn result;\n }\n\n--\n2.33.0.96.g73915697e6\n\n"},{"id":"438366","messageId":"0accab9139e095c10bf6f299b77bab048e2b4c77.1633729502.git.me@ttaylorr.com","threadId":"56669","inReplyTo":"cover.1633729502.git.me@ttaylorr.com","subject":"[PATCH 3/4] midx.c: lookup MIDX by object directory during repack","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-08T21:46:35Z","receivedAt":"2021-10-08T21:46:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Apply similar treatment as in the last commit to the MIDX `repack`\noperation.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 7f1addf4b6..137af3fc67 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1872,7 +1872,7 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tFILE *cmd_in;\n \tstruct strbuf base_name = STRBUF_INIT;\n-\tstruct multi_pack_index *m = load_multi_pack_index(object_dir, 1);\n+\tstruct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);\n \n \t/*\n \t * When updating the default for these configuration\n@@ -1944,11 +1944,8 @@ int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,\n \t}\n \n \tresult = write_midx_internal(object_dir, NULL, NULL, NULL, NULL, flags);\n-\tm = NULL;\n \n cleanup:\n-\tif (m)\n-\t\tclose_midx(m);\n \tfree(include_pack);\n \treturn result;\n }\n-- \n2.33.0.96.g73915697e6\n\n"},{"id":"438367","messageId":"ed7407cefa89b973d1085973f4ddb39397597036.1633729502.git.me@ttaylorr.com","threadId":"56669","inReplyTo":"cover.1633729502.git.me@ttaylorr.com","subject":"[PATCH 4/4] midx.c: guard against commit_lock_file() failures","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-10-08T21:46:38Z","receivedAt":"2021-10-08T21:46:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"When writing a MIDX, we atomically move the new MIDX into place via\ncommit_lock_file(), but do not check to see if that call was successful.\n\nMake sure that we do check in order to prevent us from incorrectly\nreporting that we wrote a new MIDX if we actually encountered an error.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n midx.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 137af3fc67..e79fb4427d 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir,\n \tif (ctx.m)\n \t\tclose_object_store(the_repository->objects);\n \n-\tcommit_lock_file(&lk);\n+\tif (commit_lock_file(&lk) < 0)\n+\t\tdie_errno(_(\"could not write multi-pack-index\"));\n \n \tclear_midx_files_ext(object_dir, \".bitmap\", midx_hash);\n \tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n-- \n2.33.0.96.g73915697e6\n"},{"id":"438388","messageId":"875yu7lzkp.fsf@evledraar.gmail.com","threadId":"56669","inReplyTo":"ed7407cefa89b973d1085973f4ddb39397597036.1633729502.git.me@ttaylorr.com","subject":"Re: [PATCH 4/4] midx.c: guard against commit_lock_file() failures","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-09T02:07:16Z","receivedAt":"2021-10-09T02:08:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 08 2021, Taylor Blau wrote:\n\n> When writing a MIDX, we atomically move the new MIDX into place via\n> commit_lock_file(), but do not check to see if that call was successful.\n>\n> Make sure that we do check in order to prevent us from incorrectly\n> reporting that we wrote a new MIDX if we actually encountered an error.\n>\n>\n> Signed-off-by: Taylor Blau <me@ttaylorr.com>\n> ---\n>  midx.c | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/midx.c b/midx.c\n> index 137af3fc67..e79fb4427d 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1434,7 +1434,8 @@ static int write_midx_internal(const char *object_dir,\n>  \tif (ctx.m)\n>  \t\tclose_object_store(the_repository->objects);\n>  \n> -\tcommit_lock_file(&lk);\n> +\tif (commit_lock_file(&lk) < 0)\n> +\t\tdie_errno(_(\"could not write multi-pack-index\"));\n>  \n>  \tclear_midx_files_ext(object_dir, \".bitmap\", midx_hash);\n>  \tclear_midx_files_ext(object_dir, \".rev\", midx_hash);\n\nAs an aside I've wondered with this API whether something like this\nwouldn't make sense (obviously with a better message):\n\ndiff --git a/lockfile.c b/lockfile.c\nindex cc9a4b84283..6632a634f00 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -175,6 +175,7 @@ int hold_lock_file_for_update_timeout_mode(struct lock_file *lk,\n \t\t\t\t\t   long timeout_ms, int mode)\n {\n \tint fd = lock_file_timeout(lk, path, flags, timeout_ms, mode);\n+\tlk->flags = flags;\n \tif (fd < 0) {\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\tunable_to_lock_die(path, errno);\n@@ -209,6 +210,8 @@ int commit_lock_file(struct lock_file *lk)\n \t\tint save_errno = errno;\n \t\tfree(result_path);\n \t\terrno = save_errno;\n+\t\tif (lk->flags & LOCK_DIE_ON_ERROR)\n+\t\t\tdie_errno(\"noes\");\n \t\treturn -1;\n \t}\n \tfree(result_path);\ndiff --git a/lockfile.h b/lockfile.h\nindex db93e6ba73e..40a9d91a6c5 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -119,6 +119,7 @@\n \n struct lock_file {\n \tstruct tempfile *tempfile;\n+\tint flags;\n };\n \n #define LOCK_INIT { NULL }\n\nBut a quick grep of the users reveals that some die on lock failure, but\nare happy to ignore commit_lock_file() failures, or maybe those are also\nbugs similar to the one you're fixing here & we could fix the API more\ngenerally.\n"},{"id":"438655","messageId":"9203050e-7636-6dba-c3a3-0e5b3ca23e29@gmail.com","threadId":"56669","inReplyTo":"84e95aacbdfb092082d0ca467892552982134774.1633729502.git.me@ttaylorr.com","subject":"Re: [PATCH 2/4] midx.c: lookup MIDX by object directory during expire","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-13T13:28:33Z","receivedAt":"2021-10-13T13:28:36Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/8/2021 5:46 PM, Taylor Blau wrote:\n> Before a new MIDX can be written, expire_midx_packs() first loads the\n> existing MIDX, figures out which packs can be expired, and then writes a\n> new MIDX based on that information.\n> \n> In order to load the existing MIDX, it uses load_multi_pack_index(),\n> which mmaps the multi-pack-index file, but does not store the resulting\n> `struct multi_pack_index *` in the object store.\n> \n> write_midx_internal() also needs to open the existing MIDX, and it does\n> so by iterating the results of get_multi_pack_index(), so that it reuses\n> the same pointer held by the object store. But before it can move the\n> new MIDX into place, it close_object_store() to munmap() the\n> multi-pack-index file to accommodate platforms like Windows which don't\n> allow overwriting files which are memory mapped.\n> \n> That's where things get weird. Since expire_midx_packs has its own\n> *separate* memory mapped copy of the MIDX, the MIDX file is still memory\n> mapped! Interestingly, this doesn't seem to cause a problem in our\n> tests. (I believe that this has much more to do with my own lack of\n> familiarity with Windows than it does a lack of coverage in our tests).\n\nYou are fixing a bug in two ways:\n\n1. This change ensures we use the 'struct multi_pack_index' that exists\n   in the object store, ensuring it is closed before committing the lockfile.\n\n2. Without this change, the change in patch 4 would cause the 'expire' tests\n   to start failing on Windows, because the commands would die().\n\nIf our tests also verified that the .git/objects/pack/multi-pack-index.lock\nfile was missing at the end of the command, then we would have caught this\nbug on Windows. I don't think that's a reasonable test, but instead we(I)\nshould have used the API correctly from the start.\n\nThe tests _do_ verify that the expired packs are deleted, but the new MIDX\nprobably refers to the old packs still. Since those packs are not actually\nused (the necessary condition for expiring them), later Git commands do not\nattempt to load them and hence do not fail. That is, until we try to expire\nagain and likely get warnings about missing pack-files.\n\nThanks,\n-Stolee\n"},{"id":"438656","messageId":"42a8d2ef-3a67-ca2c-4334-b79975d45da7@gmail.com","threadId":"56669","inReplyTo":"cover.1633729502.git.me@ttaylorr.com","subject":"Re: [PATCH 0/4] midx: avoid potential rename-while-mapped on Windows","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-13T13:29:58Z","receivedAt":"2021-10-13T13:30:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/8/2021 5:46 PM, Taylor Blau wrote:\n> (This topic is based on tb/repack-write-midx).\n> \n> While looking through some failures in t5319 when Git is compiled with\n> SANITIZE=leak, I noticed a curious pattern that basically looks like:\n> \n>     struct multi_pack_index *m = load_multi_pack_index(object_dir, 0);\n>     /* ... */\n>     write_midx_internal();\n> \n> This can cause problems on platforms where a memory mapped file cannot be\n> overridden via a rename, because load_multi_pack_index() builds a new MIDX from\n> scratch each time it is called without adding that MIDX to the object store. But\n> write_midx_internal() loads a MIDX from the object store, causing the same\n> region to be mapped twice.\n> \n> The details are fully spelled out in the second patch, but the gist is that\n> while write_midx_internal() unmaps *its* copy of the MIDX, the caller's copy is\n> still mapped in memory.\n> \n> Strangely, I could not get this to produce a failure on Windows, even though I\n> would have expected one. I asked around off-list, and it sounds like it's\n> possible this behavior has changed, or Windows' mmap-equivalent may not lock\n> files that fit within a single page.\n\nThat's a good possibility. Once everything is loaded into memory, there is no\nneed for a file handle.\n \n> In any case, we let's side step the issue entirely by only interacting with\n> MIDXs that can be found from the object store.\n\nI think this is a good strategy, and the patch series goes above and beyond\nthis one failure by making it easier to get the MIDX from a specific alternate.\n\nThanks,\n-Stolee\n"}]}