* Re: [PATCH 2/2] format-patch: fix leak of rev_info in prepare_bases()
From: Jeff King @ 2026-07-01 8:13 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <akOZy-BygZS8fqPM@pks.im>
On Tue, Jun 30, 2026 at 12:26:19PM +0200, Patrick Steinhardt wrote:
> > make SANITIZE=leak
> > cd t
> > GIT_TEST_COMMIT_GRAPH=1 ./t4014-format-patch.sh
> >
> > which yields many entries like:
> >
> > ==git==3687620==ERROR: LeakSanitizer: detected memory leaks
> > Direct leak of 200 byte(s) in 1 object(s) allocated from:
> > #0 0x7f4ccba185cb in malloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:74
> > #1 0x55cd452cdd0b in do_xmalloc wrapper.c:55
> > #2 0x55cd452cdd9d in xmalloc wrapper.c:76
> > #3 0x55cd45255473 in init_topo_walk revision.c:3845
> > #4 0x55cd45255bef in prepare_revision_walk revision.c:4017
> > #5 0x55cd44ffec40 in prepare_bases builtin/log.c:1872
> > #6 0x55cd450010ec in cmd_format_patch builtin/log.c:2439
>
> Interesting. Makes me wonder whether we should modify linux-TEST-vars to
> also run with the leak checker enabled. Ideally we'd of course just do
> this for all jobs, but the overhead is probably way too high... yes,
> doing a simple benchmark shows a ~3x hit.
>
> So this is definitely nothing we want to do for all jobs. But for the
> linux-TEST-vars job it might make sense, as it exercises a bunch of
> non-default code paths.
We already run a special leak job for linux-reftables. Why not turn that
job into "leaks plus reftables plus test-vars"? The only downside would
be potentially hiding leaks found by linux-reftables-leaks if the
test-vars features force us into a difference code path. But looking at
the list, it doesn't seem likely to me. None of them is particularly
ref-related.
In fact, I kind of wonder if we could fold linux-reftables into the
test-vars job completely.
> One thing worth noting: there are still six test suites that are failing
> with this patch: t0095, t3451, t3452, t3453, t4013 and t4211. The t345x
> failures are because of the missing call to `repo_unuse_commit_buffer()`
> in git-history(1), which we already noted elsewhere.
>
> All of the remaining leaks in t0095, t4013 and t4211 seem to be related
> to bloom filters.
I sent some patches to fix the bloom-filter cases.
Building with OPENSSL_SHA1_UNSAFE turns up more. The core issue is that
recent versions of openssl require an allocation to open a sha1 context,
and we free it in git_hash_final(). So code paths that abort mid-hash
will leak the allocation, and we need a git_hash_discard().
It comes up mostly with csum-file.[ch], since that's where we use the
unsafe variant.
If you further build with OPENSSL_SHA1 (using it for _all_ hash
computations), there are a few more cases. It's hard to care too much
since that isn't a recommended build (and we've even discussed dropping
support for non-dc sha1 totally). But sha256 has the same issue, so
we'll want to fix it eventually (I didn't try leak-checking the
linux-sha256 build, but I expect it would complain a lot).
I have some patches but they need a bit of polish. In particular I think
we'll have to tweak the hash.h #define mess to expose a "discard"
primitive from each implementation (otherwise we have to finalize the
hash to discard, which is a little inefficient). I didn't quite have the
stomach for that tonight.
-Peff
^ permalink raw reply
* Re: [PATCH] submodule absorbgitdirs tests: use test_* helper functions
From: Patrick Steinhardt @ 2026-07-01 8:04 UTC (permalink / raw)
To: Junio C Hamano
Cc: Bryan B. Lima, git, gustavoscorrea,
Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqmrwbsybn.fsf@gitster.g>
On Tue, Jun 30, 2026 at 11:00:28AM -0700, Junio C Hamano wrote:
> "Bryan B. Lima" <bblima@usp.br> writes:
>
> > Use modern helper functions from test-lib-functions.sh to provide nice error messages.
> >
> > Signed-off-by: Bryan B. Lima <bblima@usp.br>
> > Co-authored-by: Gustavo S. Correa <gustavoscorrea@usp.br>
> > Signed-off-by: Gustavo S. Correa <gustavoscorrea@usp.br>
> > ---
> > t/t7412-submodule-absorbgitdirs.sh | 22 +++++++++++-----------
> > 1 file changed, 11 insertions(+), 11 deletions(-)
>
> Welcome to the Git development community.
Welcome indeed!
> It is rare, but it happens from time to time, that we see a patch by
> somebody we haven't seen on this list, and the patch looks perfect.
> Very delighted.
Sorry to spoil it: the order of SOBs is incorrect. The patch is authored
by Bryan, so that signoff needs to come last.
The patch itself looks good though, thanks!
Patrick
^ permalink raw reply
* Re: [PATCH 06/13] line-log: avoid redundant copy that leaks in process_ranges
From: Jeff King @ 2026-07-01 8:02 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <5a6b17f075ca2d0442d512a0021557aa112860fc.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:24AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When bloom_filter_check() indicates that a commit does not touch
> any of the tracked paths, line_log_process_ranges_arbitrary_commit()
> propagates the current ranges to the parent by calling
> line_log_data_copy() and passing the copy to add_line_range().
> However, add_line_range() always makes its own copy internally
> (via line_log_data_copy or line_log_data_merge), so the caller's
> copy is never freed and leaks every time this path is taken.
>
> Pass range directly to add_line_range() instead of making a
> redundant intermediate copy. The callee's internal copy handles
> ownership correctly.
>
> Pointed out by Coverity.
Heh, I just posted the identical patch (in my case found by running the
test suite with GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS=1).
So yeah, looks good to me. :)
-Peff
^ permalink raw reply
* Re: weird quadratic reftable behavior, was: Re: [PATCH 3/3] t5551: pack refs after creating many tags
From: Jeff King @ 2026-07-01 8:00 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Michael Montalbo, git, Junio C Hamano
In-Reply-To: <akSxCUfm2P7ocLJX@pks.im>
On Wed, Jul 01, 2026 at 08:17:45AM +0200, Patrick Steinhardt wrote:
> This is a known issue, I think [1].
>
> The problem here is the tombstoning: when you delete all references,
> chances are that they are not truly gone but that every reference is
> just tombstoned. The problem with this is that reading refs may now take
> signifciantly more time as we cannot just say "this stack is empty".
> Instead, we need to figure out that it is empty by processing all the
> tombstones, and that takes a lot of time.
>
> I remember that I did some digging back then and improved the status quo
> quite significantly by optimizing `refs_verify_refname_available()`. I'm
> sure there are more opportunities for optimization here though -- I have
> a feeling that we for example exhaust the merged iterator until its end
> when searching for a specific refname, where we could easily abort once
> the observed tombstone name sorts lexicographically after the needle.
Yeah, it is (mostly) the same problem. About half the time is spent in
refs_verify_refnames_available().
The other half is in reftable_be_transaction_prepare(). Looks like it
makes individual calls to prepare_single_update(), which reads each ref.
And those reads are expensive because of all of the tombstones. It might
be possible to do an iterator merge or similar between the sorted list
of transaction refs and the reftable contents.
> But eventually I decided to not care too much about this edge case, as
> it seems very specific to this artificial benchmark scenario. Which of
> course doesn't mean that it's not worth doing, I just had bigger fish to
> fry and didn't get around to it yet.
Yeah, that's fair. I dug a bit further in case there was anything useful
to write up, but I don't have much to add beyond what's here and in the
thread you linked. We can let it live on in the archive for now.
-Peff
^ permalink raw reply
* Re: [PATCH 01/13] load_one_loose_object_map(): fix resource leak
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <17242c249f0beb387fd30634663f13ce42d34f79.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:19AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 0b626c1b85..47b7f5ec38 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -65,6 +65,7 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> {
> struct strbuf buf = STRBUF_INIT, path = STRBUF_INIT;
> FILE *fp;
> + int ret = -1;
>
> if (!loose->map)
> loose_object_map_init(&loose->map);
> @@ -98,13 +99,12 @@ static int load_one_loose_object_map(struct repository *repo, struct odb_source_
> insert_loose_map(loose, &oid, &compat_oid);
> }
>
> - strbuf_release(&buf);
> - strbuf_release(&path);
> - return errno ? -1 : 0;
> + ret = 0;
> err:
> + fclose(fp);
> strbuf_release(&buf);
> strbuf_release(&path);
> - return -1;
> + return ret;
> }
Makes sense. There's no `goto err` before we assign `fp`, and when the
call to `fopen()` fails we return via a different path. So the added
call to `fclose(fp)` is fine.
Patrick
^ permalink raw reply
* Re: [PATCH 07/13] dir: free allocations on parse-error paths in read_one_dir()
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <62ce03454aa1928edd8fa538e0600155629939cd.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:25AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/dir.c b/dir.c
> index 32430090dc..23335b9f7a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
> ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
>
> ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
> - if (data > end)
> + if (data > end) {
> + free(ud.untracked);
> return -1;
> + }
> ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
>
> eos = memchr(data, '\0', end - data);
> - if (!eos || eos == end)
> + if (!eos || eos == end) {
> + free(ud.untracked);
> + free(ud.dirs);
> return -1;
> + }
>
> *untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
> memcpy(untracked, &ud, sizeof(ud));
Hm. Here we assign ownership to the caller, but this still feels quite
off to me as we also have two more early returns after this point that
seem to leak memory. Do the callers make sure to always free the data?
Patrick
^ permalink raw reply
* Re: [PATCH 08/13] submodule: fix cwd leak in get_superproject_working_tree()
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <6a43f952417259c23ca456c547b0e4587a0ce6fa.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:26AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/submodule.c b/submodule.c
> index fd91201a92..8ddeebd8af 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> * We might have a superproject, but it is harder
> * to determine.
> */
> - return 0;
> + goto out;
>
> if (!strbuf_realpath(&one_up, "../", 0))
> - return 0;
> + goto out;
>
> subpath = relative_path(cwd, one_up.buf, &sb);
> strbuf_release(&one_up);
> @@ -2693,6 +2693,10 @@ int get_superproject_working_tree(struct strbuf *buf)
> die(_("ls-tree returned unexpected return code %d"), code);
>
> return ret;
> +
> +out:
> + free(cwd);
> + return 0;
> }
Okay. This is fine, but it feels a bit fragile as we also have a call to
`free(cwd)` a bit further up. So if somebody were to add a `goto out`
after that call we'd have a double free. Makes me wonder whether we want
to have a single exit path for the complete function and then drop the
other call to free(3p).
Patrick
^ permalink raw reply
* Re: [PATCH 05/13] run_diff_files: avoid memory leak
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <860bc8f52dc9be8bbfafcda296be831a1ffaf1c2.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:23AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/diff-lib.c b/diff-lib.c
> index ae91027a02..7ba839b4a8 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -152,7 +152,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> continue;
>
> if (ce_stage(ce)) {
> - struct combine_diff_path *dpath;
> + struct combine_diff_path *dpath = NULL;
> struct diff_filepair *pair;
> unsigned int wt_mode = 0;
> int num_compare_stages = 0;
> @@ -164,6 +164,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
> else {
> if (changed < 0) {
> perror(ce->name);
> + free(dpath);
> continue;
> }
> wt_mode = 0;
Huh. There is no assignment between the variable declaration and this
call to `continue`, so how could this ever plug a memory leak? None of
the other paths seem to leak the variable, either.
Patrick
^ permalink raw reply
* Re: [PATCH 04/13] run-command: avoid close(-1) in start_command() error paths
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <d7bcdda31276b5a17f11c307deb0f99ae1dc2861.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:22AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/run-command.c b/run-command.c
> index e70a8a387b..ce84db8782 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
> failed_errno = errno;
> if (need_in)
> close_pair(fdin);
> - else if (cmd->in)
> + else if (cmd->in > 0)
> close(cmd->in);
> str = "standard output";
> goto fail_pipe;
> @@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
> failed_errno = errno;
> if (need_in)
> close_pair(fdin);
> - else if (cmd->in)
> + else if (cmd->in > 0)
> close(cmd->in);
> if (need_out)
> close_pair(fdout);
> - else if (cmd->out)
> + else if (cmd->out > 0)
> close(cmd->out);
> str = "standard error";
> fail_pipe:
Right. There's a fourth site that does `close(cmd->out)`, but that site
already guards with `if (cmd->out > 0)`.
Patrick
^ permalink raw reply
* Re: [PATCH 02/13] loose: avoid closing invalid fd on error path
From: Patrick Steinhardt @ 2026-07-01 7:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin
In-Reply-To: <a1cd229e33c0ecf8ccbef9ab07b4b93896eae22e.1782889472.git.gitgitgadget@gmail.com>
On Wed, Jul 01, 2026 at 07:04:20AM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/loose.c b/loose.c
> index 47b7f5ec38..2c6db45245 100644
> --- a/loose.c
> +++ b/loose.c
> @@ -202,7 +202,8 @@ static int write_one_object(struct odb_source_loose *loose,
> return 0;
> errout:
> error_errno(_("failed to write loose object index %s"), path.buf);
> - close(fd);
> + if (fd >= 0)
> + close(fd);
> rollback_lock_file(&lock);
> strbuf_release(&buf);
> strbuf_release(&path);
Makes sense. At the time we hit the first `goto errout` we have already
assigned `fd = open(...)`, so we know it should be either negative or a
positive file descriptor.
There's also a second call to `close(fd)`, but if that call is
successful then we would not use the `errout` path. If it fails we may
try to close the file descriptor a second time, but that's probably a
non-issue.
Patrick
^ permalink raw reply
* Re: After installing Git version 2.55.0.windows.1 on Windows, it is impossible to pull or push code using the domain account password
From: Johannes Schindelin @ 2026-07-01 7:42 UTC (permalink / raw)
To: 2484937193@qq.co; +Cc: git
In-Reply-To: <tencent_7021B02790D4CB40EA0EF7A9E93DE4D48009@qq.com>
Hi,
I understand that you're distressed, but asking here and at
https://github.com/git-for-windows/git/discussions/6307 and at
https://github.com/git-for-windows/git/issues/6308 is actually _less_
likely for people to want to help you.
Let's continue this discussion over at
https://github.com/git-for-windows/git/issues/6308 (because there is at
least one person on this list who prefers to keep Windows stuff out of
this list).
Ciao,
Johannes
On Wed, 1 Jul 2026, 2484937193@qq.co wrote:
> URGENT! After installing git version 2.53.0.windows.2 on Windows, it was impossible to pull and push code using domain account passwords. Searching online, I found that configuring git config --global http."xxx".allowNTLMAuth true would make it work normally. However, after upgrading git to version 2.55.0.windows.1, even after configuring git config --global http."xx".allowNTLMAuth true, it still reported authentication failure.
> We are eagerly awaiting your reply.
^ permalink raw reply
* After installing Git version 2.55.0.windows.1 on Windows, it is impossible to pull or push code using the domain account password
From: 2484937193@qq.co @ 2026-07-01 7:06 UTC (permalink / raw)
To: git
URGENT! After installing git version 2.53.0.windows.2 on Windows, it was impossible to pull and push code using domain account passwords. Searching online, I found that configuring git config --global http."xxx".allowNTLMAuth true would make it work normally. However, after upgrading git to version 2.55.0.windows.1, even after configuring git config --global http."xx".allowNTLMAuth true, it still reported authentication failure.
We are eagerly awaiting your reply.
^ permalink raw reply
* [PATCH 13/13] mingw: make exit_process() own the process handle on all paths
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
After "mingw: kill child processes in a gentler way", the ownership of
the HANDLE passed to exit_process() and terminate_process_tree() is
inconsistent. terminate_process_tree() always closes the handle;
exit_process() closes it on success and on the terminate-tree
fallback, but leaks it on the early return where GetExitCodeProcess()
fails or reports the process is no longer STILL_ACTIVE.
mingw_kill() compensated by closing the handle on its own error path,
which is a double-close on every error path that does not hit that
one leaky branch -- the callee has already closed the handle by then.
Coverity flagged the resulting use-after-free as CID 1437238.
Pin down the invariant that exit_process() and
terminate_process_tree() own the handle from the call onward and
close it on every return path; with that, the bogus close in
mingw_kill() goes away.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/mingw.c | 4 +---
compat/win32/exit-process.h | 1 +
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 41e055f7de..e2cb92a414 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2269,10 +2269,8 @@ int mingw_kill(pid_t pid, int sig)
}
ret = terminate_process_tree(h, 128 + sig);
}
- if (ret) {
+ if (ret)
errno = err_win_to_posix(GetLastError());
- CloseHandle(h);
- }
return ret;
} else if (pid > 0 && sig == 0) {
HANDLE h = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, pid);
diff --git a/compat/win32/exit-process.h b/compat/win32/exit-process.h
index d53989884c..26004161bc 100644
--- a/compat/win32/exit-process.h
+++ b/compat/win32/exit-process.h
@@ -159,6 +159,7 @@ static int exit_process(HANDLE process, int exit_code)
return terminate_process_tree(process, exit_code);
}
+ CloseHandle(process);
return 0;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 12/13] fsmonitor: plug token-data leak on early daemon-startup failures
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
`fsmonitor_run_daemon()` allocates `state.current_token_data`
before any subordinate setup step that may fail (alias resolution,
listener/health constructors, asynchronous IPC server init). On
the successful path the listener thread takes ownership and clears
the field during its teardown, so the `done:` cleanup block sees a
NULL pointer. On every early-error path, however, control jumps
straight to `done:` with the freshly allocated token data still
referenced, and it is never freed, as Coverity flagged.
Free it at the top of `done:` and clear the pointer. The success
path is a no-op (the pointer is already NULL there); the error
paths now drop the otherwise-leaked allocation.
`fsmonitor_free_token_data()` is NULL-safe and asserts
`client_ref_count == 0`, which holds trivially here because the
IPC server has not yet begun accepting clients when these failures
occur.
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/fsmonitor--daemon.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index f920cf3a82..4161dd8282 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1418,6 +1418,8 @@ static int fsmonitor_run_daemon(void)
err = fsmonitor_run_daemon_1(&state);
done:
+ fsmonitor_free_token_data(state.current_token_data);
+ state.current_token_data = NULL;
pthread_cond_destroy(&state.cookies_cond);
pthread_mutex_destroy(&state.main_lock);
{
--
gitgitgadget
^ permalink raw reply related
* [PATCH 11/13] reftable/table: release filter on error path
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
reftable_table_refs_for_unindexed() allocates a filtering_ref_iterator
and then calls reftable_buf_add() to populate its oid buffer. On
success ownership is transferred to the output iterator, but if
reftable_buf_add() fails, the goto-out cleanup only frees the table
iterator and walks away from both the filter allocation and the
oid buffer that reftable_buf_add() may have grown.
Release filter->oid and free filter alongside the existing table
iterator cleanup.
Reported by Coverity as CID 1671512 ("Resource leak").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
reftable/table.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..d604ddebf4 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -709,6 +709,10 @@ out:
if (ti)
table_iter_close(ti);
reftable_free(ti);
+ if (filter) {
+ reftable_buf_release(&filter->oid);
+ reftable_free(filter);
+ }
}
return err;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH 10/13] imap-send: avoid leaking the IMAP upload buffer
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When uploading messages via libcurl, curl_append_msgs_to_imap()
accumulates each one in a strbuf that grows across loop iterations
but is never released before the function returns.
Release it alongside the existing libcurl cleanup.
Reported by Coverity as CID 1671507 ("Resource leak").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
imap-send.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/imap-send.c b/imap-send.c
index cfd6a5120c..0d16d02029 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1750,6 +1750,7 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
curl_easy_cleanup(curl);
curl_global_cleanup();
+ strbuf_release(&msgbuf.buf);
if (cred.username) {
if (res == CURLE_OK)
--
gitgitgadget
^ permalink raw reply related
* [PATCH] ci(dockerized): reduce the PID limit for private repositories
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Every once in a while I need to verify that Microsoft Git's test suite
passes for changes that are not yet meant for public consumption, and
since it was (made) too difficult to keep up a working Azure Pipeline
definition, I have to use GitHub Actions in a private GitHub repository
for that purpose.
In these tests, basically all Dockerized CI jobs fail consistently. The
symptom is something like:
error: cannot create async thread: Resource temporarily unavailable
in the middle of a test, typically in the t5xxx-t6xxx range. The first
such error is immediately followed by plenty more of these errors, and
not a single test succeeds afterwards.
At first, I thought that maybe the massive parallelism I enjoy there is
the problem, and I thought that the cgroups limits might be shared
between the many containers that run on essentially the same physical
machine. But even reducing the matrix to just a single of those
Dockerized jobs runs into the very same problems.
The underlying reason seems to be a substantial difference in the hosted
runners that execute these Dockerized jobs: forcing the PID limit of the
container to a high number lets the jobs pass, even when running the
complete matrix of all 13 Dockerized jobs concurrently. But that's not
the only difference: The jobs seem to take a lot longer in these
containers than, say, in the containers made available to
https://github.com/git/git.
When forcing a PID limit of 64k in that private repository, the jobs
completed successfully, but they also took a lot longer, between 2x to
2.5x longer, i.e. painfully much longer. Reducing the PID limit to 16k,
the CI jobs still passed, but took an equally long amount of time.
Reducing the PID limit to 8k caused the errors to reappear.
Here are the numbers from three example runs, the first one forcing the
PID and nproc limit to 65536, the second one to 16384, the third run is
from the public git/git repository:
Job | 64k | 16k | reference
------------------------------|---------|---------|---------
almalinux-8 | 19m 3s | 16m 0s | 9m 36s
debian-11 | 20m 31s | 20m 3s | 8m 5s
fedora-breaking-changes-meson | 16m 29s | 19m 19s | 9m 40s
linux-asan-ubsan | 1h 10m | 1h 11m | 34m 36s
linux-breaking-changes | 25m 39s | 25m 58s | 13m 15s
linux-leaks | 1h 9m | 1h 10m | 33m 30s
linux-meson | 28m 9s | 27m 4s | 13m 45s
linux-musl-meson | 16m 32s | 13m 39s | 8m 6s
linux-reftable-leaks | 1h 13m | 1h 13m | 34m 34s
linux-reftable | 26m 2s | 25m 48s | 13m 31s
linux-sha256 | 26m 12s | 26m 3s | 12m 36s
linux-TEST-vars | 26m 5s | 25m 21s | 13m 25s
linux32 | 21m 16s | 19m 57s | 10m 44s
It does not look as if the PID limit is the reason for the longer
runtime, seeing as the 64k vs 16k timings deviate no more than as is
usual with GitHub workflows. So let's go for 16k.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
ci(dockerized): reduce the PID limit for private repositories
I needed to craft this patch while developing fixes for vulnerabilities
which eventually were published as Git for Windows v2.53.0(3).
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2164%2Fdscho%2Fraise-pid-limit-in-private-repositories-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2164/dscho/raise-pid-limit-in-private-repositories-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2164
.github/workflows/main.yml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index cf341d74db..85cfedf5b0 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -420,7 +420,9 @@ jobs:
CI_JOB_IMAGE: ${{matrix.vector.image}}
CUSTOM_PATH: /custom
runs-on: ubuntu-latest
- container: ${{matrix.vector.image}}
+ container:
+ image: ${{ matrix.vector.image }}
+ options: ${{ github.repository_visibility == 'private' && '--pids-limit 16384 --ulimit nproc=16384:16384 --ulimit nofile=32768:32768' || '' }}
steps:
- name: prepare libc6 for actions
if: matrix.vector.jobname == 'linux32'
base-commit: e9019fcafe0040228b8631c30f97ae1adb61bcdc
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/13] worktree: fix resource leaks when branch creation fails
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In the "add" subcommand, when run_command() fails while creating
a new branch (line 948), the function returns -1 immediately
without freeing the allocations made earlier: path (from
prefix_filename at line 858), opt_track, branch_to_free, and
new_branch_to_free.
Redirect the error return through the existing cleanup block at
the end of the function so all four allocations are properly
freed.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/worktree.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d21c43fde3..4bc7b4f6e7 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -945,14 +945,17 @@ static int add(int ac, const char **av, const char *prefix,
strvec_push(&cp.args, branch);
if (opt_track)
strvec_push(&cp.args, opt_track);
- if (run_command(&cp))
- return -1;
+ if (run_command(&cp)) {
+ ret = -1;
+ goto cleanup;
+ }
branch = new_branch;
} else if (opt_track) {
die(_("--[no-]track can only be used if a new branch is created"));
}
ret = add_worktree(path, branch, &opts);
+cleanup:
free(path);
free(opt_track);
free(branch_to_free);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/13] submodule: fix cwd leak in get_superproject_working_tree()
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
get_superproject_working_tree() allocates cwd via xgetcwd() at
the top of the function, but two early-return paths (when not
inside a work tree, and when strbuf_realpath for "../" fails)
return 0 without freeing it.
Redirect these early returns through a cleanup label that frees
cwd before returning.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
submodule.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index fd91201a92..8ddeebd8af 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2627,10 +2627,10 @@ int get_superproject_working_tree(struct strbuf *buf)
* We might have a superproject, but it is harder
* to determine.
*/
- return 0;
+ goto out;
if (!strbuf_realpath(&one_up, "../", 0))
- return 0;
+ goto out;
subpath = relative_path(cwd, one_up.buf, &sb);
strbuf_release(&one_up);
@@ -2693,6 +2693,10 @@ int get_superproject_working_tree(struct strbuf *buf)
die(_("ls-tree returned unexpected return code %d"), code);
return ret;
+
+out:
+ free(cwd);
+ return 0;
}
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/13] dir: free allocations on parse-error paths in read_one_dir()
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When read_one_dir() encounters a parse error while reading the
untracked cache from disk, it returns -1 immediately. Two
allocations made earlier in the function can leak on these
early-return paths: ud.untracked (allocated at line 3846 when
untracked_nr > 0) and ud.dirs (allocated at line 3851).
Free both before returning on the two error paths between these
allocations and the point where they are transferred into the
final xmalloc'd struct at line 3857.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
dir.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/dir.c b/dir.c
index 32430090dc..23335b9f7a 100644
--- a/dir.c
+++ b/dir.c
@@ -3792,13 +3792,18 @@ static int read_one_dir(struct untracked_cache_dir **untracked_,
ALLOC_ARRAY(ud.untracked, ud.untracked_nr);
ud.dirs_alloc = ud.dirs_nr = decode_varint(&data);
- if (data > end)
+ if (data > end) {
+ free(ud.untracked);
return -1;
+ }
ALLOC_ARRAY(ud.dirs, ud.dirs_nr);
eos = memchr(data, '\0', end - data);
- if (!eos || eos == end)
+ if (!eos || eos == end) {
+ free(ud.untracked);
+ free(ud.dirs);
return -1;
+ }
*untracked_ = untracked = xmalloc(st_add3(sizeof(*untracked), eos - data, 1));
memcpy(untracked, &ud, sizeof(ud));
--
gitgitgadget
^ permalink raw reply related
* [PATCH 06/13] line-log: avoid redundant copy that leaks in process_ranges
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When bloom_filter_check() indicates that a commit does not touch
any of the tracked paths, line_log_process_ranges_arbitrary_commit()
propagates the current ranges to the parent by calling
line_log_data_copy() and passing the copy to add_line_range().
However, add_line_range() always makes its own copy internally
(via line_log_data_copy or line_log_data_merge), so the caller's
copy is never freed and leaks every time this path is taken.
Pass range directly to add_line_range() instead of making a
redundant intermediate copy. The callee's internal copy handles
ownership correctly.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
line-log.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/line-log.c b/line-log.c
index 5fc75ae275..0179f138f7 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1141,8 +1141,7 @@ int line_log_process_ranges_arbitrary_commit(struct rev_info *rev, struct commit
if (range) {
if (commit->parents && !bloom_filter_check(rev, commit, range)) {
- struct line_log_data *prange = line_log_data_copy(range);
- add_line_range(rev, commit->parents->item, prange);
+ add_line_range(rev, commit->parents->item, range);
clear_commit_line_range(rev, commit);
} else if (commit->parents && commit->parents->next)
changed = process_ranges_merge_commit(rev, commit, range);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 05/13] run_diff_files: avoid memory leak
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In 4fc970c4388 (diff --cc: fix display of symlink conflicts during a
merge., 2007-02-25) a conditional block was introduced in
`run_diff_files()` that skips the rest of the loop iteration and
advances directly to the next iteration.
However, it missed that there was a similar conditional block that was
last touched in b4b1550315c (Don't instantiate structures with FAMs.,
2006-06-18) and which demonstrated that the `dpath` structure needed to
be released.
Let's fix this.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff-lib.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/diff-lib.c b/diff-lib.c
index ae91027a02..7ba839b4a8 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -152,7 +152,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
continue;
if (ce_stage(ce)) {
- struct combine_diff_path *dpath;
+ struct combine_diff_path *dpath = NULL;
struct diff_filepair *pair;
unsigned int wt_mode = 0;
int num_compare_stages = 0;
@@ -164,6 +164,7 @@ void run_diff_files(struct rev_info *revs, unsigned int option)
else {
if (changed < 0) {
perror(ce->name);
+ free(dpath);
continue;
}
wt_mode = 0;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 04/13] run-command: avoid close(-1) in start_command() error paths
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When start_command() fails to set up a pipe partway through, it
rolls back by closing the pipe ends it has already opened. For
descriptors supplied by the caller rather than allocated locally,
that rollback tested `if (cmd->in)` / `if (cmd->out)` before calling
close(). The CHILD_PROCESS_INIT default of -1 ("no descriptor") is
non-zero and so passes the test, meaning a caller that sets
cmd->no_stdin or cmd->no_stdout without supplying a real fd ends up
triggering close(-1) on the error path.
The stdin-pipe failure branch a few lines above already uses the
right idiom, `if (cmd->out > 0)`, which rejects both the -1 sentinel
and 0 (the parent's own standard streams). Apply it to the three
remaining rollback sites.
Reported by Coverity as CID 1049722 ("Argument cannot be negative").
Assisted-by: Opus 4.7
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
run-command.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/run-command.c b/run-command.c
index e70a8a387b..ce84db8782 100644
--- a/run-command.c
+++ b/run-command.c
@@ -706,7 +706,7 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (need_in)
close_pair(fdin);
- else if (cmd->in)
+ else if (cmd->in > 0)
close(cmd->in);
str = "standard output";
goto fail_pipe;
@@ -720,11 +720,11 @@ int start_command(struct child_process *cmd)
failed_errno = errno;
if (need_in)
close_pair(fdin);
- else if (cmd->in)
+ else if (cmd->in > 0)
close(cmd->in);
if (need_out)
close_pair(fdout);
- else if (cmd->out)
+ else if (cmd->out > 0)
close(cmd->out);
str = "standard error";
fail_pipe:
--
gitgitgadget
^ permalink raw reply related
* [PATCH 03/13] download_https_uri_to_file(): do not leak fd upon failure
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When the `git-remote-https` command fails, we do not want to leak
`child_out`.
Pointed out by Coverity.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
bundle-uri.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bundle-uri.c b/bundle-uri.c
index 3b2e347288..34fa452e76 100644
--- a/bundle-uri.c
+++ b/bundle-uri.c
@@ -378,7 +378,7 @@ cleanup:
if (child_in)
fclose(child_in);
if (finish_command(&cp))
- return 1;
+ result = 1;
if (child_out)
fclose(child_out);
return result;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/13] loose: avoid closing invalid fd on error path
From: Johannes Schindelin via GitGitGadget @ 2026-07-01 7:04 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.2163.git.1782889472.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
write_one_object() opens a file at line 186 and jumps to the
errout label on failure. The errout cleanup unconditionally calls
close(fd), but when open() itself failed, fd is -1. Calling
close(-1) is harmless on most platforms (returns EBADF) but is
undefined behavior per POSIX and can confuse fd tracking in
sanitizer builds.
Guard the close with fd >= 0.
Pointed out by Coverity.
Assisted-by: Claude Opus 4.6
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
loose.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/loose.c b/loose.c
index 47b7f5ec38..2c6db45245 100644
--- a/loose.c
+++ b/loose.c
@@ -202,7 +202,8 @@ static int write_one_object(struct odb_source_loose *loose,
return 0;
errout:
error_errno(_("failed to write loose object index %s"), path.buf);
- close(fd);
+ if (fd >= 0)
+ close(fd);
rollback_lock_file(&lock);
strbuf_release(&buf);
strbuf_release(&path);
--
gitgitgadget
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox