{"thread":{"id":"65144","subject":"memory leak when cloning a repository","startedAt":"2026-03-05T20:51:26Z","lastAt":"2026-03-10T12:23:59Z","messageCount":25,"participants":["Jacob Keller","Jeff King","Ramsay Jones","Junio C Hamano","Patrick Steinhardt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"537998","messageId":"b9fa930e-7d5e-47f1-8896-1997cf7c0cdb@intel.com","threadId":"65144","inReplyTo":null,"subject":"memory leak when cloning a repository","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-03-05T20:51:17Z","receivedAt":"2026-03-05T20:51:26Z","isPatch":false,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"Hi,\n\nI recently ran into a memory leak that appears consistently when I clone\na repository:\n\n=================================================================\n==581989==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 27168 byte(s) in 1 object(s) allocated from:\n    #0 0x7f0e100e6f2b in malloc (/lib64/libasan.so.8+0xe6f2b) (BuildId: 25975f766867e9e604dc5a71a8befeaed3301942)\n    #1 0x00000122ab77 in git_mmap ../compat/mmap.c:15\n    #2 0x000001169466 in xmmap_gently ../wrapper.c:884\n    #3 0x00000116959b in xmmap ../wrapper.c:907\n    #4 0x000000d168fd in check_packed_git_idx ../packfile.c:179\n    #5 0x000000d16cce in open_pack_index ../packfile.c:282\n    #6 0x000000d25273 in find_pack_entry_one ../packfile.c:2078\n    #7 0x00000099f969 in check_connected ../connected.c:148\n    #8 0x0000004dabdb in update_remote_refs ../builtin/clone.c:550\n    #9 0x0000004dabdb in cmd_clone ../builtin/clone.c:1602\n    #10 0x00000080a9b4 in run_builtin ../git.c:506\n    #11 0x00000080a9b4 in handle_builtin ../git.c:780\n    #12 0x000000810727 in run_argv ../git.c:863\n    #13 0x000000810727 in cmd_main ../git.c:985\n    #14 0x000000449e6f in main ../common-main.c:9\n    #15 0x7f0e0f6105b4 in __libc_start_call_main (/lib64/libc.so.6+0x35b4) (BuildId: ff0267465bc3d76e21003b3bc5598fd5ee63e261)\n    #16 0x7f0e0f610667 in __libc_start_main@@GLIBC_2.34 (/lib64/libc.so.6+0x3667) (BuildId: ff0267465bc3d76e21003b3bc5598fd5ee63e261)\n    #17 0x00000044c264 in _start (/home/jekeller/libexec/git-core/git+0x44c264) (BuildId: f75e04052d9435ea15ebf4480b490fe2eb150d92)\n\nSUMMARY: AddressSanitizer: 27168 byte(s) leaked in 1 allocation(s).\n\nI tried digging into why this leak occurs but so far I don't have a good idea.\n\nThis happens when running on next: 7842e34a6654 (\"Sync with 'master'\")\n\nThanks,\nJake\n"},{"id":"538002","messageId":"20260305220214.GB736322@coredump.intra.peff.net","threadId":"65144","inReplyTo":"b9fa930e-7d5e-47f1-8896-1997cf7c0cdb@intel.com","subject":"Re: memory leak when cloning a repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T22:02:14Z","receivedAt":"2026-03-05T22:02:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 05, 2026 at 12:51:17PM -0800, Jacob Keller wrote:\n\n> I tried digging into why this leak occurs but so far I don't have a good idea.\n> \n> This happens when running on next: 7842e34a6654 (\"Sync with 'master'\")\n\nI can reproduce it on master. This seems to fix it:\n\ndiff --git a/connected.c b/connected.c\nindex 79403108dd..e0f8ff38cb 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -90,6 +90,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n promisor_pack_found:\n \t\t\t;\n \t\t} while ((oid = fn(cb_data)) != NULL);\n+\t\tclose_pack(new_pack);\n \t\tfree(new_pack);\n \t\treturn 0;\n \t}\n@@ -128,6 +129,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\trev_list.no_stderr = opt->quiet;\n \n \tif (start_command(&rev_list)) {\n+\t\tclose_pack(new_pack);\n \t\tfree(new_pack);\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n \t}\n@@ -162,6 +164,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\terr = error_errno(_(\"failed to close rev-list's stdin\"));\n \n \tsigchain_pop(SIGPIPE);\n+\tclose_pack(new_pack);\n \tfree(new_pack);\n \treturn finish_command(&rev_list) || err;\n }\n\n\nI think this has been leaky forever, but it's usually leaking a single\nmmap, so nobody notices. But I noticed something odd about your trace:\n\n> Direct leak of 27168 byte(s) in 1 object(s) allocated from:\n>     #0 0x7f0e100e6f2b in malloc (/lib64/libasan.so.8+0xe6f2b) (BuildId: 25975f766867e9e604dc5a71a8befeaed3301942)\n>     #1 0x00000122ab77 in git_mmap ../compat/mmap.c:15\n>     #2 0x000001169466 in xmmap_gently ../wrapper.c:884\n>     #3 0x00000116959b in xmmap ../wrapper.c:907\n>     #4 0x000000d168fd in check_packed_git_idx ../packfile.c:179\n>     #5 0x000000d16cce in open_pack_index ../packfile.c:282\n>     #6 0x000000d25273 in find_pack_entry_one ../packfile.c:2078\n>     #7 0x00000099f969 in check_connected ../connected.c:148\n\nWe're in the compat git_mmap, which implies you're building with\nNO_MMAP. We turn that on automatically when building with ASan (so that\nwe can detect single-byte overflows even when mmap would round up to a\npage boundary). But as a side effect, the \"mmap\" for index and pack data\nis done with a heap-allocated buffer. So now ASan/LSan will notice and\ncomplain about it.\n\nWe usually disable leak-checking for our ASan builds, so we wouldn't run\nthe tests with the compat mmap. And our leak-checking builds use LSan,\nwhich doesn't set NO_MMAP. But if you combine them with:\n\n  make SANITIZE=address,leak\n\nor even just build with:\n\n  make NO_MMAP=MallocHarder SANITIZE=leak\n\nthen the leak will be reported. I guess maybe you're building with\nSANITIZE=address, but then running the result independently, without\nsetting ASAN_OPTIONS=detect_leaks=0.\n\nAnyway, I think the solution is probably something like the patch above,\nthough probably it needs to cover the case where new_pack is NULL.\n\n-Peff\n"},{"id":"538012","messageId":"20260305230315.GA2354983@coredump.intra.peff.net","threadId":"65144","inReplyTo":"20260305220214.GB736322@coredump.intra.peff.net","subject":"[PATCH 0/4] plugging some mmap() leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T23:03:15Z","receivedAt":"2026-03-05T23:03:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 05, 2026 at 05:02:14PM -0500, Jeff King wrote:\n\n> Anyway, I think the solution is probably something like the patch above,\n> though probably it needs to cover the case where new_pack is NULL.\n\nSo here is a more polished version. I decided to try running the whole\ntest suite with leak-checking and NO_MMAP, and it turned up one other\ncase. This series fixes that, too, and then turns on the flag for all\nleak-checking builds.\n\n  [1/4]: check_connected(): delay opening new_pack\n  [2/4]: check_connected(): fix leak of pack-index mmap\n  [3/4]: pack-revindex: avoid double-loading .rev files\n  [4/4]: Makefile: turn on NO_MMAP when building with LSan\n\n Makefile        |  1 +\n connected.c     | 38 +++++++++++++++++++-------------------\n pack-revindex.c |  4 ++++\n 3 files changed, 24 insertions(+), 19 deletions(-)\n\n-Peff\n"},{"id":"538014","messageId":"20260305230854.GA2901305@coredump.intra.peff.net","threadId":"65144","inReplyTo":"20260305230315.GA2354983@coredump.intra.peff.net","subject":"[PATCH 1/4] check_connected(): delay opening new_pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T23:08:54Z","receivedAt":"2026-03-05T23:08:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In check_connected(), if the transport tells us we got a single packfile\nthat has already been verified as self-contained and connected, then we\ncan skip checking connectivity for any tips that are mentioned in that\npack. This goes back to c6807a40dc (clone: open a shortcut for\nconnectivity check, 2013-05-26).\n\nWe don't need to open that pack until we are about to start sending oids\nto our child rev-list process, since that's when we check whether they\nare in the self-contained pack. Let's push the opening of that pack\nfurther down in the function. That saves us from having to clean it up\nwhen we leave the function early (and by the time have opened the\nrev-list process, we never leave the function early, since we have to\nclean up the child process).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOne thing I noticed here is that for a clone with a single\nself-contained pack, we could probably skip running rev-list entirely. I\ndon't know if it matters much, though, as a noop rev-list process is not\nthat expensive compared to the cost of a clone. And in the worst case,\nit would involve calling find_pack_entry() on each proposed ref tip an\nextra time only to find that at least one does need to be sent. Though\nthat is also not very expensive.\n\nI left it out of this series, though it would involve moving the\nnew_pack opening up above the start_command() invocation again.\n\nI also wondered if this whole thing out to be written to avoid a one-off \npacked_git in the first place, like:\n\n  - call reprepare_packed_git() to re-scan objects/pack\n\n  - find the pack by name in the packed_git list\n\n  - don't clean it up; it's owned by the repository struct now\n\nBut that's a somewhat bigger change, and I'm not sure it really buys us\nthat much.\n\n connected.c | 33 +++++++++++++++------------------\n 1 file changed, 15 insertions(+), 18 deletions(-)\n\ndiff --git a/connected.c b/connected.c\nindex 79403108dd..530357de54 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\treturn err;\n \t}\n \n-\tif (transport && transport->smart_options &&\n-\t    transport->smart_options->self_contained_and_connected &&\n-\t    transport->pack_lockfiles.nr == 1 &&\n-\t    strip_suffix(transport->pack_lockfiles.items[0].string,\n-\t\t\t \".keep\", &base_len)) {\n-\t\tstruct strbuf idx_file = STRBUF_INIT;\n-\t\tstrbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n-\t\t\t   base_len);\n-\t\tstrbuf_addstr(&idx_file, \".idx\");\n-\t\tnew_pack = add_packed_git(the_repository, idx_file.buf,\n-\t\t\t\t\t  idx_file.len, 1);\n-\t\tstrbuf_release(&idx_file);\n-\t}\n-\n \tif (repo_has_promisor_remote(the_repository)) {\n \t\t/*\n \t\t * For partial clones, we don't want to have to do a regular\n@@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n promisor_pack_found:\n \t\t\t;\n \t\t} while ((oid = fn(cb_data)) != NULL);\n-\t\tfree(new_pack);\n \t\treturn 0;\n \t}\n \n@@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \telse\n \t\trev_list.no_stderr = opt->quiet;\n \n-\tif (start_command(&rev_list)) {\n-\t\tfree(new_pack);\n+\tif (start_command(&rev_list))\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n-\t}\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\n \n \trev_list_in = xfdopen(rev_list.in, \"w\");\n \n+\tif (transport && transport->smart_options &&\n+\t    transport->smart_options->self_contained_and_connected &&\n+\t    transport->pack_lockfiles.nr == 1 &&\n+\t    strip_suffix(transport->pack_lockfiles.items[0].string,\n+\t\t\t \".keep\", &base_len)) {\n+\t\tstruct strbuf idx_file = STRBUF_INIT;\n+\t\tstrbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n+\t\t\t   base_len);\n+\t\tstrbuf_addstr(&idx_file, \".idx\");\n+\t\tnew_pack = add_packed_git(the_repository, idx_file.buf,\n+\t\t\t\t\t  idx_file.len, 1);\n+\t\tstrbuf_release(&idx_file);\n+\t}\n+\n \tdo {\n \t\t/*\n \t\t * If index-pack already checked that:\n-- \n2.53.0.786.g466665faa3\n\n"},{"id":"538015","messageId":"20260305230956.GB2901305@coredump.intra.peff.net","threadId":"65144","inReplyTo":"20260305230315.GA2354983@coredump.intra.peff.net","subject":"[PATCH 2/4] check_connected(): fix leak of pack-index mmap","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T23:09:56Z","receivedAt":"2026-03-05T23:09:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since c6807a40dc (clone: open a shortcut for connectivity check,\n2013-05-26), we may open a one-off packed_git struct to check what's in\nthe pack we just received. At the end of the function we throw away the\nstruct (rather than linking it into the repository struct as usual).\n\nWe used to leak the struct until dd4143e7bf (connected.c: free the\n\"struct packed_git\", 2022-11-08), which calls free(). But that's not\nsufficient; inside the struct we'll have mmap'd the pack idx data from\ndisk, which needs an munmap() call.\n\nBuilding with SANITIZE=leak doesn't detect this, because we are leaking\nour own mmap(), and it only finds heap allocations from malloc(). But if\nwe use our compat mmap implementation like this:\n\n  make NO_MMAP=MapsBecomeMallocs SANITIZE=leak\n\nthen LSan will notice the leak, because now it's a regular heap buffer\nallocated by malloc().\n\nWe can fix it by calling close_pack(), which will free any associated\nmemory. Note that we need to check for NULL ourselves; unlike free(), it\nis not safe to pass a NULL pointer to close_pack().\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/connected.c b/connected.c\nindex 530357de54..6718503649 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -159,6 +159,9 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\terr = error_errno(_(\"failed to close rev-list's stdin\"));\n \n \tsigchain_pop(SIGPIPE);\n-\tfree(new_pack);\n+\tif (new_pack) {\n+\t\tclose_pack(new_pack);\n+\t\tfree(new_pack);\n+\t}\n \treturn finish_command(&rev_list) || err;\n }\n-- \n2.53.0.786.g466665faa3\n\n"},{"id":"538017","messageId":"20260305231229.GC2901305@coredump.intra.peff.net","threadId":"65144","inReplyTo":"20260305230315.GA2354983@coredump.intra.peff.net","subject":"[PATCH 3/4] pack-revindex: avoid double-loading .rev files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T23:12:29Z","receivedAt":"2026-03-05T23:12:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The usual entry point for loading the pack revindex is the\nload_pack_revindex() function. It returns immediately if the packed_git\nhas a non-NULL revindex or revindex data field (representing an\nin-memory or mmap'd .rev file, respectively), since the data is already\nloaded.\n\nBut in 5a6072f631 (fsck: validate .rev file header, 2023-04-17) the fsck\ncode path switched to calling load_pack_revindex_from_disk() directly,\nsince it wants to check the on-disk data (if there is any). But that\nfunction does _not_ check to see if the data has already been loaded; it\njust maps the file, overwriting the revindex_map pointer (and pointing\nrevindex_data inside that map). And in that case we've leaked the mmap()\npointed to by revindex_map (if it was non-NULL).\n\nThis usually doesn't happen, since fsck wouldn't need to load the\nrevindex for any reason before we get to these checks. But there are\nsome cases where it does. For example, is_promisor_object() runs\nodb_for_each_object() with the PACK_ORDER flag, which uses the revindex.\n\nThis happens a few times in our test suite, but SANITIZE=leak doesn't\ndetect it because we are leaking an mmap(), not a heap-allocated buffer\nfrom malloc(). However, if you build with NO_MMAP, then our compat mmap\nwill read into a heap buffer instead, and LSan will complain. This\ncauses failures in t5601, t0410, t5702, and t5616.\n\nWe can fix it by checking for existing revindex_data when loading from\ndisk. This is redundant when we're called from load_pack_revindex(), but\nit's a cheap check. The alternative is to teach check_pack_rev_indexes()\nin fsck to skip the load, but that seems messier; it doesn't otherwise\nknow about internals like revindex_map and revindex_data.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n pack-revindex.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/pack-revindex.c b/pack-revindex.c\nindex 56cd803a67..1fe0afe899 100644\n--- a/pack-revindex.c\n+++ b/pack-revindex.c\n@@ -277,6 +277,10 @@ int load_pack_revindex_from_disk(struct packed_git *p)\n {\n \tchar *revindex_name;\n \tint ret;\n+\n+\tif (p->revindex_data)\n+\t\treturn 0;\n+\n \tif (open_pack_index(p))\n \t\treturn -1;\n \n-- \n2.53.0.786.g466665faa3\n\n"},{"id":"538018","messageId":"20260305231305.GD2901305@coredump.intra.peff.net","threadId":"65144","inReplyTo":"20260305230315.GA2354983@coredump.intra.peff.net","subject":"[PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-05T23:13:05Z","receivedAt":"2026-03-05T23:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The past few commits fixed some cases where we leak memory allocated by\nmmap(). Building with SANITIZE=leak doesn't detect these because it\ncovers only heap buffers allocated by malloc().\n\nBut if we build with NO_MMAP, our compat mmap() implementation will\nallocate a heap buffer and pread() into it. And thus Lsan will detect\nthese leaks for free.\n\nUsing NO_MMAP is less performant, of course, since we have to use extra\nmemory and read in the whole file, rather than faulting in pages from\ndisk. But LSan builds are already slow, and this doesn't make them\nmeasurably worse. Getting extra coverage for our leak-checking is worth\nit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/Makefile b/Makefile\nindex f3264d0a37..4cf1afd395 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n endif\n ifneq ($(filter leak,$(SANITIZERS)),)\n BASIC_CFLAGS += -O0\n+NO_MMAP = CatchMapLeaks\n SANITIZE_LEAK = YesCompiledWithIt\n endif\n ifneq ($(filter address,$(SANITIZERS)),)\n-- \n2.53.0.786.g466665faa3\n"},{"id":"538020","messageId":"CA+P7+xp7HTykrBdr8WKb__M3Hj09-WQ6HRrTb9ZiHbWV1U=GhA@mail.gmail.com","threadId":"65144","inReplyTo":"20260305220214.GB736322@coredump.intra.peff.net","subject":"Re: memory leak when cloning a repository","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2026-03-05T23:16:35Z","receivedAt":"2026-03-05T23:16:46Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 5, 2026 at 2:02 PM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Mar 05, 2026 at 12:51:17PM -0800, Jacob Keller wrote:\n>\n> > I tried digging into why this leak occurs but so far I don't have a good idea.\n> >\n> > This happens when running on next: 7842e34a6654 (\"Sync with 'master'\")\n>\n> I can reproduce it on master. This seems to fix it:\n>\n> diff --git a/connected.c b/connected.c\n> index 79403108dd..e0f8ff38cb 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -90,6 +90,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  promisor_pack_found:\n>                         ;\n>                 } while ((oid = fn(cb_data)) != NULL);\n> +               close_pack(new_pack);\n>                 free(new_pack);\n>                 return 0;\n>         }\n> @@ -128,6 +129,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>                 rev_list.no_stderr = opt->quiet;\n>\n>         if (start_command(&rev_list)) {\n> +               close_pack(new_pack);\n>                 free(new_pack);\n>                 return error(_(\"Could not run 'git rev-list'\"));\n>         }\n> @@ -162,6 +164,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>                 err = error_errno(_(\"failed to close rev-list's stdin\"));\n>\n>         sigchain_pop(SIGPIPE);\n> +       close_pack(new_pack);\n>         free(new_pack);\n>         return finish_command(&rev_list) || err;\n>  }\n>\n>\n> I think this has been leaky forever, but it's usually leaking a single\n> mmap, so nobody notices. But I noticed something odd about your trace:\n>\n\nWow thanks for the quick response. I tried looking at this but I\nwasn't sure where it was correct to put the pack and I was having\ntrouble tracking the storage of the mmap through the compat_mmap.\n\nYea, we're leaking but its not a huge deal if the program is about to\nexit generally.\n\n> > Direct leak of 27168 byte(s) in 1 object(s) allocated from:\n> >     #0 0x7f0e100e6f2b in malloc (/lib64/libasan.so.8+0xe6f2b) (BuildId: 25975f766867e9e604dc5a71a8befeaed3301942)\n> >     #1 0x00000122ab77 in git_mmap ../compat/mmap.c:15\n> >     #2 0x000001169466 in xmmap_gently ../wrapper.c:884\n> >     #3 0x00000116959b in xmmap ../wrapper.c:907\n> >     #4 0x000000d168fd in check_packed_git_idx ../packfile.c:179\n> >     #5 0x000000d16cce in open_pack_index ../packfile.c:282\n> >     #6 0x000000d25273 in find_pack_entry_one ../packfile.c:2078\n> >     #7 0x00000099f969 in check_connected ../connected.c:148\n>\n> We're in the compat git_mmap, which implies you're building with\n> NO_MMAP. We turn that on automatically when building with ASan (so that\n> we can detect single-byte overflows even when mmap would round up to a\n> page boundary). But as a side effect, the \"mmap\" for index and pack data\n> is done with a heap-allocated buffer. So now ASan/LSan will notice and\n> complain about it.\n>\n> We usually disable leak-checking for our ASan builds, so we wouldn't run\n> the tests with the compat mmap. And our leak-checking builds use LSan,\n> which doesn't set NO_MMAP. But if you combine them with:\n>\n>   make SANITIZE=address,leak\n>\n\nRight. I built with meson and set the option to build with\n-fsanitize=address. I might have set leak too I am not certain. I was\nnot aware of the NO_MMAP.\n\n> or even just build with:\n>\n>   make NO_MMAP=MallocHarder SANITIZE=leak\n>\n> then the leak will be reported. I guess maybe you're building with\n> SANITIZE=address, but then running the result independently, without\n> setting ASAN_OPTIONS=detect_leaks=0.\n>\n\nYa, I don't have that set. The only options I have is (as of recently)\nto set LSAN_OPTIONS=exit_code=0 to avoid changing the exit code on a\nleak detection (after many hours wondering why my bash completion was\nfailing due to the leak I reported a while ago...)\n\n> Anyway, I think the solution is probably something like the patch above,\n> though probably it needs to cover the case where new_pack is NULL.\n>\n\nI can double check that later today. Its low priority, but I do think\nit is important to avoid leaks since code can be refactored into\nlibrary status over time where a leak becomes more problematic.\n\n> -Peff\n>\n"},{"id":"538021","messageId":"CA+P7+xqaCtqTwa3FTCkXyAVt0wX=EW_T1fr_u84w9Dm8XhJBow@mail.gmail.com","threadId":"65144","inReplyTo":"20260305230854.GA2901305@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] check_connected(): delay opening new_pack","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2026-03-05T23:18:12Z","receivedAt":"2026-03-05T23:18:24Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 5, 2026 at 3:08 PM Jeff King <peff@peff.net> wrote:\n>\n> In check_connected(), if the transport tells us we got a single packfile\n> that has already been verified as self-contained and connected, then we\n> can skip checking connectivity for any tips that are mentioned in that\n> pack. This goes back to c6807a40dc (clone: open a shortcut for\n> connectivity check, 2013-05-26).\n>\n> We don't need to open that pack until we are about to start sending oids\n> to our child rev-list process, since that's when we check whether they\n> are in the self-contained pack. Let's push the opening of that pack\n> further down in the function. That saves us from having to clean it up\n> when we leave the function early (and by the time have opened the\n> rev-list process, we never leave the function early, since we have to\n> clean up the child process).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> One thing I noticed here is that for a clone with a single\n> self-contained pack, we could probably skip running rev-list entirely. I\n> don't know if it matters much, though, as a noop rev-list process is not\n> that expensive compared to the cost of a clone. And in the worst case,\n> it would involve calling find_pack_entry() on each proposed ref tip an\n> extra time only to find that at least one does need to be sent. Though\n> that is also not very expensive.\n>\n> I left it out of this series, though it would involve moving the\n> new_pack opening up above the start_command() invocation again.\n>\n> I also wondered if this whole thing out to be written to avoid a one-off\n> packed_git in the first place, like:\n>\n>   - call reprepare_packed_git() to re-scan objects/pack\n>\n>   - find the pack by name in the packed_git list\n>\n>   - don't clean it up; it's owned by the repository struct now\n>\n> But that's a somewhat bigger change, and I'm not sure it really buys us\n> that much.\n\nI agree, this seems like the best low hanging fruit improvement to\navoid the unnecessary cleanup.\n\nReviewed-by: Jacob Keller <jacob.e.keller@intel.com>\n\n>\n>  connected.c | 33 +++++++++++++++------------------\n>  1 file changed, 15 insertions(+), 18 deletions(-)\n>\n> diff --git a/connected.c b/connected.c\n> index 79403108dd..530357de54 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -45,20 +45,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>                 return err;\n>         }\n>\n> -       if (transport && transport->smart_options &&\n> -           transport->smart_options->self_contained_and_connected &&\n> -           transport->pack_lockfiles.nr == 1 &&\n> -           strip_suffix(transport->pack_lockfiles.items[0].string,\n> -                        \".keep\", &base_len)) {\n> -               struct strbuf idx_file = STRBUF_INIT;\n> -               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n> -                          base_len);\n> -               strbuf_addstr(&idx_file, \".idx\");\n> -               new_pack = add_packed_git(the_repository, idx_file.buf,\n> -                                         idx_file.len, 1);\n> -               strbuf_release(&idx_file);\n> -       }\n> -\n>         if (repo_has_promisor_remote(the_repository)) {\n>                 /*\n>                  * For partial clones, we don't want to have to do a regular\n> @@ -90,7 +76,6 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  promisor_pack_found:\n>                         ;\n>                 } while ((oid = fn(cb_data)) != NULL);\n> -               free(new_pack);\n>                 return 0;\n>         }\n>\n> @@ -127,15 +112,27 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>         else\n>                 rev_list.no_stderr = opt->quiet;\n>\n> -       if (start_command(&rev_list)) {\n> -               free(new_pack);\n> +       if (start_command(&rev_list))\n>                 return error(_(\"Could not run 'git rev-list'\"));\n> -       }\n>\n>         sigchain_push(SIGPIPE, SIG_IGN);\n>\n>         rev_list_in = xfdopen(rev_list.in, \"w\");\n>\n> +       if (transport && transport->smart_options &&\n> +           transport->smart_options->self_contained_and_connected &&\n> +           transport->pack_lockfiles.nr == 1 &&\n> +           strip_suffix(transport->pack_lockfiles.items[0].string,\n> +                        \".keep\", &base_len)) {\n> +               struct strbuf idx_file = STRBUF_INIT;\n> +               strbuf_add(&idx_file, transport->pack_lockfiles.items[0].string,\n> +                          base_len);\n> +               strbuf_addstr(&idx_file, \".idx\");\n> +               new_pack = add_packed_git(the_repository, idx_file.buf,\n> +                                         idx_file.len, 1);\n> +               strbuf_release(&idx_file);\n> +       }\n> +\n>         do {\n>                 /*\n>                  * If index-pack already checked that:\n> --\n> 2.53.0.786.g466665faa3\n>\n>\n"},{"id":"538022","messageId":"CA+P7+xryAanTArTc+iVkuHGSZXWZpUYjtHuu-umi15UDzpYFRg@mail.gmail.com","threadId":"65144","inReplyTo":"20260305230956.GB2901305@coredump.intra.peff.net","subject":"Re: [PATCH 2/4] check_connected(): fix leak of pack-index mmap","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2026-03-05T23:20:11Z","receivedAt":"2026-03-05T23:20:21Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Thu, Mar 5, 2026 at 3:10 PM Jeff King <peff@peff.net> wrote:\n>\n> Since c6807a40dc (clone: open a shortcut for connectivity check,\n> 2013-05-26), we may open a one-off packed_git struct to check what's in\n> the pack we just received. At the end of the function we throw away the\n> struct (rather than linking it into the repository struct as usual).\n>\n> We used to leak the struct until dd4143e7bf (connected.c: free the\n> \"struct packed_git\", 2022-11-08), which calls free(). But that's not\n> sufficient; inside the struct we'll have mmap'd the pack idx data from\n> disk, which needs an munmap() call.\n>\n> Building with SANITIZE=leak doesn't detect this, because we are leaking\n> our own mmap(), and it only finds heap allocations from malloc(). But if\n> we use our compat mmap implementation like this:\n>\n>   make NO_MMAP=MapsBecomeMallocs SANITIZE=leak\n>\n> then LSan will notice the leak, because now it's a regular heap buffer\n> allocated by malloc().\n>\n> We can fix it by calling close_pack(), which will free any associated\n> memory. Note that we need to check for NULL ourselves; unlike free(), it\n> is not safe to pass a NULL pointer to close_pack().\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n\nReviewed-by: Jacob Keller <jacob.keller@gmail.com>\n"},{"id":"538052","messageId":"9137fd66-9ac3-42ff-a892-1b6f20b49972@ramsayjones.plus.com","threadId":"65144","inReplyTo":"20260305230315.GA2354983@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-03-06T04:37:49Z","receivedAt":"2026-03-06T04:40:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 05/03/2026 11:03 pm, Jeff King wrote:\n> On Thu, Mar 05, 2026 at 05:02:14PM -0500, Jeff King wrote:\n> \n>> Anyway, I think the solution is probably something like the patch above,\n>> though probably it needs to cover the case where new_pack is NULL.\n> \n> So here is a more polished version. I decided to try running the whole\n> test suite with leak-checking and NO_MMAP, and it turned up one other\n> case. This series fixes that, too, and then turns on the flag for all\n> leak-checking builds.\n> \n\nHmm, this gives me flash-backs. ;)\n\nMany moons ago, when the cygwin build routinely set NO_MMAP I had an\nvalgrind build of git fail with a 'double free' caused by a call to\ngit_munmap() for a pointer that had already been git_munmap-ed!\n\nIn addition, the failure was not reproducible (or at least I could not\nfind such a test). This was at a time when the testsuite took 4+ hours\nto run for a regular build, let alone a valgrind build. So, to try and\npin down the failure, I created a debug version of the mmap compat\nfunctions, which I ran with for several weeks, without failing ... :(\n\nIt just so happens that about this time I was also testing running the\ncygwin build without NO_MMAP set. This was a success, so I dropped\nthe NO_MMAP investigation, never having found the cause of the failure!\n\nI have had the 'mmap' branch, with a version of the debug patch, in my\ncygwin repo for ever (well, the 'author date' says sep 9th 2012, but I\nknow it was somewhat before then). This version of the patch removed the\n'debug' output and was only concerned with the error return behaviour of\nthe 'emulated' syscalls. (it was also somewhat non-performant if you had\nmany mmap's; luckily, that wasn't the case then, and I tended to git-gc\nvery often - which I still do to this day!)\n\nAnyway, just some food for thought. I have nearly deleted that branch\nmany times. I should probably do that now! (Hmm, patch given below just\nFYI).\n\nThanks.\n\nATB,\nRamsay Jones\n\n-------- >8 --------\nFrom 40442aa06901720ec55005144438c8c733025cbb Mon Sep 17 00:00:00 2001\nFrom: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\nDate: Sun, 9 Sep 2012 20:50:32 +0100\nSubject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug\n\nWhen compiling with the NO_MMAP build variable set, the built-in\n'git_mmap()' and 'git_munmap()' compatability routines use simple\nmemory allocation and file I/O to emulate the required behaviour.\nThe current implementation is vunerable to the \"double-delete\" bug\n(where the pointer returned by malloc() is passed to free() two or\nmore times), should the mapped memory block address be passed to\nmunmap() multiple times.\n\nIn order to guard the implementation from such a calling sequence,\nwe keep a list of mmap-block descriptors, which we then consult to\ndetermine the validity of the input pointer to munmap(). This then\nallows 'git_munmap()' to return -1 on error, as required, with\nerrno set to EINVAL.\n\nUsing a list in the log of mmap-ed blocks, along with the resulting\nlinear search, means that the performance of the code is directly\nproportional to the number of concurrently active memory mapped\nfile regions. The number of such regions is not expected to be\nexcessive.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n compat/mmap.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/mmap.c b/compat/mmap.c\nindex 7f662fef7b..137c6dc005 100644\n--- a/compat/mmap.c\n+++ b/compat/mmap.c\n@@ -1,14 +1,61 @@\n #include \"../git-compat-util.h\"\n \n+struct mmbd {  /* memory mapped block descriptor */\n+\tstruct mmbd *next;  /* next in list */\n+\tvoid   *start;      /* pointer to memory mapped block */\n+\tsize_t length;      /* length of memory mapped block */\n+};\n+\n+static struct mmbd *head;  /* head of mmb descriptor list */\n+\n+\n+static void add_desc(struct mmbd *desc, void *start, size_t length)\n+{\n+\tdesc->start = start;\n+\tdesc->length = length;\n+\tdesc->next = head;\n+\thead = desc;\n+}\n+\n+static void free_desc(struct mmbd *desc)\n+{\n+\tif (head == desc)\n+\t\thead = head->next;\n+\telse {\n+\t\tstruct mmbd *d = head;\n+\t\tfor (; d; d = d->next) {\n+\t\t\tif (d->next == desc) {\n+\t\t\t\td->next = desc->next;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t}\n+\tfree(desc);\n+}\n+\n+static struct mmbd *find_desc(void *start)\n+{\n+\tstruct mmbd *d = head;\n+\tfor (; d; d = d->next) {\n+\t\tif (d->start == start)\n+\t\t\treturn d;\n+\t}\n+\treturn NULL;\n+}\n+\n void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t offset)\n {\n \tsize_t n = 0;\n+\tstruct mmbd *desc = NULL;\n \n \tif (start != NULL || !(flags & MAP_PRIVATE))\n \t\tdie(\"Invalid usage of mmap when built with NO_MMAP\");\n \n \tstart = xmalloc(length);\n-\tif (start == NULL) {\n+\tdesc = xmalloc(sizeof(*desc));\n+\tif (!start || !desc) {\n+\t\tfree(start);\n+\t\tfree(desc);\n \t\terrno = ENOMEM;\n \t\treturn MAP_FAILED;\n \t}\n@@ -23,18 +70,26 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \n \t\tif (count < 0) {\n \t\t\tfree(start);\n+\t\t\tfree(desc);\n \t\t\terrno = EACCES;\n \t\t\treturn MAP_FAILED;\n \t\t}\n \n \t\tn += count;\n \t}\n+\tadd_desc(desc, start, length);\n \n \treturn start;\n }\n \n int git_munmap(void *start, size_t length)\n {\n+\tstruct mmbd *d = find_desc(start);\n+\tif (!d) {\n+\t\terrno = EINVAL;\n+\t\treturn -1;\n+\t}\n+\tfree_desc(d);\n \tfree(start);\n \treturn 0;\n }\n-- \n2.53.0\n\n\n"},{"id":"538065","messageId":"796110ee-d795-4445-9d82-7026370a88cf@intel.com","threadId":"65144","inReplyTo":"20260305231305.GD2901305@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2026-03-06T09:17:24Z","receivedAt":"2026-03-06T09:17:35Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On 3/5/2026 3:13 PM, Jeff King wrote:\n> The past few commits fixed some cases where we leak memory allocated by\n> mmap(). Building with SANITIZE=leak doesn't detect these because it\n> covers only heap buffers allocated by malloc().\n> \n> But if we build with NO_MMAP, our compat mmap() implementation will\n> allocate a heap buffer and pread() into it. And thus Lsan will detect\n> these leaks for free.\n> \n> Using NO_MMAP is less performant, of course, since we have to use extra\n> memory and read in the whole file, rather than faulting in pages from\n> disk. But LSan builds are already slow, and this doesn't make them\n> measurably worse. Getting extra coverage for our leak-checking is worth\n> it.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  Makefile | 1 +\n>  1 file changed, 1 insertion(+)\n> \n> diff --git a/Makefile b/Makefile\n> index f3264d0a37..4cf1afd395 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n>  endif\n>  ifneq ($(filter leak,$(SANITIZERS)),)\n>  BASIC_CFLAGS += -O0\n> +NO_MMAP = CatchMapLeaks\n>  SANITIZE_LEAK = YesCompiledWithIt\n>  endif\n>  ifneq ($(filter address,$(SANITIZERS)),)\n\nShould this patch also affect the meson.build?\n\nThere is the following in meson.build:\n\nif host_machine.system() == 'windows'\n  libgit_c_args += '-DUSE_WIN32_MMAP'\nelse\n  checkfuncs += {\n    # provided by compat/mingw.c.\n    'unsetenv' : ['unsetenv.c'],\n    # provided by compat/mingw.c.\n    'getpagesize' : [],\n  }\n\n  if get_option('b_sanitize').contains('address')\n    libgit_c_args += '-DNO_MMAP'\n    libgit_sources += 'compat/mmap.c'\n  else\n    checkfuncs += { 'mmap': ['mmap.c'] }\n  endif\nendif\n\nThis probably needs to also check if it contains leak, no?\n\nAlso I think this might be somewhat less flexible than Make since you\ncan't forcibly enable mmap even with sanitizers enabled. I suppose thats\nnot a big deal since enabling sanitizers already has a high cost.\n\nThanks,\nJake\n"},{"id":"538093","messageId":"20260306162106.GA3483423@coredump.intra.peff.net","threadId":"65144","inReplyTo":"9137fd66-9ac3-42ff-a892-1b6f20b49972@ramsayjones.plus.com","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-06T16:21:06Z","receivedAt":"2026-03-06T16:21:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 06, 2026 at 04:37:49AM +0000, Ramsay Jones wrote:\n\n> Many moons ago, when the cygwin build routinely set NO_MMAP I had an\n> valgrind build of git fail with a 'double free' caused by a call to\n> git_munmap() for a pointer that had already been git_munmap-ed!\n> \n> In addition, the failure was not reproducible (or at least I could not\n> find such a test). This was at a time when the testsuite took 4+ hours\n> to run for a regular build, let alone a valgrind build. So, to try and\n> pin down the failure, I created a debug version of the mmap compat\n> functions, which I ran with for several weeks, without failing ... :(\n> \n> It just so happens that about this time I was also testing running the\n> cygwin build without NO_MMAP set. This was a success, so I dropped\n> the NO_MMAP investigation, never having found the cause of the failure!\n\nInteresting. I guess a double-free via munmap() is probably a\nharmless-ish noop, rather than a heap corruption. I could believe we\nhave such a bug somewhere, and it may even be racy (e.g., if it requires\nreprepare_packed_git(), or maybe even has to do with stat freshness when\ndiff.c tries to reuse working tree files).\n\nWe've been testing ASan builds with NO_MMAP for a few months now, so\nit's possible that might help flush it out. Though if you ran into it in\n2012, it's possible it has since been unknowingly fixed. ;)\n\n> Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug\n> [...]\n> In order to guard the implementation from such a calling sequence,\n> we keep a list of mmap-block descriptors, which we then consult to\n> determine the validity of the input pointer to munmap(). This then\n> allows 'git_munmap()' to return -1 on error, as required, with\n> errno set to EINVAL.\n\nGross. :)\n\nThis is a clever workaround, but I think we should consider it a bug if\nwe are calling munmap() twice and fix that.\n\n-Peff\n"},{"id":"538094","messageId":"20260306162513.GB3483423@coredump.intra.peff.net","threadId":"65144","inReplyTo":"796110ee-d795-4445-9d82-7026370a88cf@intel.com","subject":"[PATCH 5/4] meson: turn on NO_MMAP when building with LSan","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-06T16:25:13Z","receivedAt":"2026-03-06T16:25:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 06, 2026 at 01:17:24AM -0800, Jacob Keller wrote:\n\n> > diff --git a/Makefile b/Makefile\n> > index f3264d0a37..4cf1afd395 100644\n> > --- a/Makefile\n> > +++ b/Makefile\n> > @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n> >  endif\n> >  ifneq ($(filter leak,$(SANITIZERS)),)\n> >  BASIC_CFLAGS += -O0\n> > +NO_MMAP = CatchMapLeaks\n> >  SANITIZE_LEAK = YesCompiledWithIt\n> >  endif\n> >  ifneq ($(filter address,$(SANITIZERS)),)\n> \n> Should this patch also affect the meson.build?\n\nUgh, yes.\n\nI don't think we use meson in the CI sanitizer builds (which is where\nI'd guess most leak-checking happens), but the two systems should remain\nconsistent.\n\nPatch below (that can go on top or be squashed into 4/4).\n\n> Also I think this might be somewhat less flexible than Make since you\n> can't forcibly enable mmap even with sanitizers enabled. I suppose thats\n> not a big deal since enabling sanitizers already has a high cost.\n\nI don't pay much attention to the meson support, but yeah, it looks like\nthere's no equivalent to tweak the NO_MMAP knob independently there. I\ndoubt anybody is clamoring for it.\n\n-- >8 --\nSubject: [PATCH] meson: turn on NO_MMAP when building with LSan\n\nThe previous commit taught the Makefile to turn on NO_MMAP in this\ninstance. We should do the same with meson for consistency. We already\ndo this for ASan builds, so we can just tweak one conditional.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nTested and confirmed this finds the test failures fixed by the earlier\npatches.\n\n meson.build | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/meson.build b/meson.build\nindex 4b536e0124..4e13afbb41 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -1426,7 +1426,7 @@ else\n     'getpagesize' : [],\n   }\n \n-  if get_option('b_sanitize').contains('address')\n+  if get_option('b_sanitize').contains('address') or get_option('b_sanitize').contains('leak')\n     libgit_c_args += '-DNO_MMAP'\n     libgit_sources += 'compat/mmap.c'\n   else\n-- \n2.53.0.791.g8baeb4ea4d\n\n"},{"id":"538096","messageId":"aa83861a-cc0f-4fc2-9599-182dac8b4e9a@ramsayjones.plus.com","threadId":"65144","inReplyTo":"20260306162106.GA3483423@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-03-06T17:49:04Z","receivedAt":"2026-03-06T17:49:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/03/2026 4:21 pm, Jeff King wrote:\n> On Fri, Mar 06, 2026 at 04:37:49AM +0000, Ramsay Jones wrote:\n> \n>> Many moons ago, when the cygwin build routinely set NO_MMAP I had an\n>> valgrind build of git fail with a 'double free' caused by a call to\n>> git_munmap() for a pointer that had already been git_munmap-ed!\n>>\n>> In addition, the failure was not reproducible (or at least I could not\n>> find such a test). This was at a time when the testsuite took 4+ hours\n>> to run for a regular build, let alone a valgrind build. So, to try and\n>> pin down the failure, I created a debug version of the mmap compat\n>> functions, which I ran with for several weeks, without failing ... :(\n>>\n>> It just so happens that about this time I was also testing running the\n>> cygwin build without NO_MMAP set. This was a success, so I dropped\n>> the NO_MMAP investigation, never having found the cause of the failure!\n> \n> Interesting. I guess a double-free via munmap() is probably a\n> harmless-ish noop, rather than a heap corruption. I could believe we\n> have such a bug somewhere, and it may even be racy (e.g., if it requires\n> reprepare_packed_git(), or maybe even has to do with stat freshness when\n> diff.c tries to reuse working tree files).\n> \n> We've been testing ASan builds with NO_MMAP for a few months now, so\n> it's possible that might help flush it out. Though if you ran into it in\n> 2012, it's possible it has since been unknowingly fixed. ;)\n\nYep, it was before 2012 and I suspect it has been 'fixed' (but could, of\ncourse, still be lingering ...). ;)\n\n>> Subject: [PATCH] mmap.c: log mmap() blocks to avoid double-delete bug\n>> [...]\n>> In order to guard the implementation from such a calling sequence,\n>> we keep a list of mmap-block descriptors, which we then consult to\n>> determine the validity of the input pointer to munmap(). This then\n>> allows 'git_munmap()' to return -1 on error, as required, with\n>> errno set to EINVAL.\n> \n> Gross. :)\n\nHeh, agreed! :) There were several reasons I didn't submit it in all these\nyears.\n\n> This is a clever workaround, but I think we should consider it a bug if\n> we are calling munmap() twice and fix that.\n\nAgreed. (I just wanted to bring this to your attention).\n\nATB,\nRamsay Jones\n\n\n\n"},{"id":"538098","messageId":"d5a22245-662b-4caa-9ed6-0e981f9e0d37@ramsayjones.plus.com","threadId":"65144","inReplyTo":"20260306162513.GB3483423@coredump.intra.peff.net","subject":"Re: [PATCH 5/4] meson: turn on NO_MMAP when building with LSan","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-03-06T18:00:50Z","receivedAt":"2026-03-06T18:00:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/03/2026 4:25 pm, Jeff King wrote:\n> On Fri, Mar 06, 2026 at 01:17:24AM -0800, Jacob Keller wrote:\n> \n[snip]\n\n>> Also I think this might be somewhat less flexible than Make since you\n>> can't forcibly enable mmap even with sanitizers enabled. I suppose thats\n>> not a big deal since enabling sanitizers already has a high cost.\n> \n> I don't pay much attention to the meson support, but yeah, it looks like\n> there's no equivalent to tweak the NO_MMAP knob independently there. I\n> doubt anybody is clamoring for it.\n\nIgnoring old Cygwin, IRIX, IRIX64, Minix, Nonstop and OS/390 set NO_MMAP\nin the config.mak.uname file. I suspect none of them are using meson to\nbuild git, so it shouldn't be an issue. (although I don't quite know why\nI suspect that!).\n\n[NOTE that NO_MMAP is not set in GIT-BUILD-OPTIONS, so ...]\n\nATB,\nRamsay Jones\n\n\n"},{"id":"538101","messageId":"xmqq5x78249v.fsf@gitster.g","threadId":"65144","inReplyTo":"9137fd66-9ac3-42ff-a892-1b6f20b49972@ramsayjones.plus.com","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-06T18:37:16Z","receivedAt":"2026-03-06T18:37:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> When compiling with the NO_MMAP build variable set, the built-in\n> 'git_mmap()' and 'git_munmap()' compatability routines use simple\n> memory allocation and file I/O to emulate the required behaviour.\n> The current implementation is vunerable to the \"double-delete\" bug\n> (where the pointer returned by malloc() is passed to free() two or\n> more times), should the mapped memory block address be passed to\n> munmap() multiple times.\n\nSorry if I am missing something glaringly obvious, but quite\nhonestly I am confused.  Wouldn't it be a bug to call munmap() again\non the same region of memory obtained from mmap() and then already\nunmapped by calling munmap()?\n\nOr can the emulation layer cause such a second free() even if the\nmunmap() is done once and only once per memory region obtained from\na single mmap()?\n\n"},{"id":"538102","messageId":"c3e66e36-cba0-49d3-b2a6-d65367f4be0f@ramsayjones.plus.com","threadId":"65144","inReplyTo":"xmqq5x78249v.fsf@gitster.g","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-03-06T18:55:18Z","receivedAt":"2026-03-06T18:55:21Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/03/2026 6:37 pm, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> When compiling with the NO_MMAP build variable set, the built-in\n>> 'git_mmap()' and 'git_munmap()' compatability routines use simple\n>> memory allocation and file I/O to emulate the required behaviour.\n>> The current implementation is vunerable to the \"double-delete\" bug\n>> (where the pointer returned by malloc() is passed to free() two or\n>> more times), should the mapped memory block address be passed to\n>> munmap() multiple times.\n> \n> Sorry if I am missing something glaringly obvious, but quite\n> honestly I am confused.  Wouldn't it be a bug to call munmap() again\n> on the same region of memory obtained from mmap() and then already\n> unmapped by calling munmap()?\n\nYes. The (second) call to munmap() with the (already unmapped) memory\nregion would return -1 with errno set to EINVAL.\n\nThe emulation layer does not detect this situation and simply calls\nfree() on the given pointer. Hence the 'double-delete' bug.\n\n> Or can the emulation layer cause such a second free() even if the\n> munmap() is done once and only once per memory region obtained from\n> a single mmap()?\n\nNo. If you only git_munmap() once for a given memory region, everything\nis fine.\n\nThanks.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"538121","messageId":"xmqqjyvoy5p2.fsf@gitster.g","threadId":"65144","inReplyTo":"c3e66e36-cba0-49d3-b2a6-d65367f4be0f@ramsayjones.plus.com","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-06T22:05:29Z","receivedAt":"2026-03-06T22:05:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> On 06/03/2026 6:37 pm, Junio C Hamano wrote:\n>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n>> \n>>> When compiling with the NO_MMAP build variable set, the built-in\n>>> 'git_mmap()' and 'git_munmap()' compatability routines use simple\n>>> memory allocation and file I/O to emulate the required behaviour.\n>>> The current implementation is vunerable to the \"double-delete\" bug\n>>> (where the pointer returned by malloc() is passed to free() two or\n>>> more times), should the mapped memory block address be passed to\n>>> munmap() multiple times.\n>> \n>> Sorry if I am missing something glaringly obvious, but quite\n>> honestly I am confused.  Wouldn't it be a bug to call munmap() again\n>> on the same region of memory obtained from mmap() and then already\n>> unmapped by calling munmap()?\n>\n> Yes. The (second) call to munmap() with the (already unmapped) memory\n> region would return -1 with errno set to EINVAL.\n>\n> The emulation layer does not detect this situation and simply calls\n> free() on the given pointer. Hence the 'double-delete' bug.\n>\n>> Or can the emulation layer cause such a second free() even if the\n>> munmap() is done once and only once per memory region obtained from\n>> a single mmap()?\n>\n> No. If you only git_munmap() once for a given memory region, everything\n> is fine.\n\nHmph.  You make it sound as if we have some code that calls munmap()\non something that we are not sure if we have unmapped just in case,\ntrusting that it won't crash us and instead give us EINVAL, and that\nis very much deliberate?  Unless we have such a code, bending over\nbackwards to track what has already been unmapped and return -1 with\nEINVAL from munmap() for a second call is of dubious value, no?  I\nstill must be missing something...\n\nThanks.\n\n"},{"id":"538137","messageId":"c3ae9ff6-8577-48de-8473-9ec8d22ebc71@ramsayjones.plus.com","threadId":"65144","inReplyTo":"xmqqjyvoy5p2.fsf@gitster.g","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2026-03-06T23:25:03Z","receivedAt":"2026-03-06T23:25:06Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 06/03/2026 10:05 pm, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> On 06/03/2026 6:37 pm, Junio C Hamano wrote:\n>>> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n>>>\n>>>> When compiling with the NO_MMAP build variable set, the built-in\n>>>> 'git_mmap()' and 'git_munmap()' compatability routines use simple\n>>>> memory allocation and file I/O to emulate the required behaviour.\n>>>> The current implementation is vunerable to the \"double-delete\" bug\n>>>> (where the pointer returned by malloc() is passed to free() two or\n>>>> more times), should the mapped memory block address be passed to\n>>>> munmap() multiple times.\n>>>\n>>> Sorry if I am missing something glaringly obvious, but quite\n>>> honestly I am confused.  Wouldn't it be a bug to call munmap() again\n>>> on the same region of memory obtained from mmap() and then already\n>>> unmapped by calling munmap()?\n>>\n>> Yes. The (second) call to munmap() with the (already unmapped) memory\n>> region would return -1 with errno set to EINVAL.\n>>\n>> The emulation layer does not detect this situation and simply calls\n>> free() on the given pointer. Hence the 'double-delete' bug.\n>>\n>>> Or can the emulation layer cause such a second free() even if the\n>>> munmap() is done once and only once per memory region obtained from\n>>> a single mmap()?\n>>\n>> No. If you only git_munmap() once for a given memory region, everything\n>> is fine.\n> \n> Hmph.  You make it sound as if we have some code that calls munmap()\n> on something that we are not sure if we have unmapped just in case,\n> trusting that it won't crash us and instead give us EINVAL, and that\n> is very much deliberate?  Unless we have such a code, bending over\n> backwards to track what has already been unmapped and return -1 with\n> EINVAL from munmap() for a second call is of dubious value, no?  I\n> still must be missing something...\n\nSorry to confuse you, I'm not trying to, honest! :)\n\nFirst, forget that patch. It was not meant to be applied by anyone to\nanything. It was just to (hopefully) support the explanation (to Jeff\nspecifically) of a bug which happened to me long ago.\n\nIn particular, valgrind demonstrated a bug, which has probably been fixed\nin the intervening years, which directly implied that some code called\nmunmap() on a memory region which had already been unmapped.\n\nGiven that this was on cygwin and, at that time, was built with NO_MMAP,\nthis meant that free() was called twice on the same malloc()ed memory.\n(Indeed this is what valgrind reported).\n\nIf you look at the (currently 31) calls to munmap(), only one seems to look\nat the return (in refs/packed-backend.c:183). So, calling munmap() twice\non the same memory region will probably go unnoticed when NO_MMAP is not\nset. I have no idea why munmap() was called twice on the same memory region,\nsince I didn't track down the code responsible.\n\nIt was just an FYI about a _potential_ lurking bug when using the mmap compat\nroutines.\n\nHave I cleared that up, or confused you more. :)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"538152","messageId":"xmqqqzpwv3t7.fsf@gitster.g","threadId":"65144","inReplyTo":"20260305231305.GD2901305@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] Makefile: turn on NO_MMAP when building with LSan","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-07T01:14:28Z","receivedAt":"2026-03-07T01:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n>  endif\n>  ifneq ($(filter leak,$(SANITIZERS)),)\n>  BASIC_CFLAGS += -O0\n> +NO_MMAP = CatchMapLeaks\n>  SANITIZE_LEAK = YesCompiledWithIt\n>  endif\n>  ifneq ($(filter address,$(SANITIZERS)),)\n\nAnd of course, this \"breaks\" the leaks job at CI without being the\ntrue culprit.\n\n    https://github.com/git/git/actions/runs/22786105918/job/66103114142\n\nMy bisection between v2.52.0 and v2.53.0 with the following\n\n    $ git bisect start v2.53.0 v2.52.0\n    $ git bisect run sh :doit\n\nwhere :doit has the shell script attached at the end of this message\nblames this commit.  I didn't dig further than that.\n\ncommit 4c89d31494bff4bde6079a0e0821f1437e37d07b\nAuthor: Patrick Steinhardt <ps@pks.im>\nDate:   Sun Nov 23 19:59:37 2025 +0100\n\n    streaming: rely on object sources to create object stream\n    \n    When creating an object stream we first look up the object info and, if\n    it's present, we call into the respective backend that contains the\n    object to create a new stream for it.\n    \n    This has the consequence that, for loose object source, we basically\n    iterate through the object sources twice: we first discover that the\n    file exists as a loose object in the first place by iterating through\n    all sources. And, once we have discovered it, we again walk through all\n    sources to try and map the object. The same issue will eventually also\n    surface once the packfile store becomes per-object-source.\n    \n    Furthermore, it feels rather pointless to first look up the object only\n    to then try and read it.\n    \n    Refactor the logic to be centered around sources instead. Instead of\n    first reading the object, we immediately ask the source to create the\n    object stream for us. If the object exists we get stream, otherwise\n    we'll try the next source.\n    \n    Like this we only have to iterate through sources once. But even more\n    importantly, this change also helps us to make the whole logic\n    pluggable. The object read stream subsystem does not need to be aware of\n    the different source backends anymore, but eventually it'll only have to\n    call the source's callback function.\n    \n    Note that at the current point in time we aren't fully there yet:\n    \n      - The packfile store still sits on the object database level and is\n        thus agnostic of the sources.\n    \n      - We still have to call into both the packfile store and the loose\n        object source.\n    \n    But both of these issues will soon be addressed.\n    \n    This refactoring results in a slight change to semantics: previously, it\n    was `odb_read_object_info_extended()` that picked the source for us, and\n    it would have favored packed (non-deltified) objects over loose objects.\n    And while we still favor packed over loose objects for a single source\n    with the new logic, we'll now favor a loose object from an earlier\n    source over a packed object from a later source.\n    \n    Ultimately this shouldn't matter though: the stream doesn't indicate to\n    the caller which source it is from and whether it was created from a\n    packed or loose object, so such details are opaque to the caller. And\n    other than that we should be able to assume that two objects with the\n    same object ID should refer to the same content, so the streamed data\n    would be the same, too.\n    \n    Signed-off-by: Patrick Steinhardt <ps@pks.im>\n    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\n streaming.c | 65 +++++++++++++++++++++++--------------------------------------\n 1 file changed, 24 insertions(+), 41 deletions(-)\n\n\n---- >8 ----\n\n#!/bin/sh\n\ngit apply -3 <\"$0\" || {\n\tgit reset --hard\n\texit 125\n}\n\n(\n\texport SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=true \n\tmake NO_MMAP=CatchMapLeaks CC=clang &&\n\tcd t && sh t1060-object-corruption.sh\n)\n\nstatus=$?\n\nmake distclean\ngit reset --hard\n\nexit $status\n\ndiff --git i/compat/mmap.c w/compat/mmap.c\nindex 2fe1c7732e..1a118711f7 100644\n--- i/compat/mmap.c\n+++ w/compat/mmap.c\n@@ -38,7 +38,7 @@ void *git_mmap(void *start, size_t length, int prot, int flags, int fd, off_t of\n \treturn start;\n }\n \n-int git_munmap(void *start, size_t length)\n+int git_munmap(void *start, size_t length UNUSED)\n {\n \tfree(start);\n \treturn 0;\n"},{"id":"538153","messageId":"xmqqms0kv3rn.fsf@gitster.g","threadId":"65144","inReplyTo":"c3ae9ff6-8577-48de-8473-9ec8d22ebc71@ramsayjones.plus.com","subject":"Re: [PATCH 0/4] plugging some mmap() leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-07T01:15:24Z","receivedAt":"2026-03-07T01:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> If you look at the (currently 31) calls to munmap(), only one seems to look\n> at the return (in refs/packed-backend.c:183). So, calling munmap() twice\n> on the same memory region will probably go unnoticed when NO_MMAP is not\n> set. I have no idea why munmap() was called twice on the same memory region,\n> since I didn't track down the code responsible.\n>\n> It was just an FYI about a _potential_ lurking bug when using the mmap compat\n> routines.\n>\n> Have I cleared that up, or confused you more. :)\n\nOh, absolutely.  Thanks.\n"},{"id":"538162","messageId":"20260307022459.GA693632@coredump.intra.peff.net","threadId":"65144","inReplyTo":"xmqqqzpwv3t7.fsf@gitster.g","subject":"[PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-03-07T02:24:59Z","receivedAt":"2026-03-07T02:25:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 06, 2026 at 05:14:28PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > @@ -1600,6 +1600,7 @@ BASIC_CFLAGS += -DSHA1DC_FORCE_ALIGNED_ACCESS\n> >  endif\n> >  ifneq ($(filter leak,$(SANITIZERS)),)\n> >  BASIC_CFLAGS += -O0\n> > +NO_MMAP = CatchMapLeaks\n> >  SANITIZE_LEAK = YesCompiledWithIt\n> >  endif\n> >  ifneq ($(filter address,$(SANITIZERS)),)\n> \n> And of course, this \"breaks\" the leaks job at CI without being the\n> true culprit.\n> \n>     https://github.com/git/git/actions/runs/22786105918/job/66103114142\n> \n> My bisection between v2.52.0 and v2.53.0 with the following\n> \n>     $ git bisect start v2.53.0 v2.52.0\n>     $ git bisect run sh :doit\n> \n> where :doit has the shell script attached at the end of this message\n> blames this commit.  I didn't dig further than that.\n\nInteresting. I ran my tests on \"master\", which would include v2.53.0,\nand it came up clean. But I use gcc locally; switching to clang does\nindeed report a leak for me.\n\nEven more curiously, if I try testing the tip of jch, then gcc does find\nthe same leak! Bisecting, it starts to find the leak as of 1f3fd68e06\n(odb/source: make `read_object_stream()` function pluggable,\n2026-03-05).\n\nThere is a real leak here; the fix is below. But curiously, it is _not_\nthe fault of the commit you found by bisection.\n\nIn the test in question, we die() shortly after the leak happens. We've\ndefinitely left the function that holds the pointer to the leaked\nbuffer, so it's a true leak. But my guess is that the leak detector\ndoesn't quite know which parts of stack memory are valid or not when we\ndie(), so it scans the whole thing looking for plausible pointers to\nallocations. If it gets \"lucky\", then the stale out-of-scope pointer is\nstill in stack memory, and we consider it still reachable.\n\nAnd whether that happens or not can depend on the compiler, or even\ncompile options. And as the code is refactored to use the more abstract\nodb API (and call more functions), it is increasingly likely that\nsomething else has re-used that bit of stack memory.\n\nSo that's why the leak \"appears\" in 4c89d31494 (streaming: rely on\nobject sources to create object stream, 2025-11-23) for clang, and\n1f3fd68e06 (odb/source: make `read_object_stream()` function pluggable,\n2026-03-05) for gcc. But it was really there all along.\n\nAnyway, here's the fix. It should probably be slotted in before patch 4\n(which turns on NO_MMAP for leak-check builds).\n\n-- >8 --\nSubject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()\n\nWe mmap() a loose object file, storing the result in the local variable\n\"mapped\", which is eventually assigned into our stream struct as\n\"st.mapped\". If we hit an error, we jump to an error label which does:\n\n  munmap(st.mapped, st.mapsize);\n\nto clean up. But this is wrong; we don't assign st.mapped until the end\nof the function, after all of the \"goto error\" jumps. So this munmap()\nis never cleaning up anything (st.mapped is always NULL, because we\ninitialize the struct with calloc).\n\nInstead, we should feed the local variable to munmap().\n\nThis leak is due to 595296e124 (streaming: allocate stream inside the\nbackend-specific logic, 2025-11-23), which introduced the local\nvariable. Before that, we assigned the mmap result directly into\nst.mapped. It was probably switched there so that we do not have to\nallocate/free the struct when the map operation fails (e.g., because we\ndon't have the loose object). Before that commit, the struct was passed\nin from the caller, so there was no allocation at all.\n\nYou can see the leak in the test suite by building with:\n\n  make SANITIZE=leak NO_MMAP=1 CC=clang\n\nand running t1060. We need NO_MMAP so that the mmap() is backed by an\nactual malloc(), which allows LSan to detect it. And the leak seems not\nto be detected when compiling with gcc, probably due to some internal\ncompiler decisions about how the stack memory is written.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 3094140055..ab2fb9c4eb 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -2197,7 +2197,7 @@ int odb_source_loose_read_object_stream(struct odb_read_stream **out,\n \treturn 0;\n error:\n \tgit_inflate_end(&st->z);\n-\tmunmap(st->mapped, st->mapsize);\n+\tmunmap(mapped, mapsize);\n \tfree(st);\n \treturn -1;\n }\n-- \n2.53.0.791.g8baeb4ea4d\n\n"},{"id":"538163","messageId":"xmqqv7f8td6b.fsf@gitster.g","threadId":"65144","inReplyTo":"20260307022459.GA693632@coredump.intra.peff.net","subject":"Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-03-07T05:35:08Z","receivedAt":"2026-03-07T05:35:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()\n>\n> We mmap() a loose object file, storing the result in the local variable\n> \"mapped\", which is eventually assigned into our stream struct as\n> \"st.mapped\". If we hit an error, we jump to an error label which does:\n>\n>   munmap(st.mapped, st.mapsize);\n>\n> to clean up. But this is wrong; we don't assign st.mapped until the end\n> of the function, after all of the \"goto error\" jumps. So this munmap()\n> is never cleaning up anything (st.mapped is always NULL, because we\n> initialize the struct with calloc).\n>\n> Instead, we should feed the local variable to munmap().\n>\n> This leak is due to 595296e124 (streaming: allocate stream inside the\n> backend-specific logic, 2025-11-23), which introduced the local\n> variable. Before that, we assigned the mmap result directly into\n> st.mapped. It was probably switched there so that we do not have to\n> allocate/free the struct when the map operation fails (e.g., because we\n> don't have the loose object). Before that commit, the struct was passed\n> in from the caller, so there was no allocation at all.\n\nMakes sense.  Thanks for finding and fixing the issue so quickly.\n\n\n>\n> You can see the leak in the test suite by building with:\n>\n>   make SANITIZE=leak NO_MMAP=1 CC=clang\n>\n> and running t1060. We need NO_MMAP so that the mmap() is backed by an\n> actual malloc(), which allows LSan to detect it. And the leak seems not\n> to be detected when compiling with gcc, probably due to some internal\n> compiler decisions about how the stack memory is written.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  object-file.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/object-file.c b/object-file.c\n> index 3094140055..ab2fb9c4eb 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -2197,7 +2197,7 @@ int odb_source_loose_read_object_stream(struct odb_read_stream **out,\n>  \treturn 0;\n>  error:\n>  \tgit_inflate_end(&st->z);\n> -\tmunmap(st->mapped, st->mapsize);\n> +\tmunmap(mapped, mapsize);\n>  \tfree(st);\n>  \treturn -1;\n>  }\n"},{"id":"538409","messageId":"abANWS_j2g-ae89b@pks.im","threadId":"65144","inReplyTo":"xmqqv7f8td6b.fsf@gitster.g","subject":"Re: [PATCH 3.5/4] object-file: fix mmap() leak in odb_source_loose_read_object_stream()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-03-10T12:23:53Z","receivedAt":"2026-03-10T12:23:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Mar 06, 2026 at 09:35:08PM -0800, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > Subject: object-file: fix mmap() leak in odb_source_loose_read_object_stream()\n> >\n> > We mmap() a loose object file, storing the result in the local variable\n> > \"mapped\", which is eventually assigned into our stream struct as\n> > \"st.mapped\". If we hit an error, we jump to an error label which does:\n> >\n> >   munmap(st.mapped, st.mapsize);\n> >\n> > to clean up. But this is wrong; we don't assign st.mapped until the end\n> > of the function, after all of the \"goto error\" jumps. So this munmap()\n> > is never cleaning up anything (st.mapped is always NULL, because we\n> > initialize the struct with calloc).\n> >\n> > Instead, we should feed the local variable to munmap().\n> >\n> > This leak is due to 595296e124 (streaming: allocate stream inside the\n> > backend-specific logic, 2025-11-23), which introduced the local\n> > variable. Before that, we assigned the mmap result directly into\n> > st.mapped. It was probably switched there so that we do not have to\n> > allocate/free the struct when the map operation fails (e.g., because we\n> > don't have the loose object). Before that commit, the struct was passed\n> > in from the caller, so there was no allocation at all.\n> \n> Makes sense.  Thanks for finding and fixing the issue so quickly.\n\nYup, indeed, this is an obvious fix. Thanks for cleaning up after me!\n\nPatrick\n"}]}