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>
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>
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>
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>
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>
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 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>
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>
Commit c3d034df16 (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 1b6b2bfae5 (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 c3d034df16 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>
6cc6d1b4c6 (Documentation: update add --force option + ignore=all
config, 2026-02-06) added text describing both the ignore=none and
ignore=all behaviors. The former had minor formatting and grammatical
errors, while the latter was a bit garbled. I have tried to tweak the
wording on the latter to make it read as I think was intended, and fixed
the minor grammatical issues with both as well.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
a8215a2051 (send-email: add client certificate options, 2026-03-02)
added documentation for sendemail.smtpSSLClientKey that says it works
"in conjunction with `sendemail.smtpSSLClientKey`" -- referring to
itself. It appears that `sendemail.smtpSSLClientCert` was the intended
reference; fix it.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Fix various issues in the release notes -- missing/wrong articles, typo,
indentation, quote consistency, and wording improvement or corrections.
Other than the indentation fix for "The way combined list-object filter
options...", this patch is much easier to view with --color-words.
Signed-off-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As writing version 2 MIDX files by default breaks older versions of
Git and its reimplementations, use V2 only when necessary.
* jk/midx-write-v1-by-default:
MIDX: revert the default version to v1
We introduced midx version 2 in b2ec8e90c2 (midx: do not require packs
to be sorted in lexicographic order, 2026-02-24) and now write it by
default. The rationale was that older versions should ignore the v2 midx
and fall back to using the packs (just like we do for other midx
errors). Unfortunately this is not the case, as we have a hard die()
when we see an unknown midx version.
As a result, writing a midx with Git 2.54-rc2 puts the repository into a
state that is unusable with Git 2.53. And this midx write may happen
behind the scenes as part of normal operations, like fetch.
Let's switch back to writing v1 by default to avoid regressing the case
where multiple versions of Git are used on the same repository.
There is one gotcha, though: the v2 format is required for some new
features, like midx compaction, and running "git multi-pack-index
compact" will complain when asked to write a v1 index. The user must set
midx.version to "2" to make the feature work.
So instead of always using v1, we'll base the default on whether the
requested feature requires v2. That does mean that running midx
compaction will create a repository that can't be read by older versions
of Git. But we never do that by default; only people experimenting with
the new feature will be affected.
We have to adjust the test expectation in t5319, since it will now
generate v1 files. And our "auto-select v2" is covered by the tests in
t5335, which continue to check that compaction works without having to
set midx.version manually (and also explicitly check that asking for v1
with compaction reports the problem).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
- correct translation of pathspec msgs
Corrects cases where the “pathspec” is translated as if it was a
path
- correct translation of refspec msgs
Corrects cases where the “refspec” were not consistently translated
- correct translation of credential msgs
Corrects cases where the “credential” were not correctly translated
Signed-off-by: Stefan Björnelund <stefan.bjornelund.gnome@gmail.com>
Modified-by: Peter Krefting <peter@softwolves.pp.se>
Translate 198 previously fuzzy or untranslated messages, bringing the
total number of translated messages to 6226.
Reviewed-by: 依云 <lilydjwg@gmail.com>
Reviewed-by: Fangyi Zhou <me@fangyi.io>
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
The glossary entry is a list of terms and their definitions, so
multi-paragraph definitions need "+" continuation lines to indicate
that they are part of a single entry.
When an entry contains a sub-list (say, a bulleted list), the final "+"
may become ambiguous: is it connecting the next paragraph to the final
entry of the sub-list, or to the original list of definition paragraphs?
Asciidoc generally connects it to the former, even when we mean the
latter, and you end up with the next paragraph indented incorrectly,
like this:
glob
...defines glob...
Two consecutive asterisks ("**") in patterns matched
against full pathname may have special meaning:
- ...some special meaning of **...
- ...another special meaning of **...
- Other consecutive asterisks are considered invalid.
Glob magic is incompatible with literal magic.
That final "Glob magic is incompatible" paragraph is in the wrong spot.
It should be at the same level as "Two consecutive asterisks", as it is
not part of the final "Other consecutive asterisks" bullet point.
The same problem appears in several other spots in the glossary.
Usually we'd fix this by using "--" markers, which put the sub-list into
its own block. But there's a catch: in some of these spots we are
already in an open block, and nesting open blocks is a problem. It seems
to work for me using Asciidoc 10.2.1, but Asciidoctor 2.0.26 makes a
mess of it (our intent to open a new block seems to close the old one).
Fortunately there's a work-around: when using a "+" list-continuation,
the number of empty lines above the continuation indicates which level
of parent list to continue. So by adding an empty line after our
unordered list (before the "+"), we should be able to continue the
definition list item.
But asciidoc being asciidoc, of course that is not the end of the story.
That technique works fine for the "glob" and "attr" lists in this patch,
but under the "refs" item it works for only 1 of the 2 lists! I can't
figure out why, and this may be an asciidoctor bug. But we can work
around it by using "--" open-block markers here, since we're not
already in an open block.
So using the extra blank line for the first two instances, and "--"
markers for the second two, this patch produces identical output from
"doc-diff HEAD^ HEAD" for both --asciidoctor and --asciidoc modes.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
GitHub Actions started complaining about use of Node.js 20 and I was
wondering why only one job uses actions/checkout@v4, while everybody
else already uses actions/checkout@v5.
It turns out that it is caused by a semantic mismerge between
e75cd059 (ci: check formatting of our Rust code, 2025-10-15) that
added a new use of actions/checkout@v4 that happened very close to
another change 63541ed9 (build(deps): bump actions/checkout from 4
to 5, 2025-10-16) that updated all uses of actions/checkout@v4 to
use vactions/checkout@v5.
Update the leftover and the last use of actions/checkout@v4 to use
actions/checkout@v5 to help ourselves to move away from Node.js 20.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
I claimed in 3c18135b (doc: am: say that --message-id adds a trailer,
2026-02-09) that `git am --message-id` adds a Git trailer. But that
isn’t the case; for the case of a commit message with a subject, body,
and no trailer block:
<subject>
<paragrah>
It just appends the line right after `paragraph`:
<subject>
<paragraph>
Message-ID: <message-id_trailer.323@msgid.xyz>
It does work for two other cases though, namely subject-only and with an
existing trailer block.
This is at best an inconsistency and arguably a bug, but we’re at the
trailing end of the release cycle now. So reverting the doc is safer
than making msg-id act as a trailer, for now.
Revert this hunk from commit 3c18135b except the only useful
change (“Also use inline-verbatim for `Message-ID`”).
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We used writev() in limited code paths and supplied emulation for
platforms without working writev(), but the emulation was too
faithful to the spec to make the result useless to send even 64kB;
revert the topic and plan to restart the effort later.
* jc/no-writev-does-not-work:
Revert "compat/posix: introduce writev(3p) wrapper"
Revert "wrapper: introduce writev(3p) wrappers"
Revert "sideband: use writev(3p) to send pktlines"
Revert "cmake: use writev(3p) wrapper as needed"
Earlier we timelined that we'd tune our build procedures to build
with Rust by default in Git 2.53, but we are already in prerelease
freeze for 2.54 now. Update the BreakingChanges document to delay
it until Git 2.55 (slated for the end of June 2026).
Noticed-by: brian m. carlson <sandals@crustytoothpaste.net>
Helped-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King