[pull] master from git:master#234
Merged
Merged
Conversation
Commit c3d034d (csum-file: introduce discard_hashfile(), 2024-07-25) added a cleanup function that no longer has any callers. In that commit we adjusted do_write_index() to use the new function. But a similar fix occurred on a parallel branch, making free_hashfile() public, and the merge resolution in 1b6b2bf (Merge branch 'ps/leakfixes-part-4', 2024-08-23) took the free_hashfile() version. So now we have two functions, discard_hashfile() and free_hashfile(), and we only need one. Which one do we want to keep? The only difference between them is that the discard variant also closes the descriptors held in the struct. Let's look at the three callers: 1. In finalize_hashfile() we've either already closed the descriptors (if the CSUM_CLOSE flag is passed) or the caller didn't want them closed (if it didn't pass that flag). So we want the more limited free_hashfile(). 2. In object-file.c:flush_packfile_transaction() we close the descriptor ourselves. So discard_hashfile() could save us a line of code. 3. In do_write_index() we don't close the descriptor. This was the spot for which c3d034d added the discard function in the first place, but I'm skeptical that closing the descriptor here is the right thing. It is true that we are done with the descriptor at this point and closing it would be ideal. But we don't really own it! The descriptor comes from a tempfile struct (as part of a lock) and that tempfile will hold on to the descriptor and try to close it when it is deleted. This might happen at the end of the program, in which case the double-close is mostly harmless (we might accidentally close some other open descriptor, but at that point we're just closing and unlinking everything we can). But in theory it could also cause subtle bugs. If do_write_index() fails, we return the error up the stack and would eventually end up in write_locked_index(). There we roll back the lock file on error, which will close the descriptor. So now we get our double close, and we might actually close something else that was opened in the interim. This is probably unlikely in practice (as soon as we see the error we'd mostly be unwinding the stack, not opening new files). But it highlights a potential problem with the discard_hashfile() interface: the hashfile doesn't necessarily own that descriptor. Note that I said "descriptors" plural above. Those callers all care about the "fd" member of the struct. But discard_hashfile() also closes check_fd. That is only used if the struct is initialized with hashfd_check(), and neither of its two callers call either discard or free (they always "finalize" instead). So closing it is irrelevant for the current callers. I think we're better off sticking with the simpler free_hashfile() interface, and the handful of callers can decide how to handle the descriptors themselves. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The usual life-cycle for a git_hash_ctx is calling git_hash_init(), adding some data, and then using git_hash_final() to get the output digest and free any resources. Sometimes we decide to abort the operation without the final() call (e.g., due to errors or other reasons). In that case we just abandon the hash_ctx completely and let it go out of scope. For most hash implementations this is fine; they were just holding values directly in the struct. But some implementations do allocate memory, and in these cases we leak the memory. Notably OpenSSL >= 3.0 requires us to allocate the digest context on the heap with EVP_MD_CTX_new(). Let's provide a git_hash_discard() function that can be used in these code paths to free any resources. For now we'll implement it by just calling git_hash_final() into a dummy output, relying on its side effect of freeing the resources. Our view of the underlying hash implementation is abstracted behind the platform_SHA_* macros, so that's the best we can do without widening that interface. It's a little inefficient, but probably not noticeably so in practice, especially as we'd usually hit this on an error code path. And by abstracting it in this function, we can later swap it out when the platform_SHA interface lets us do so. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a hashfile struct is created, we always initialize the git_hash_ctx
inside it. We usually end up in hashfile_finalize(), which passes that
ctx to git_hash_final(), cleaning it up.
But a few code paths don't do so:
1. If we bail on the hashfile and call free_hashfile() directly rather
than finalizing.
2. If the skip_hash flag is set, the hashfile_finalize() call will
never call git_hash_final(). (You might think that we should just
avoid git_hash_init() entirely in this case, but the skip_hash flag
is set by the caller after the hashfile is initialized).
For most hash implementations this is OK, but for ones that allocate on
initialization it causes a memory leak. You can see many failures by
running:
make SANITIZE=leak OPENSSL_SHA1_UNSAFE=1 test
since OpenSSL >= 3.0 is such an allocating hash implementation (and
csum-file uses the "unsafe" algorithm variant).
We can solve this by calling git_hash_discard() as appropriate.
Note that free_hashfile() is used both directly by callers to abort
without finalizing, and by hashfile_finalize() to free memory. In the
latter case we _don't_ want to call git_hash_discard(), because we'll
already have either finalized or discarded it. So we'll push that to an
internal "free_memory" function, and keep free_hashfile() as the public
interface to abort a hashfile without finalizing.
This fix makes several scripts leak-free with the command above: t1600,
t1601, t2107, t7008, t9210, t9211.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A hashfile_checkpoint struct is basically just a copy of the hash_ctx state at a given point in the file. As such, it contains its own git_hash_ctx which may (depending on the underlying hash implementation) need to be discarded when we're done with it. Let's add a "release" function which cleans up the hash context it holds. I chose "release" here and not "discard" because you'd use this to clean up every checkpoint, whether you used it or not. As opposed to git_hash_discard(), which is needed only if you didn't call git_hash_final(). There are only two callers which use hashfile_checkpoints, and we can add release calls to both. When built with "SANITIZE=leak OPENSSL_SHA1_UNSAFE=1", this makes both t1050 and t9300 leak-free. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When computing a patch-id, we have a flush_one_hunk() helper that calls
git_hash_final() on our running hunk git_hash_ctx, and then
reinitializes that context for the next hunk.
When we run out of hunks to look at, we return, discarding the
git_hash_ctx. This can cause a leak if the hash implementation we are
using allocates any memory during its initialization. This includes
OpenSSL >= 3.0, for both SHA-1 and SHA-256. Normally we would not use
SHA-1 here at all, as we only recommend using non-DC implementations for
the "unsafe" variant (and patch-id, though they probably _could_ use the
unsafe variant, were never taught to do so).
But it is certainly a problem for SHA-256, which you can see with:
make SANITIZE=leak \
OPENSSL_SHA256=1 \
GIT_TEST_DEFAULT_HASH=sha256 \
test
That results in leak failures of 60 scripts, 57 of which are fixed by
this patch (basically anything which runs rebase will hit this case).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The happy path of check_stream_oid() is to initialize a hash, feed the
loose object zlib stream into it, and then get the final result. But if
we hit a zlib error or see extra cruft we'll bail early with an error.
Since we never call git_hash_final() in this cases, any resources held
by the git_hash_ctx may be leaked. Our default hash algorithms don't
allocate anything in the hash_ctx, but some implementations do. For
example, running:
make SANITIZE=leak \
OPENSSL_SHA256=1 \
GIT_TEST_DEFAULT_HASH=sha256 \
test
will fail t1450, since it feeds corrupted objects that cause us to bail
from check_stream_oid(). This patch fixes it by discarding the hash in
those early return paths. Trying to jump to a common "out:" label is not
worth it here, as we must _not_ discard a hash that was already fed to
git_hash_final(). And the hash_ctx itself does not carry any information
(so we cannot check for a NULL pointer, etc).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Usually an object request results in finish_http_object_request()
calling git_hash_final_oid(), after we've received all of the data. But
if we hit an error, we'll bail early and free the http_object_request,
dropping the git_hash_ctx entirely. This can cause a leak for hash
implementations that allocate memory in their context, like OpenSSL >=
3.0.
The obvious fix is for abort_http_object_request() to call
git_hash_discard(), under the assumption that every request is either
finished or aborted. But that's not quite true:
1. Not everybody calls the abort function. Sometimes they jump
straight to release_http_object_request(). So we'd have to put it
there.
2. After the finish function finalizes the hash, we can still
encounter errors! In that case we end up aborting or releasing,
and they must not discard that hash (since that would be a
double-free).
So we'll keep a flag marking the validity of the hash_ctx field of the
request. The lifetime is simple: it is valid immediately after creation,
up until we call finalize. And then our release function can just
conditionally discard the hash based on that flag.
This fixes test failures in t5550 and t5619 when run with:
make SANITIZE=leak \
OPENSSL_SHA256=1 \
GIT_TEST_DEFAULT_HASH=sha256 \
test
The flag handling could be removed if the hash-discard function were
idempotent. This could be done easily-ish by having the underlying
hash functions (like the ones in sha256/openssl.h) set the context
pointer to NULL after free-ing. But it's something that every platform
implementation would have to remember to do, and the benefit for the
callers is not that huge (it would let us shave a few lines here and
probably in a few other spots).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our abstracted hash-algorithm API allows for cloning a hash context. By
default this just memcpy()s the bytes, but specific implementations can
provide a custom clone function.
Our API is based around the way that OpenSSL works, which is that you
first initialize the destination context, then copy into it. In our code
that is this:
algo->init_fn(&dst);
git_hash_clone(&dst, src);
and that translates into OpenSSL calls like:
/* init_fn */
dst->ectx = EVP_MD_CTX_new();
EVP_DigestInit_ex(dst->ectx, EVP_sha256());
/* clone */
EVP_MD_CTX_copy_ex(dst->ectx, src->ectx);
So the allocation happens in the first step, and then the clone is just
copying values (the DigestInit is initializing values that just get
overwritten, but that's not wrong, just a little inefficient).
But libgcrypt doesn't work like that! Its copy function initializes dst
from scratch. So when using the sha256 gcrypt backend, that becomes:
/* init_fn; this allocates */
gcry_md_open(&dst, GCRY_MD_SHA256);
/* clone; this also allocates, leaking the previous value! */
gcry_md_copy(&dst, src);
You can see the leaks in the test suite by running:
make \
SANITIZE=leak \
GCRYPT_SHA256=1 \
GIT_TEST_DEFAULT_SHA=256 \
test
which has many failures, as opposed to building with OPENSSL_SHA256,
which is leak-free.
The easy fix here is for the clone function to close the open context
we're about to overwrite. It's a little inefficient (we did a pointless
open in the init function), but probably not a big deal in practice.
If our API went the other way, assuming that we're always cloning into
garbage bytes, then we could be more efficient. We'd teach OpenSSL's
clone function to do its own new(), skip the DigestInit, and then copy
into it. And gcrypt could stick with just the copy() call.
But look again at the asymmetry in the very first code example. We call
the init function straight from the git_hash_algo struct, and then
subsequent calls are dispatched through our git_hash_* wrappers. If you
wanted to clone into an uninitialized destination, you'd do something
like:
algo->clone_fn(&dst, src);
instead. That would require changing all of the callers. There's not
that many of them, but I don't know that it's worth changing our calling
conventions to try to reclaim this tiny bit of efficiency.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our git_hash_discard() is a bit hacky: it just calls git_hash_final()
into a dummy result buffer, using the side effect that each
implementation's Final() function will also free any resources.
This is probably not too terrible, since generating the final hash is
not that expensive and we'd mostly call discard on unusual or error code
paths. But we can do better by widening the platform API a bit to add an
explicit discard function.
This requires an annoying amount of boilerplate:
- Each algorithm needs a git_$ALGO_discard() wrapper that dereferences
the union'd git_hash_ctx into the type-safe field. So sha1 + sha256
+ sha1-unsafe, plus a BUG() for the unknown algo. And then these all
need to be referenced in the git_hash_algo structs.
- Platforms which don't do anything special to discard now need a
fallback function which does nothing. And we need this for each algo
(sha1, sha256, and sha1-unsafe).
- Platforms which do need to discard must define their discard
functions. This includes sha1/openssl, sha256/openssl, and
sha256/gcrypt (no sha1-unsafe here as it sits atop the sha1/openssl
functions).
- Algo selection needs to point platform_*_Discard to the appropriate
underlying macro, or indicate that the fallback should be used. We
have a similar situation for the Clone function (where a straight
memcpy() of the context struct is not enough for some platforms).
I've tied Discard to the same flag used by Clone here, since they
are basically the same problem: is the hash context a sequence of
bytes, or does it need smart copying/discarding?
It's easy to miss a case here since we don't even compile the
implementations we aren't using. I've tested with each of:
- no flags, which uses our internal sha1/sha256 implementations, both
of which exercise the noop fallback function
- OPENSSL_SHA1_UNSAFE=1, which checks that our unsafe macro
redirections work
- OPENSSL_SHA1=1, though you should not do that in real life!
- OPENSSL_SHA256=1, passes tests with GIT_TEST_DEFAULT_HASH=sha256
- GCRYPT_SHA256=1, which likewise passes
The other implementations do not set the CLONE_HELPER flag, so they
treat the context as bytes and should be fine with the fallback.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
On macOS, git status may abort while reading a directory entry whose UTF-8 name grows past NAME_MAX bytes: __chk_fail_overflow __strlcpy_chk precompose_utf8_readdir read_directory_recursive wt_status_collect cmd_status The precompose wrapper already reallocates dirent_prec_psx for long names, but d_name is declared as char[NAME_MAX + 1]. A fortified libc can still see that declared object size and reject a larger strlcpy bound, even though the allocation was grown. Make d_name a FLEX_ARRAY and size allocations from offsetof(). That matches the actual object layout with the dynamic allocation, so the fortified copy sees a destination whose size can grow with max_name_len. Add a regression test that creates an over-NAME_MAX non-ASCII basename and runs status with core.precomposeunicode enabled. Signed-off-by: Ihar Hrachyshka <ihar.hrachyshka@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Pointed out by Coverity. While at it, reduce near-duplicate clean-up code at the end of the function. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Two of `read_one_dir()`'s parse-error early returns leak ud.untracked and ud.dirs. Plug them. The other early returns in the same function are fine: they occur after the `xmalloc()`+`memcpy()` that copies ud into `*untracked_`, at which point ownership is transferred to the caller. `read_untracked_extension()` then releases everything via `free_untracked_cache()` on failure. Pointed out by Coverity. Assisted-by: Claude Opus 4.6 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
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> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The Git project uses CI systems from both GitHub and GitLab. While both of these systems are extensively used in day-to-day work, we only have a link to the GitHub Workflows in our README, which makes the GitLab CI hard to discover. Improve the situation by adding a second badge for GitLab CI to our README. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
One of the tests in t0021 writes a 2GB file and then roundtrips it through the clean/sumdge filters. This test is broken on 32 bit platforms because they typically don't handle files larger then `SSIZE_MAX` well at all. While our CI has a "linux32" job that should in theory hit this issue, we never noticed it because we didn't use to run EXPENSIVE tests until 7a094d6 (ci: run expensive tests on push builds to integration branches, 2026-05-08). And after that commit, the test does not fail but instead hangs completely. Ideally, we'd of course properly detect this situation and then test for it. In practice, this turns out to be hard as the test failure are not reliable as they often (but not always) run into ENOMEM errors. Instead, skip the test altogether. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In t4141 we generate a patch that is roughly 1GB in size to verify that git-apply(1) indeed rejects that patch. We generate that patch by prepending a patch header and then executing `test-tool genzeros` without a limit. This causes us to print infinitely many zeros, and we limit the overall amount of generated bytes via `test_copy_bytes`. This test setup is extremely expensive, as `test_copy_bytes` is implemented via `dd ibs=1 count="$1"`, which copies data one byte at a time. So as we write 1GB of data, we end up doing 1 billion reads and writes. This naturally takes a while: it takes 6 minutes on my system, and around 40 minutes in some CI jobs! We can do much better though, as genzeros already knows to handle an optional limit of how much data it is supposed to write, which allows us to remove the call to `test_copy_bytes`. Furthermore, it has already been optimized to generate the data fast. And indeed, doing this conversion drops the test execution to less than a second on my machine. That means that in theory it becomes feasible to drop the EXPENSIVE prerequisite now. But git-apply(1) still soaks up 1GB of data into memory, which may count as being expensive. Consequently, we keep the prerequisite intact. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The tests in t5608 perform a couple of clones of repositories that are somewhat large. Ultimately, we end up creating: - A setup repository that contains 2GB of uncompressed pack data. - A bare clone that contains the same 2GB of data. - A clone with worktree writes a 2GB packfile and a 2GB worktree. - A second setup repository that contains a 4GB packfile. - Two 4GB clone of that repository. Some of these clones ultimately hardlink files, which ensures that we at least don't end up with more than 20GB of data. But at the end of the test we still have around 16GB of data, which is only a tiny bit better. Refactor the test to prune repositories after they have no use anymore. This reduced the peak disk usage of this test to 8GB. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
One of the tests in t7508 is marked as EXPENSIVE because it ends up creating and adding files that are multiple gigabytes in size. This takes a while to complete, hence the EXPENSIVE prerequisite. Besides being expensive though the test can only work on systems where `size_t` is at least 64 bit. This is because one of the created files is larger than 4GB, and because Git tracks object size via `size_t` it will eventually blow up. This test has also been blowing up in the "linux32" CI job in GitHub Workflows since 7a094d6 (ci: run expensive tests on push builds to integration branches, 2026-05-08). But that job doesn't only fail, it also hangs, and that has been concealing the failure. Fix the issue by marking the test as requiring 64 bit `size_t`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
One of the tests in t7900 is marked with EXPENSIVE because we create a repository with 2GB of data that we end up repacking. We never clean up that repository though, so we occupy the full 2GB of data until the end of the test suite. Besides clogging our disk, having an EXPENSIVE test that alters the repository's state used by subsequent tests is also a bad idea, as it can easily have an impact on the heuristics used by other maintenance tasks. Adapt the test so that we create the data in a standalone repository that we clean up at the end of the test. While at it, also disable auto-maintenance so that it does not race with our manual maintenance. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
It's currently hard to explicitly disable GIT_TEST_LONG by setting it to `false`. Fix this by using `test_bool_env` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When we added the macOS jobs to GitLab CI in 56090a3 (ci: add macOS jobs to GitLab CI, 2024-01-18) we had to work around some very slow disks. This workaround essentially creates a RAM disk that we mount, where all test data is being written into RAM instead of the real disk. In the next commit though we're about to enable "GIT_TEST_LONG", which will make tests run that are marked with the "EXPENSIVE" prerequisite. This change will make a couple of tests run that write up to 8GB of data into the test output directory. As our RAM disk is only 4GB in size, this change will cause ENOSPC errors. We could accommodate for this by increasing the size of the RAM disk. In c9d708b (gitlab-ci: upgrade macOS runners, 2026-05-21) we have upgraded our runners to use the "large" runners, which have 16GB of RAM available. So we could easily expand the RAM disk to a capacity of for example 12GB. But some test runs have shown that this is still quite flaky overall, as we get quite close to our limits. Instead, drop the workaround completely. This does indeed slow down execution of the test jobs: - osx-clang goes from 18 minutes to 25 minutes - osx-meson goes from 21 minutes to 33 minutes - osx-reftable stays at 21 minutes The last one seems like an outlier. The only explanation that I have is that we end up writing significantly less files with the reftable backend, which ultimately causes less I/O. Overall though, it's preferable to have something that works with the least amount of flakiness compared to having something else that is faster but unstable. Despite that, the macOS jobs aren't even the slowest jobs, so this doesn't extend the overall pipeline's length. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Starting with 7a094d6 (ci: run expensive tests on push builds to integration branches, 2026-05-08) we run expensive tests in our CI for certain events. So far, this has only been wired up for GitHub Workflows though, which creates a test gap for GitLab CI. Plug this gap by also making this work for the latter. Note that these tests cannot be run on the Windows runners, as they only have 7.5GB of RAM. This is insufficient for some of the EXPENSIVE tests, so we explicitly disable "GIT_TEST_LONG" on these jobs. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
* jk/hash-algo-leak-fixes: hash: add platform-specific discard functions hash: fix memory leak copying sha256 gcrypt handles http: discard hash in dumb-http http_object_request check_stream_oid(): discard hash on read error patch-id: discard hash when done csum-file: provide a function to release checkpoints csum-file: always finalize or discard hash hash: add discard primitive csum-file: drop discard_hashfile()
The SGR values used for 256-color formatting are officially defined to be a single field with :-separated subfields (e.g. "\e[1;38:5:XX;40m") despite the more common but kludgy use of separate values (which then become context-dependent and lead to misinterpretation by incompatible terminals). Signed-off-by: Mantas Mikulėnas <grawity@gmail.com> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We'd like to add more logic to git_hash_init(), but many callers skip it and call algop->init_fn() directly. Let's make sure we're consistently using the wrapper by adding a coccinelle rule. Besides the coccinelle file itself, this is a purely mechanical conversion based on the patch it generates. There should be no bare init_fn() calls left (except for the one in the wrapper). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous patch added a coccinelle rule to make sure callers always use git_hash_init() rather than direct function pointers from the algo struct. Let's do the same for the rest of the git_hash_*() wrappers. I split these out because they're a bit different: they implicitly use the algop pointer in the git_hash_ctx. So when we convert: -algo->update_fn(&ctx, buf, len); +git_hash_update(&ctx, buf, len); we drop the reference to algo entirely! But this is always going to be the right thing. If "algo" does not match what is in ctx.algop, then we'd already be invoking undefined behavior. So in addition to making it possible to add more logic to the git_hash_*() functions, we're avoiding the need to pass around the extra algo pointer and make sure that it matches what's in "ctx". The rest of the patch is the mechanical application of that coccinelle patch, plus a minor cleanup in test-synthesize.c to drop a now-unused function parameter (since we don't have to pass around the algo separately anymore). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We want people to use the git_hash_*() wrappers rather than the bare
function pointers in the git_hash_algo struct. Let's document them
rather than the bare pointers, and warn people away from the pointers.
Coccinelle will eventually force the use of the wrappers, but it's
helpful to lead readers in the right direction from the start.
While we're here we can document a few other bits of wisdom I've turned
up while working in this area:
- You have to initialize the destination of a git_hash_clone(). This
is something we may eventually change for efficiency, but we should
definitely document the requirement for now.
- You must eventually finalize or discard a hash, since some backends
may allocate resources during initialization.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
You must always either finalize or discard a hash context to release any
resources, but you must call only one such function. This creates extra
work for some callers, since their cleanup code paths need to know
whether they got there via their happy path (and the finalization
happened) or due to an error (in which case they need to discard).
Let's add an "active" flag that turns a redundant discard into a noop.
That lets you safely do this:
git_hash_init(&ctx, algo);
...
if (some_error)
goto out;
...
git_hash_final(result, &ctx);
out:
git_hash_discard(&ctx);
This should avoid future errors, and will also let us simplify a few
existing callers (in future patches).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Now that it is safe to call git_hash_discard() even after finalizing it, we can simplify our cleanup logic a bit. This is mostly undoing a few bits of 64337ae (csum-file: always finalize or discard hash, 2026-07-02): - We no longer need a separate free_hashfile_memory() function for finalize_hashfile(). It can just call free_hashfile(), which will now discard (or not) the hash as appropriate. - When f->skip_hash is set, we don't need to discard; we can rely on free_hashfile() to do it. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Now that it is OK to call git_hash_discard() even after finalizing the hash, we no longer need the ctx_valid bool added by a2d8ea5 (http: discard hash in dumb-http http_object_request, 2026-07-02). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
It only makes sense to call git_hash_update(), etc, on a hash context that has been initialized but not yet finalized or discarded. This is an unlikely error to make, but it's easy for us to catch it and complain. It's especially important because it would quietly "work" for many hash backends (like sha1dc, which is just manipulating some bytes) but would cause undefined behavior with others (like OpenSSL, which puts the context onto the heap). Checking the flag lets us catch problems consistently on every build. Note that we can't do the same for git_hash_init(). Even though it would cause a leak to call it twice (without an intervening final/discard), the point of the function is that the contents of the struct are undefined before the call. But calling it twice is an even less likely error to make, so not covering it is OK. We leave git_hash_discard() alone, as its idempotent behavior is convenient for callers. We _could_ try to do something similar for git_hash_final(), allowing: git_hash_final(result, &ctx); git_hash_final(other_result, &ctx); but it does not make much sense. After the first final() call we have thrown away the state, so we cannot produce the same output. We could come up with some sensible output (the null hash, or the empty hash), but double-calls like this are more likely a bug, so our best bet is to complain loudly (whereas the current code produces either nonsense output or undefined behavior, depending on the backend). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The earlier d22a488 (wincred: avoid memory corruption, 2025-11-17) repaired only get_credential(); match_cred_password() has the same defect and is reached on `git credential reject`. When Git asks the helper to erase a stored credential whose password was supplied by the caller, the helper copies the candidate's password into a freshly allocated buffer for comparison. That copy overruns the allocation by one WCHAR of NUL, which on uninstrumented Windows manifests as process termination with status 0xC0000374. Because the helper can die before reaching CredDeleteW(), `git credential reject` masks the failure and the rejected credential remains stored. CredentialBlobSize is documented as a byte count, so for an N-WCHAR blob it equals N * sizeof(WCHAR). The pre-fix code allocated that many bytes and asked wcsncpy_s to copy N wide characters, but wcsncpy_s always appends a terminating NUL WCHAR, writing one WCHAR past the allocation. The destination-capacity argument was also passed in bytes rather than in WCHAR elements as the API requires, so the safe-CRT runtime never rejected the copy. See GHSA-rxqw-wxqg-g7hw. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When `git credential approve` hands the wincred helper a password together with an `oauth_refresh_token`, the OAuth branch of `store_credential()` writes one WCHAR past the allocation while formatting both fields into a single `CredentialBlob`. On Windows this trips heap verification and tears the helper down with status `0xC0000374`; `approve` masks the failure, so the credential the user meant to save never reaches `CredWriteW()` and the next session prompts for it again. The bug has the same shape as the one fixed in the previous commit: the allocation leaves no room for the terminating NUL, and the `sizeOfBuffer` argument to `_snwprintf_s()` is a byte count where the API expects a WCHAR count, which lets the safe-CRT runtime write the terminator out of bounds. Apply the same remedy d22a488 (wincred: avoid memory corruption, 2025-11-17) applied in `get_credential()`: allocate `(wlen + 1) * sizeof(WCHAR)` bytes and pass `wlen + 1` as the destination capacity in WCHARs. This closes the second of the two heap writes tracked under GHSA-rxqw-wxqg-g7hw. Assisted-by: Opus 4.7 Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The wincred credential helper has been updated to avoid memory corruption when erasing credentials and to prevent silent credential loss when storing OAuth tokens, by correcting buffer allocations and arguments passed to safe-CRT APIs. * js/wincred-fixes: wincred: prevent silent credential loss when storing OAuth tokens wincred: avoid memory corruption when erasing a credential
Various code paths that initialize a cryptographic hash context but bail out or finish without calling 'git_hash_final()' have been taught to call 'git_hash_discard()' to release allocated resources, fixing memory leaks when Git is built with non-default backends like 'OpenSSL' or 'libgcrypt'. * jk/hash-algo-leak-fixes: hash: add platform-specific discard functions hash: fix memory leak copying sha256 gcrypt handles http: discard hash in dumb-http http_object_request check_stream_oid(): discard hash on read error patch-id: discard hash when done csum-file: provide a function to release checkpoints csum-file: always finalize or discard hash hash: add discard primitive csum-file: drop discard_hashfile()
Various resource leaks, invalid file descriptor closures, and process handle ownership issues flagged by Coverity have been fixed. * js/coverity-fixes: mingw: make `exit_process()` own the process handle on all paths fsmonitor: plug token-data leak on early daemon-startup failures reftable/table: release filter on error path imap-send: avoid leaking the IMAP upload buffer worktree: fix resource leaks when branch creation fails submodule: fix cwd leak in `get_superproject_working_tree()` dir: free allocations on parse-error paths in `read_one_dir()` line-log: avoid redundant copy that leaks in process_ranges run-command: avoid `close(-1)` in `start_command()` error paths download_https_uri_to_file(): do not leak fd upon failure loose: avoid closing invalid fd on error path load_one_loose_object_map(): fix resource leak
Dockerized CI jobs running in private GitHub repositories have been adjusted to use explicit process and file limits, preventing resource exhaustion errors on private runners. * js/ci-dockerized-pid-limit: ci(dockerized): raise the PID limit for private repositories
Various test scripts have been updated to clean up large temporary files and repositories, reducing peak disk usage during testing. Also, expensive tests have been disabled on platforms that lack sufficient resources (like 32-bit platforms and Windows CI runners), and the long test suite has been enabled in GitLab CI. * ps/t-fixes-for-git-test-long: gitlab-ci: enable "GIT_TEST_LONG" gitlab-ci: disable RAM disk on macOS jobs t: use `test_bool_env` to parse GIT_TEST_LONG t7900: clean up large EXPENSIVE repository t7508: skip EXPENSIVE test that is broken without SIZE_T_IS_64BIT t5608: reduce maximum disk usage t4141: fix inefficient use of dd(1) t0021: skip EXPENSIVE test that is broken without SIZE_T_IS_64BIT README: add GitLab CI badge to make it more discoverable
The UTF-8 precomposition wrapper on macOS has been updated to use a flexible array member to represent the name of a directory entry, preventing fortified libc checks from failing when the name is reallocated to be larger than 'NAME_MAX' bytes. * ih/precompose-flex-array: precompose_utf8: use a flex array for d_name
The 'git_hash_*()' wrappers have been updated to be used consistently across the codebase instead of direct calls to members of 'struct git_hash_algo', and 'git_hash_discard()' has been made idempotent to simplify cleanups. * jk/git-hash-cleanups: hash: check ctx->active flag in all wrapper functions http: use idempotent git_hash_discard() csum-file: use idempotent git_hash_discard() hash: make git_hash_discard() idempotent hash: document function pointers and wrappers hash: convert remaining direct function calls hash: use git_hash_init() consistently
The sideband demultiplexer has been updated to recognize ANSI SGR escape sequences that use colon-separated subfields (e.g., for 256-color or true-color codes). * mm/sideband-ansi-sgr-colon-fix: sideband: allow ANSI SGR with colon-separated subfields
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )