{"thread":{"id":"65911","subject":"[PATCH 0/9] hash algorithm leak fixes","startedAt":"2026-07-02T07:52:36Z","lastAt":"2026-07-06T06:16:07Z","messageCount":20,"participants":["Jeff King","Junio C Hamano","Patrick Steinhardt","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"546959","messageId":"20260702075234.GA1548258@coredump.intra.peff.net","threadId":"65911","inReplyTo":null,"subject":"[PATCH 0/9] hash algorithm leak fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T07:52:34Z","receivedAt":"2026-07-02T07:52:36Z","isPatch":true,"body":"This series fixes some leaks you can find by running:\n\n  make SANITIZE=leak \\\n       OPENSSL_SHA256=1 \\\n       GIT_TEST_DEFAULT_HASH=sha256 \\\n       test\n\nThe crux of the issue is that we depend on calling git_hash_final() to\nclean up any git_hash_ctx we've initialized. But we don't always call\nthat function (we may return early due to an error, etc).\n\nWe don't see these in our regular leak-test builds because the default\nhash implementations we use treat the hash_ctx as a sequence of bytes.\nSo there's no cleanup needed, and just letting the context go out of\nscope is fine. But other implementations do allocate on initialization,\nand need to have some kind of free/discard function. So building with\nOPENSSL_SHA256 above is what lets us see the leaks.\n\nYou can see the same thing with OPENSSL_SHA1, but of course we don't\nrecommend that. Using OPENSSL_SHA1_UNSAFE likewise, but it sees only a\nsubset of the leaks since it is only used in a few code paths. Those\nleaks would be found if we turned on leak-checking in the\nlinux-TEST-vars job, but the rest of them would require leak-checking\nthe linux-sha256 job.\n\nAnd as a special bonus, patch 8 is a semi-related leak that only affects\nlibgcrypt. I don't think we build against that in CI at all. :-/\n\n  [1/9]: csum-file: drop discard_hashfile()\n  [2/9]: hash: add discard primitive\n  [3/9]: csum-file: always finalize or discard hash\n  [4/9]: csum-file: provide a function to release checkpoints\n  [5/9]: patch-id: discard hash when done\n  [6/9]: check_stream_oid(): discard hash on read error\n  [7/9]: http: discard hash in dumb-http http_object_request\n  [8/9]: hash: fix memory leak copying sha256 gcrypt handles\n  [9/9]: hash: add platform-specific discard functions\n\n builtin/fast-import.c |  1 +\n builtin/patch-id.c    |  1 +\n csum-file.c           | 30 +++++++++++++++++-------------\n csum-file.h           |  2 +-\n diff.c                |  1 +\n hash.c                | 29 +++++++++++++++++++++++++++++\n hash.h                | 22 ++++++++++++++++++++++\n http.c                |  4 ++++\n http.h                |  1 +\n object-file.c         |  4 ++++\n sha1/openssl.h        |  6 ++++++\n sha256/gcrypt.h       |  7 +++++++\n sha256/openssl.h      |  6 ++++++\n 13 files changed, 100 insertions(+), 14 deletions(-)\n\n-Peff\n"},{"id":"546960","messageId":"20260702075744.GA2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 1/9] csum-file: drop discard_hashfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T07:57:44Z","receivedAt":"2026-07-02T07:57:45Z","isPatch":true,"body":"Commit c3d034df16 (csum-file: introduce discard_hashfile(), 2024-07-25)\nadded a cleanup function that no longer has any callers. In that commit\nwe adjusted do_write_index() to use the new function. But a similar fix\noccurred on a parallel branch, making free_hashfile() public, and the\nmerge resolution in 1b6b2bfae5 (Merge branch 'ps/leakfixes-part-4',\n2024-08-23) took the free_hashfile() version.\n\nSo now we have two functions, discard_hashfile() and free_hashfile(),\nand we only need one. Which one do we want to keep?\n\nThe only difference between them is that the discard variant also closes\nthe descriptors held in the struct. Let's look at the three callers:\n\n  1. In finalize_hashfile() we've either already closed the descriptors\n     (if the CSUM_CLOSE flag is passed) or the caller didn't want them\n     closed (if it didn't pass that flag). So we want the more limited\n     free_hashfile().\n\n  2. In object-file.c:flush_packfile_transaction() we close the\n     descriptor ourselves. So discard_hashfile() could save us a line of\n     code.\n\n  3. In do_write_index() we don't close the descriptor. This was the spot\n     for which c3d034df16 added the discard function in the first place,\n     but I'm skeptical that closing the descriptor here is the right\n     thing. It is true that we are done with the descriptor at this\n     point and closing it would be ideal. But we don't really own it!\n\n     The descriptor comes from a tempfile struct (as part of a lock) and\n     that tempfile will hold on to the descriptor and try to close it\n     when it is deleted. This might happen at the end of the program, in\n     which case the double-close is mostly harmless (we might\n     accidentally close some other open descriptor, but at that point\n     we're just closing and unlinking everything we can).\n\n     But in theory it could also cause subtle bugs. If do_write_index()\n     fails, we return the error up the stack and would eventually end up\n     in write_locked_index(). There we roll back the lock file on error,\n     which will close the descriptor. So now we get our double close,\n     and we might actually close something else that was opened in the\n     interim.\n\n     This is probably unlikely in practice (as soon as we see the error\n     we'd mostly be unwinding the stack, not opening new files). But it\n     highlights a potential problem with the discard_hashfile()\n     interface: the hashfile doesn't necessarily own that descriptor.\n\nNote that I said \"descriptors\" plural above. Those callers all care\nabout the \"fd\" member of the struct. But discard_hashfile() also closes\ncheck_fd. That is only used if the struct is initialized with\nhashfd_check(), and neither of its two callers call either discard or\nfree (they always \"finalize\" instead). So closing it is irrelevant for\nthe current callers.\n\nI think we're better off sticking with the simpler free_hashfile()\ninterface, and the handful of callers can decide how to handle the\ndescriptors themselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a semi-related cleanup that is in this series because we'll be\ntouching the free function in a bit. And at first I thought we'd\nwant the discard() variant, but after poking around a bit I'm pretty\nsure we don't.\n\nI do like the name discard() better, as it makes it more clear that it\nis an alternative to finalize(). Since they have the same signature,\nswapping the names/implementations _could_ confuse long-running branches\nor topics in flight, but I kind of doubt there are any, given the\nhistory.\n\n csum-file.c | 9 ---------\n csum-file.h | 1 -\n 2 files changed, 10 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex d7a682c2b6..8ca9246a80 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -101,15 +101,6 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \treturn fd;\n }\n \n-void discard_hashfile(struct hashfile *f)\n-{\n-\tif (0 <= f->check_fd)\n-\t\tclose(f->check_fd);\n-\tif (0 <= f->fd)\n-\t\tclose(f->fd);\n-\tfree_hashfile(f);\n-}\n-\n void hashwrite(struct hashfile *f, const void *buf, uint32_t count)\n {\n \twhile (count) {\ndiff --git a/csum-file.h b/csum-file.h\nindex a270738a7a..d1a0ff29cd 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -74,7 +74,6 @@ void free_hashfile(struct hashfile *f);\n  * Finalize the hashfile by flushing data to disk and free'ing it.\n  */\n int finalize_hashfile(struct hashfile *, unsigned char *, enum fsync_component, unsigned int);\n-void discard_hashfile(struct hashfile *);\n void hashwrite(struct hashfile *, const void *, uint32_t);\n void hashflush(struct hashfile *f);\n void crc32_begin(struct hashfile *);\n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546961","messageId":"20260702075953.GB2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 2/9] hash: add discard primitive","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T07:59:53Z","receivedAt":"2026-07-02T07:59:54Z","isPatch":true,"body":"The usual life-cycle for a git_hash_ctx is calling git_hash_init(),\nadding some data, and then using git_hash_final() to get the output\ndigest and free any resources.\n\nSometimes we decide to abort the operation without the final() call\n(e.g., due to errors or other reasons). In that case we just abandon the\nhash_ctx completely and let it go out of scope. For most hash\nimplementations this is fine; they were just holding values directly in\nthe struct.\n\nBut some implementations do allocate memory, and in these cases we leak\nthe memory. Notably OpenSSL >= 3.0 requires us to allocate the digest\ncontext on the heap with EVP_MD_CTX_new().\n\nLet's provide a git_hash_discard() function that can be used in these\ncode paths to free any resources. For now we'll implement it by just\ncalling git_hash_final() into a dummy output, relying on its side effect\nof freeing the resources. Our view of the underlying hash implementation\nis abstracted behind the platform_SHA_* macros, so that's the best we\ncan do without widening that interface.\n\nIt's a little inefficient, but probably not noticeably so in practice,\nespecially as we'd usually hit this on an error code path. And by\nabstracting it in this function, we can later swap it out when the\nplatform_SHA interface lets us do so.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIn case you're on the edge of your seat, that widening happens in patch\n9. It was helpful to make sure the simple-and-stupid thing actually\nfixed the leaks first, and then do the convoluted platform-macro magic\nlater.\n\n hash.c | 12 ++++++++++++\n hash.h |  1 +\n 2 files changed, 13 insertions(+)\n\ndiff --git a/hash.c b/hash.c\nindex e925b9754e..63672a3d22 100644\n--- a/hash.c\n+++ b/hash.c\n@@ -283,6 +283,18 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)\n \tctx->algop->final_oid_fn(oid, ctx);\n }\n \n+void git_hash_discard(struct git_hash_ctx *ctx)\n+{\n+\t/*\n+\t * XXX Many implementations do not need to do anything here,\n+\t * and a dummy final() call is wasteful. But we can't fix\n+\t * that unless our implementation API exposes a discard\n+\t * primitive.\n+\t */\n+\tunsigned char dummy[GIT_MAX_RAWSZ];\n+\tgit_hash_final(dummy, ctx);\n+}\n+\n uint32_t hash_algo_by_name(const char *name)\n {\n \tif (!name)\ndiff --git a/hash.h b/hash.h\nindex c082a53c9a..6b2f04e2a4 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -325,6 +325,7 @@ void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src);\n void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len);\n void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx);\n void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx);\n+void git_hash_discard(struct git_hash_ctx *ctx);\n const struct git_hash_algo *hash_algo_ptr_by_number(uint32_t algo);\n struct git_hash_ctx *git_hash_alloc(void);\n void git_hash_free(struct git_hash_ctx *ctx);\n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546962","messageId":"20260702080130.GC2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 3/9] csum-file: always finalize or discard hash","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:01:30Z","receivedAt":"2026-07-02T08:01:31Z","isPatch":true,"body":"When a hashfile struct is created, we always initialize the git_hash_ctx\ninside it. We usually end up in hashfile_finalize(), which passes that\nctx to git_hash_final(), cleaning it up.\n\nBut a few code paths don't do so:\n\n  1. If we bail on the hashfile and call free_hashfile() directly rather\n     than finalizing.\n\n  2. If the skip_hash flag is set, the hashfile_finalize() call will\n     never call git_hash_final(). (You might think that we should just\n     avoid git_hash_init() entirely in this case, but the skip_hash flag\n     is set by the caller after the hashfile is initialized).\n\nFor most hash implementations this is OK, but for ones that allocate on\ninitialization it causes a memory leak. You can see many failures by\nrunning:\n\n  make SANITIZE=leak OPENSSL_SHA1_UNSAFE=1 test\n\nsince OpenSSL >= 3.0 is such an allocating hash implementation (and\ncsum-file uses the \"unsafe\" algorithm variant).\n\nWe can solve this by calling git_hash_discard() as appropriate.\n\nNote that free_hashfile() is used both directly by callers to abort\nwithout finalizing, and by hashfile_finalize() to free memory. In the\nlatter case we _don't_ want to call git_hash_discard(), because we'll\nalready have either finalized or discarded it. So we'll push that to an\ninternal \"free_memory\" function, and keep free_hashfile() as the public\ninterface to abort a hashfile without finalizing.\n\nThis fix makes several scripts leak-free with the command above: t1600,\nt1601, t2107, t7008, t9210, t9211.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n csum-file.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 8ca9246a80..44ff460692 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -55,24 +55,32 @@ void hashflush(struct hashfile *f)\n \t}\n }\n \n-void free_hashfile(struct hashfile *f)\n+static void free_hashfile_memory(struct hashfile *f)\n {\n \tfree(f->buffer);\n \tfree(f->check_buffer);\n \tfree(f);\n }\n \n+void free_hashfile(struct hashfile *f)\n+{\n+\tgit_hash_discard(&f->ctx);\n+\tfree_hashfile_memory(f);\n+}\n+\n int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \t\t      enum fsync_component component, unsigned int flags)\n {\n \tint fd;\n \n \thashflush(f);\n \n-\tif (f->skip_hash)\n+\tif (f->skip_hash) {\n+\t\tgit_hash_discard(&f->ctx);\n \t\thashclr(f->buffer, f->algop);\n-\telse\n+\t} else {\n \t\tgit_hash_final(f->buffer, &f->ctx);\n+\t}\n \n \tif (result)\n \t\thashcpy(result, f->buffer, f->algop);\n@@ -97,7 +105,7 @@ int finalize_hashfile(struct hashfile *f, unsigned char *result,\n \t\tif (close(f->check_fd))\n \t\t\tdie_errno(\"%s: sha1 file error on close\", f->name);\n \t}\n-\tfree_hashfile(f);\n+\tfree_hashfile_memory(f);\n \treturn fd;\n }\n \n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546963","messageId":"20260702080319.GD2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 4/9] csum-file: provide a function to release checkpoints","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:03:19Z","receivedAt":"2026-07-02T08:03:20Z","isPatch":true,"body":"A hashfile_checkpoint struct is basically just a copy of the hash_ctx\nstate at a given point in the file. As such, it contains its own\ngit_hash_ctx which may (depending on the underlying hash implementation)\nneed to be discarded when we're done with it.\n\nLet's add a \"release\" function which cleans up the hash context it\nholds. I chose \"release\" here and not \"discard\" because you'd use this\nto clean up every checkpoint, whether you used it or not. As opposed to\ngit_hash_discard(), which is needed only if you didn't call\ngit_hash_final().\n\nThere are only two callers which use hashfile_checkpoints, and we can\nadd release calls to both. When built with \"SANITIZE=leak\nOPENSSL_SHA1_UNSAFE=1\", this makes both t1050 and t9300 leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fast-import.c | 1 +\n csum-file.c           | 5 +++++\n csum-file.h           | 1 +\n object-file.c         | 2 ++\n 4 files changed, 9 insertions(+)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex aa656c5195..f6473dcc8e 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1216,6 +1216,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)\n out:\n \tfree(in_buf);\n \tfree(out_buf);\n+\thashfile_checkpoint_release(&checkpoint);\n }\n \n /* All calls must be guarded by find_object() or find_mark() to\ndiff --git a/csum-file.c b/csum-file.c\nindex 44ff460692..b166f89624 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -223,6 +223,11 @@ int hashfile_truncate(struct hashfile *f, struct hashfile_checkpoint *checkpoint\n \treturn 0;\n }\n \n+void hashfile_checkpoint_release(struct hashfile_checkpoint *checkpoint)\n+{\n+\tgit_hash_discard(&checkpoint->ctx);\n+}\n+\n void crc32_begin(struct hashfile *f)\n {\n \tf->crc32 = crc32(0, NULL, 0);\ndiff --git a/csum-file.h b/csum-file.h\nindex d1a0ff29cd..6ed74d1637 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -39,6 +39,7 @@ struct hashfile_checkpoint {\n void hashfile_checkpoint_init(struct hashfile *, struct hashfile_checkpoint *);\n void hashfile_checkpoint(struct hashfile *, struct hashfile_checkpoint *);\n int hashfile_truncate(struct hashfile *, struct hashfile_checkpoint *);\n+void hashfile_checkpoint_release(struct hashfile_checkpoint *);\n \n /* finalize_hashfile flags */\n #define CSUM_CLOSE\t\t1\ndiff --git a/object-file.c b/object-file.c\nindex e3d92bbda2..32a0d6d237 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1352,6 +1352,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas\n \t\t\t   state->alloc_written);\n \t\tstate->written[state->nr_written++] = idx;\n \t}\n+\n+\thashfile_checkpoint_release(&checkpoint);\n \treturn 0;\n }\n \n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546964","messageId":"20260702080411.GE2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 5/9] patch-id: discard hash when done","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:04:11Z","receivedAt":"2026-07-02T08:04:12Z","isPatch":true,"body":"When computing a patch-id, we have a flush_one_hunk() helper that calls\ngit_hash_final() on our running hunk git_hash_ctx, and then\nreinitializes that context for the next hunk.\n\nWhen we run out of hunks to look at, we return, discarding the\ngit_hash_ctx. This can cause a leak if the hash implementation we are\nusing allocates any memory during its initialization. This includes\nOpenSSL >= 3.0, for both SHA-1 and SHA-256. Normally we would not use\nSHA-1 here at all, as we only recommend using non-DC implementations for\nthe \"unsafe\" variant (and patch-id, though they probably _could_ use the\nunsafe variant, were never taught to do so).\n\nBut it is certainly a problem for SHA-256, which you can see with:\n\n  make SANITIZE=leak \\\n       OPENSSL_SHA256=1 \\\n       GIT_TEST_DEFAULT_HASH=sha256 \\\n       test\n\nThat results in leak failures of 60 scripts, 57 of which are fixed by\nthis patch (basically anything which runs rebase will hit this case).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/patch-id.c | 1 +\n diff.c             | 1 +\n 2 files changed, 2 insertions(+)\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 2781598ede..57d9bd4a65 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -173,6 +173,7 @@ static size_t get_one_patchid(struct object_id *next_oid, struct object_id *resu\n \t\toidclr(next_oid, the_repository->hash_algo);\n \n \tflush_one_hunk(result, &ctx);\n+\tgit_hash_discard(&ctx);\n \n \treturn patchlen;\n }\ndiff --git a/diff.c b/diff.c\nindex 2a9d0d8687..1568f0ed9c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6987,6 +6987,7 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid\n \t\tflush_one_hunk(oid, &ctx);\n \t}\n \n+\tgit_hash_discard(&ctx);\n \treturn 0;\n }\n \n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546965","messageId":"20260702080503.GF2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 6/9] check_stream_oid(): discard hash on read error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:05:03Z","receivedAt":"2026-07-02T08:05:04Z","isPatch":true,"body":"The happy path of check_stream_oid() is to initialize a hash, feed the\nloose object zlib stream into it, and then get the final result. But if\nwe hit a zlib error or see extra cruft we'll bail early with an error.\n\nSince we never call git_hash_final() in this cases, any resources held\nby the git_hash_ctx may be leaked. Our default hash algorithms don't\nallocate anything in the hash_ctx, but some implementations do. For\nexample, running:\n\n  make SANITIZE=leak \\\n       OPENSSL_SHA256=1 \\\n       GIT_TEST_DEFAULT_HASH=sha256 \\\n       test\n\nwill fail t1450, since it feeds corrupted objects that cause us to bail\nfrom check_stream_oid(). This patch fixes it by discarding the hash in\nthose early return paths. Trying to jump to a common \"out:\" label is not\nworth it here, as we must _not_ discard a hash that was already fed to\ngit_hash_final(). And the hash_ctx itself does not carry any information\n(so we cannot check for a NULL pointer, etc).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n object-file.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/object-file.c b/object-file.c\nindex 32a0d6d237..035d005279 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1587,11 +1587,13 @@ static int check_stream_oid(git_zstream *stream,\n \n \tif (status != Z_STREAM_END) {\n \t\terror(_(\"corrupt loose object '%s'\"), oid_to_hex(expected_oid));\n+\t\tgit_hash_discard(&c);\n \t\treturn -1;\n \t}\n \tif (stream->avail_in) {\n \t\terror(_(\"garbage at end of loose object '%s'\"),\n \t\t      oid_to_hex(expected_oid));\n+\t\tgit_hash_discard(&c);\n \t\treturn -1;\n \t}\n \n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546966","messageId":"20260702080707.GG2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:07:07Z","receivedAt":"2026-07-02T08:07:08Z","isPatch":true,"body":"Usually an object request results in finish_http_object_request()\ncalling git_hash_final_oid(), after we've received all of the data. But\nif we hit an error, we'll bail early and free the http_object_request,\ndropping the git_hash_ctx entirely.  This can cause a leak for hash\nimplementations that allocate memory in their context, like OpenSSL >=\n3.0.\n\nThe obvious fix is for abort_http_object_request() to call\ngit_hash_discard(), under the assumption that every request is either\nfinished or aborted. But that's not quite true:\n\n  1. Not everybody calls the abort function. Sometimes they jump\n     straight to release_http_object_request(). So we'd have to put it\n     there.\n\n  2. After the finish function finalizes the hash, we can still\n     encounter errors! In that case we end up aborting or releasing,\n     and they must not discard that hash (since that would be a\n     double-free).\n\nSo we'll keep a flag marking the validity of the hash_ctx field of the\nrequest. The lifetime is simple: it is valid immediately after creation,\nup until we call finalize. And then our release function can just\nconditionally discard the hash based on that flag.\n\nThis fixes test failures in t5550 and t5619 when run with:\n\n  make SANITIZE=leak \\\n       OPENSSL_SHA256=1 \\\n       GIT_TEST_DEFAULT_HASH=sha256 \\\n       test\n\nThe flag handling could be removed if the hash-discard function were\nidempotent. This could be done easily-ish by having the underlying\nhash functions (like the ones in sha256/openssl.h) set the context\npointer to NULL after free-ing. But it's something that every platform\nimplementation would have to remember to do, and the benefit for the\ncallers is not that huge (it would let us shave a few lines here and\nprobably in a few other spots).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI think the \"set to NULL\" thing gets weird with gcrypt, too, which does\nnot even use a pointer (we typedef their libgcrypt handle into our own\ncontext struct).\n\n http.c | 4 ++++\n http.h | 1 +\n 2 files changed, 5 insertions(+)\n\ndiff --git a/http.c b/http.c\nindex b4e7b8d00b..63abbaae8a 100644\n--- a/http.c\n+++ b/http.c\n@@ -2880,6 +2880,7 @@ struct http_object_request *new_http_object_request(const char *base_url,\n \tgit_inflate_init(&freq->stream);\n \n \tthe_hash_algo->init_fn(&freq->c);\n+\tfreq->hash_ctx_valid = 1;\n \n \tfreq->url = get_remote_object_url(base_url, hex, 0);\n \n@@ -2988,6 +2989,7 @@ int finish_http_object_request(struct http_object_request *freq)\n \t}\n \n \tgit_hash_final_oid(&freq->real_oid, &freq->c);\n+\tfreq->hash_ctx_valid = 0;\n \tif (freq->zret != Z_STREAM_END) {\n \t\tunlink_or_warn(freq->tmpfile.buf);\n \t\treturn -1;\n@@ -3028,6 +3030,8 @@ void release_http_object_request(struct http_object_request **freq_p)\n \tcurl_slist_free_all(freq->headers);\n \tstrbuf_release(&freq->tmpfile);\n \tgit_inflate_end(&freq->stream);\n+\tif (freq->hash_ctx_valid)\n+\t\tgit_hash_discard(&freq->c);\n \n \tfree(freq);\n \t*freq_p = NULL;\ndiff --git a/http.h b/http.h\nindex 729c51904d..6b0639150f 100644\n--- a/http.h\n+++ b/http.h\n@@ -255,6 +255,7 @@ struct http_object_request {\n \tstruct object_id oid;\n \tstruct object_id real_oid;\n \tstruct git_hash_ctx c;\n+\tint hash_ctx_valid;\n \tgit_zstream stream;\n \tint zret;\n \tint rename;\n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546967","messageId":"20260702080907.GH2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 8/9] hash: fix memory leak copying sha256 gcrypt handles","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:09:07Z","receivedAt":"2026-07-02T08:09:08Z","isPatch":true,"body":"Our abstracted hash-algorithm API allows for cloning a hash context. By\ndefault this just memcpy()s the bytes, but specific implementations can\nprovide a custom clone function.\n\nOur API is based around the way that OpenSSL works, which is that you\nfirst initialize the destination context, then copy into it. In our code\nthat is this:\n\n  algo->init_fn(&dst);\n  git_hash_clone(&dst, src);\n\nand that translates into OpenSSL calls like:\n\n  /* init_fn */\n  dst->ectx = EVP_MD_CTX_new();\n  EVP_DigestInit_ex(dst->ectx, EVP_sha256());\n  /* clone */\n  EVP_MD_CTX_copy_ex(dst->ectx, src->ectx);\n\nSo the allocation happens in the first step, and then the clone is just\ncopying values (the DigestInit is initializing values that just get\noverwritten, but that's not wrong, just a little inefficient).\n\nBut libgcrypt doesn't work like that! Its copy function initializes dst\nfrom scratch. So when using the sha256 gcrypt backend, that becomes:\n\n  /* init_fn; this allocates */\n  gcry_md_open(&dst, GCRY_MD_SHA256);\n  /* clone; this also allocates, leaking the previous value! */\n  gcry_md_copy(&dst, src);\n\nYou can see the leaks in the test suite by running:\n\n  make \\\n    SANITIZE=leak \\\n    GCRYPT_SHA256=1 \\\n    GIT_TEST_DEFAULT_SHA=256 \\\n    test\n\nwhich has many failures, as opposed to building with OPENSSL_SHA256,\nwhich is leak-free.\n\nThe easy fix here is for the clone function to close the open context\nwe're about to overwrite. It's a little inefficient (we did a pointless\nopen in the init function), but probably not a big deal in practice.\n\nIf our API went the other way, assuming that we're always cloning into\ngarbage bytes, then we could be more efficient. We'd teach OpenSSL's\nclone function to do its own new(), skip the DigestInit, and then copy\ninto it. And gcrypt could stick with just the copy() call.\n\nBut look again at the asymmetry in the very first code example. We call\nthe init function straight from the git_hash_algo struct, and then\nsubsequent calls are dispatched through our git_hash_* wrappers. If you\nwanted to clone into an uninitialized destination, you'd do something\nlike:\n\n  algo->clone_fn(&dst, src);\n\ninstead. That would require changing all of the callers. There's not\nthat many of them, but I don't know that it's worth changing our calling\nconventions to try to reclaim this tiny bit of efficiency.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha256/gcrypt.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/sha256/gcrypt.h b/sha256/gcrypt.h\nindex 17a90f1052..694a2b70a1 100644\n--- a/sha256/gcrypt.h\n+++ b/sha256/gcrypt.h\n@@ -27,6 +27,7 @@ static inline void gcrypt_SHA256_Final(unsigned char *digest, gcrypt_SHA256_CTX\n \n static inline void gcrypt_SHA256_Clone(gcrypt_SHA256_CTX *dst, const gcrypt_SHA256_CTX *src)\n {\n+\tgcry_md_close(*dst);\n \tgcry_md_copy(dst, *src);\n }\n \n-- \n2.55.0.418.g37da59dd42\n\n"},{"id":"546968","messageId":"20260702081349.GI2029434@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260702075234.GA1548258@coredump.intra.peff.net","subject":"[PATCH 9/9] hash: add platform-specific discard functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T08:13:49Z","receivedAt":"2026-07-02T08:13:51Z","isPatch":true,"body":"Our git_hash_discard() is a bit hacky: it just calls git_hash_final()\ninto a dummy result buffer, using the side effect that each\nimplementation's Final() function will also free any resources.\n\nThis is probably not too terrible, since generating the final hash is\nnot that expensive and we'd mostly call discard on unusual or error code\npaths. But we can do better by widening the platform API a bit to add an\nexplicit discard function.\n\nThis requires an annoying amount of boilerplate:\n\n  - Each algorithm needs a git_$ALGO_discard() wrapper that dereferences\n    the union'd git_hash_ctx into the type-safe field. So sha1 + sha256\n    + sha1-unsafe, plus a BUG() for the unknown algo. And then these all\n    need to be referenced in the git_hash_algo structs.\n\n  - Platforms which don't do anything special to discard now need a\n    fallback function which does nothing. And we need this for each algo\n    (sha1, sha256, and sha1-unsafe).\n\n  - Platforms which do need to discard must define their discard\n    functions. This includes sha1/openssl, sha256/openssl, and\n    sha256/gcrypt (no sha1-unsafe here as it sits atop the sha1/openssl\n    functions).\n\n  - Algo selection needs to point platform_*_Discard to the appropriate\n    underlying macro, or indicate that the fallback should be used. We\n    have a similar situation for the Clone function (where a straight\n    memcpy() of the context struct is not enough for some platforms).\n    I've tied Discard to the same flag used by Clone here, since they\n    are basically the same problem: is the hash context a sequence of\n    bytes, or does it need smart copying/discarding?\n\nIt's easy to miss a case here since we don't even compile the\nimplementations we aren't using. I've tested with each of:\n\n  - no flags, which uses our internal sha1/sha256 implementations, both\n    of which exercise the noop fallback function\n\n  - OPENSSL_SHA1_UNSAFE=1, which checks that our unsafe macro\n    redirections work\n\n  - OPENSSL_SHA1=1, though you should not do that in real life!\n\n  - OPENSSL_SHA256=1, passes tests with GIT_TEST_DEFAULT_HASH=sha256\n\n  - GCRYPT_SHA256=1, which likewise passes\n\nThe other implementations do not set the CLONE_HELPER flag, so they\ntreat the context as bytes and should be fine with the fallback.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nOne of the reasons I left this to the end is that I wasn't sure it would\nbe worth it. I think it probably is (hence posting it), but we could\nlive with the hacky implementation forever if we wanted. :)\n\nIt also prompted me to test with all of the backends I could build,\nwhich is how I found the unrelated gcrypt leak fixed in patch 8.\n\n hash.c           | 33 +++++++++++++++++++++++++--------\n hash.h           | 21 +++++++++++++++++++++\n sha1/openssl.h   |  6 ++++++\n sha256/gcrypt.h  |  6 ++++++\n sha256/openssl.h |  6 ++++++\n 5 files changed, 64 insertions(+), 8 deletions(-)\n\ndiff --git a/hash.c b/hash.c\nindex 63672a3d22..55d1d41770 100644\n--- a/hash.c\n+++ b/hash.c\n@@ -72,6 +72,11 @@ static void git_hash_sha1_final_oid(struct object_id *oid, struct git_hash_ctx *\n \toid->algo = GIT_HASH_SHA1;\n }\n \n+static void git_hash_sha1_discard(struct git_hash_ctx *ctx)\n+{\n+\tgit_SHA1_Discard(&ctx->state.sha1);\n+}\n+\n static void git_hash_sha1_init_unsafe(struct git_hash_ctx *ctx)\n {\n \tctx->algop = unsafe_hash_algo(&hash_algos[GIT_HASH_SHA1]);\n@@ -102,6 +107,11 @@ static void git_hash_sha1_final_oid_unsafe(struct object_id *oid, struct git_has\n \toid->algo = GIT_HASH_SHA1;\n }\n \n+static void git_hash_sha1_discard_unsafe(struct git_hash_ctx *ctx)\n+{\n+\tgit_SHA1_Discard_unsafe(&ctx->state.sha1_unsafe);\n+}\n+\n static void git_hash_sha256_init(struct git_hash_ctx *ctx)\n {\n \tctx->algop = unsafe_hash_algo(&hash_algos[GIT_HASH_SHA256]);\n@@ -135,6 +145,11 @@ static void git_hash_sha256_final_oid(struct object_id *oid, struct git_hash_ctx\n \toid->algo = GIT_HASH_SHA256;\n }\n \n+static void git_hash_sha256_discard(struct git_hash_ctx *ctx)\n+{\n+\tgit_SHA256_Discard(&ctx->state.sha256);\n+}\n+\n static void git_hash_unknown_init(struct git_hash_ctx *ctx UNUSED)\n {\n \tBUG(\"trying to init unknown hash\");\n@@ -165,6 +180,11 @@ static void git_hash_unknown_final_oid(struct object_id *oid UNUSED,\n \tBUG(\"trying to finalize unknown hash\");\n }\n \n+static void git_hash_unknown_discard(struct git_hash_ctx *ctx UNUSED)\n+{\n+\tBUG(\"trying to discard unknown hash\");\n+}\n+\n static const struct git_hash_algo sha1_unsafe_algo = {\n \t.name = \"sha1\",\n \t.format_id = GIT_SHA1_FORMAT_ID,\n@@ -176,6 +196,7 @@ static const struct git_hash_algo sha1_unsafe_algo = {\n \t.update_fn = git_hash_sha1_update_unsafe,\n \t.final_fn = git_hash_sha1_final_unsafe,\n \t.final_oid_fn = git_hash_sha1_final_oid_unsafe,\n+\t.discard_fn = git_hash_sha1_discard_unsafe,\n \t.empty_tree = &empty_tree_oid,\n \t.empty_blob = &empty_blob_oid,\n \t.null_oid = &null_oid_sha1,\n@@ -193,6 +214,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t\t.update_fn = git_hash_unknown_update,\n \t\t.final_fn = git_hash_unknown_final,\n \t\t.final_oid_fn = git_hash_unknown_final_oid,\n+\t\t.discard_fn = git_hash_unknown_discard,\n \t\t.empty_tree = NULL,\n \t\t.empty_blob = NULL,\n \t\t.null_oid = NULL,\n@@ -208,6 +230,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t\t.update_fn = git_hash_sha1_update,\n \t\t.final_fn = git_hash_sha1_final,\n \t\t.final_oid_fn = git_hash_sha1_final_oid,\n+\t\t.discard_fn = git_hash_sha1_discard,\n \t\t.unsafe = &sha1_unsafe_algo,\n \t\t.empty_tree = &empty_tree_oid,\n \t\t.empty_blob = &empty_blob_oid,\n@@ -224,6 +247,7 @@ const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n \t\t.update_fn = git_hash_sha256_update,\n \t\t.final_fn = git_hash_sha256_final,\n \t\t.final_oid_fn = git_hash_sha256_final_oid,\n+\t\t.discard_fn = git_hash_sha256_discard,\n \t\t.empty_tree = &empty_tree_oid_sha256,\n \t\t.empty_blob = &empty_blob_oid_sha256,\n \t\t.null_oid = &null_oid_sha256,\n@@ -285,14 +309,7 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)\n \n void git_hash_discard(struct git_hash_ctx *ctx)\n {\n-\t/*\n-\t * XXX Many implementations do not need to do anything here,\n-\t * and a dummy final() call is wasteful. But we can't fix\n-\t * that unless our implementation API exposes a discard\n-\t * primitive.\n-\t */\n-\tunsigned char dummy[GIT_MAX_RAWSZ];\n-\tgit_hash_final(dummy, ctx);\n+\tctx->algop->discard_fn(ctx);\n }\n \n uint32_t hash_algo_by_name(const char *name)\ndiff --git a/hash.h b/hash.h\nindex 6b2f04e2a4..0a23ef4dfd 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -37,6 +37,7 @@\n #    define platform_SHA1_Clone_unsafe openssl_SHA1_Clone\n #    define platform_SHA1_Update_unsafe openssl_SHA1_Update\n #    define platform_SHA1_Final_unsafe openssl_SHA1_Final\n+#    define platform_SHA1_Discard_unsafe openssl_SHA1_Discard\n #  else\n #    define platform_SHA_CTX_unsafe SHA_CTX\n #    define platform_SHA1_Init_unsafe SHA1_Init\n@@ -92,6 +93,7 @@\n #  define platform_SHA1_Final_unsafe   platform_SHA1_Final\n #  ifdef platform_SHA1_Clone\n #    define platform_SHA1_Clone_unsafe platform_SHA1_Clone\n+#    define platform_SHA1_Discard_unsafe platform_SHA1_Discard\n #  endif\n #  ifdef SHA1_NEEDS_CLONE_HELPER\n #    define SHA1_NEEDS_CLONE_HELPER_UNSAFE\n@@ -110,9 +112,11 @@\n \n #ifdef platform_SHA1_Clone\n #define git_SHA1_Clone\tplatform_SHA1_Clone\n+#define git_SHA1_Discard platform_SHA1_Discard\n #endif\n #ifdef platform_SHA1_Clone_unsafe\n #  define git_SHA1_Clone_unsafe platform_SHA1_Clone_unsafe\n+#  define git_SHA1_Discard_unsafe platform_SHA1_Discard_unsafe\n #endif\n \n #ifndef platform_SHA256_CTX\n@@ -129,6 +133,7 @@\n \n #ifdef platform_SHA256_Clone\n #define git_SHA256_Clone\tplatform_SHA256_Clone\n+#define git_SHA256_Discard\tplatform_SHA256_Discard\n #endif\n \n #ifdef SHA1_MAX_BLOCK_SIZE\n@@ -142,20 +147,32 @@ static inline void git_SHA1_Clone(git_SHA_CTX *dst, const git_SHA_CTX *src)\n {\n \tmemcpy(dst, src, sizeof(*dst));\n }\n+static inline void git_SHA1_Discard(git_SHA_CTX *ctx UNUSED)\n+{\n+\t/* noop */\n+}\n #endif\n #ifndef SHA1_NEEDS_CLONE_HELPER_UNSAFE\n static inline void git_SHA1_Clone_unsafe(git_SHA_CTX_unsafe *dst,\n \t\t\t\t       const git_SHA_CTX_unsafe *src)\n {\n \tmemcpy(dst, src, sizeof(*dst));\n }\n+static inline void git_SHA1_Discard_unsafe(git_SHA_CTX_unsafe *ctx UNUSED)\n+{\n+\t/* noop */\n+}\n #endif\n \n #ifndef SHA256_NEEDS_CLONE_HELPER\n static inline void git_SHA256_Clone(git_SHA256_CTX *dst, const git_SHA256_CTX *src)\n {\n \tmemcpy(dst, src, sizeof(*dst));\n }\n+static inline void git_SHA256_Discard(git_SHA256_CTX *ctx UNUSED)\n+{\n+\t/* noop */\n+}\n #endif\n \n /*\n@@ -271,6 +288,7 @@ typedef void (*git_hash_clone_fn)(struct git_hash_ctx *dst, const struct git_has\n typedef void (*git_hash_update_fn)(struct git_hash_ctx *ctx, const void *in, size_t len);\n typedef void (*git_hash_final_fn)(unsigned char *hash, struct git_hash_ctx *ctx);\n typedef void (*git_hash_final_oid_fn)(struct object_id *oid, struct git_hash_ctx *ctx);\n+typedef void (*git_hash_discard_fn)(struct git_hash_ctx *ctx);\n \n struct git_hash_algo {\n \t/*\n@@ -306,6 +324,9 @@ struct git_hash_algo {\n \t/* The hash finalization function for object IDs. */\n \tgit_hash_final_oid_fn final_oid_fn;\n \n+\t/* Discard an initialized hash without finalizing. */\n+\tgit_hash_discard_fn discard_fn;\n+\n \t/* The OID of the empty tree. */\n \tconst struct object_id *empty_tree;\n \ndiff --git a/sha1/openssl.h b/sha1/openssl.h\nindex 1038af47da..48deeb724a 100644\n--- a/sha1/openssl.h\n+++ b/sha1/openssl.h\n@@ -40,12 +40,18 @@ static inline void openssl_SHA1_Clone(struct openssl_SHA1_CTX *dst,\n \tEVP_MD_CTX_copy_ex(dst->ectx, src->ectx);\n }\n \n+static inline void openssl_SHA1_Discard(struct openssl_SHA1_CTX *ctx)\n+{\n+\tEVP_MD_CTX_free(ctx->ectx);\n+}\n+\n #ifndef platform_SHA_CTX\n #define platform_SHA_CTX openssl_SHA1_CTX\n #define platform_SHA1_Init openssl_SHA1_Init\n #define platform_SHA1_Clone openssl_SHA1_Clone\n #define platform_SHA1_Update openssl_SHA1_Update\n #define platform_SHA1_Final openssl_SHA1_Final\n+#define platform_SHA1_Discard openssl_SHA1_Discard\n #endif\n \n #endif /* SHA1_OPENSSL_H */\ndiff --git a/sha256/gcrypt.h b/sha256/gcrypt.h\nindex 694a2b70a1..d91ffe73d3 100644\n--- a/sha256/gcrypt.h\n+++ b/sha256/gcrypt.h\n@@ -31,10 +31,16 @@ static inline void gcrypt_SHA256_Clone(gcrypt_SHA256_CTX *dst, const gcrypt_SHA2\n \tgcry_md_copy(dst, *src);\n }\n \n+static inline void gcrypt_SHA256_Discard(gcrypt_SHA256_CTX *ctx)\n+{\n+\tgcry_md_close(*ctx);\n+}\n+\n #define platform_SHA256_CTX gcrypt_SHA256_CTX\n #define platform_SHA256_Init gcrypt_SHA256_Init\n #define platform_SHA256_Clone gcrypt_SHA256_Clone\n #define platform_SHA256_Update gcrypt_SHA256_Update\n #define platform_SHA256_Final gcrypt_SHA256_Final\n+#define platform_SHA256_Discard gcrypt_SHA256_Discard\n \n #endif\ndiff --git a/sha256/openssl.h b/sha256/openssl.h\nindex c1083d9491..3d457ca99d 100644\n--- a/sha256/openssl.h\n+++ b/sha256/openssl.h\n@@ -40,10 +40,16 @@ static inline void openssl_SHA256_Clone(struct openssl_SHA256_CTX *dst,\n \tEVP_MD_CTX_copy_ex(dst->ectx, src->ectx);\n }\n \n+static inline void openssl_SHA256_Discard(struct openssl_SHA256_CTX *ctx)\n+{\n+\tEVP_MD_CTX_free(ctx->ectx);\n+}\n+\n #define platform_SHA256_CTX openssl_SHA256_CTX\n #define platform_SHA256_Init openssl_SHA256_Init\n #define platform_SHA256_Clone openssl_SHA256_Clone\n #define platform_SHA256_Update openssl_SHA256_Update\n #define platform_SHA256_Final openssl_SHA256_Final\n+#define platform_SHA256_Discard openssl_SHA256_Discard\n \n #endif /* SHA256_OPENSSL_H */\n-- \n2.55.0.418.g37da59dd42\n"},{"id":"547007","messageId":"xmqqik6xl0fb.fsf@gitster.g","threadId":"65911","inReplyTo":"20260702075744.GA2029434@coredump.intra.peff.net","subject":"Re: [PATCH 1/9] csum-file: drop discard_hashfile()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-02T18:19:04Z","receivedAt":"2026-07-02T18:19:07Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> So now we have two functions, discard_hashfile() and free_hashfile(),\n> and we only need one. Which one do we want to keep?\n>\n> The only difference between them is that the discard variant also closes\n> the descriptors held in the struct. Let's look at the three callers:\n> ...\n> Note that I said \"descriptors\" plural above. Those callers all care\n> about the \"fd\" member of the struct. But discard_hashfile() also closes\n> check_fd. That is only used if the struct is initialized with\n> hashfd_check(), and neither of its two callers call either discard or\n> free (they always \"finalize\" instead). So closing it is irrelevant for\n> the current callers.\n>\n> I think we're better off sticking with the simpler free_hashfile()\n> interface, and the handful of callers can decide how to handle the\n> descriptors themselves.\n\nSonds good.\n\nOur resident naming czar (already Cc'ed) may have preference about\nthe names and word order, though ;-)\n"},{"id":"547010","messageId":"20260702210601.GA2051171@coredump.intra.peff.net","threadId":"65911","inReplyTo":"xmqqik6xl0fb.fsf@gitster.g","subject":"Re: [PATCH 1/9] csum-file: drop discard_hashfile()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-02T21:06:01Z","receivedAt":"2026-07-02T21:06:03Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 11:19:04AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So now we have two functions, discard_hashfile() and free_hashfile(),\n> > and we only need one. Which one do we want to keep?\n> >\n> > The only difference between them is that the discard variant also closes\n> > the descriptors held in the struct. Let's look at the three callers:\n> > ...\n> > Note that I said \"descriptors\" plural above. Those callers all care\n> > about the \"fd\" member of the struct. But discard_hashfile() also closes\n> > check_fd. That is only used if the struct is initialized with\n> > hashfd_check(), and neither of its two callers call either discard or\n> > free (they always \"finalize\" instead). So closing it is irrelevant for\n> > the current callers.\n> >\n> > I think we're better off sticking with the simpler free_hashfile()\n> > interface, and the handful of callers can decide how to handle the\n> > descriptors themselves.\n> \n> Sonds good.\n> \n> Our resident naming czar (already Cc'ed) may have preference about\n> the names and word order, though ;-)\n\nHeh, yes, it should be hashfile_free() but that would require changing\nthe whole interface. We could do that on top, which might also be a good\ntime to do s/free/discard/ without worrying about a subtle behavior\nchange.\n\n-Peff\n"},{"id":"547069","messageId":"akeck67vIBHe8o9C@pks.im","threadId":"65911","inReplyTo":"20260702210601.GA2051171@coredump.intra.peff.net","subject":"Re: [PATCH 1/9] csum-file: drop discard_hashfile()","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-03T11:27:15Z","receivedAt":"2026-07-03T11:27:22Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 05:06:01PM -0400, Jeff King wrote:\n> On Thu, Jul 02, 2026 at 11:19:04AM -0700, Junio C Hamano wrote:\n> \n> > Jeff King <peff@peff.net> writes:\n> > \n> > > So now we have two functions, discard_hashfile() and free_hashfile(),\n> > > and we only need one. Which one do we want to keep?\n> > >\n> > > The only difference between them is that the discard variant also closes\n> > > the descriptors held in the struct. Let's look at the three callers:\n> > > ...\n> > > Note that I said \"descriptors\" plural above. Those callers all care\n> > > about the \"fd\" member of the struct. But discard_hashfile() also closes\n> > > check_fd. That is only used if the struct is initialized with\n> > > hashfd_check(), and neither of its two callers call either discard or\n> > > free (they always \"finalize\" instead). So closing it is irrelevant for\n> > > the current callers.\n> > >\n> > > I think we're better off sticking with the simpler free_hashfile()\n> > > interface, and the handful of callers can decide how to handle the\n> > > descriptors themselves.\n> > \n> > Sonds good.\n> > \n> > Our resident naming czar (already Cc'ed) may have preference about\n> > the names and word order, though ;-)\n> \n> Heh, yes, it should be hashfile_free() but that would require changing\n> the whole interface. We could do that on top, which might also be a good\n> time to do s/free/discard/ without worrying about a subtle behavior\n> change.\n\nHeh :P\n\nI think this being called a \"free\" function makes perfect sense, because\nultimately that's all we do here. So the semantics align with other free\nfunctions.\n\nWe could of course fix the ordering while at it, but I don't want to\ntack that onto this series. It already makes the codebase a better\nplace, so I'm happy enough.\n\nPatrick\n"},{"id":"547070","messageId":"akecmzUCO7RyrQcO@pks.im","threadId":"65911","inReplyTo":"20260702075953.GB2029434@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] hash: add discard primitive","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-03T11:27:23Z","receivedAt":"2026-07-03T11:27:29Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 03:59:53AM -0400, Jeff King wrote:\n> diff --git a/hash.c b/hash.c\n> index e925b9754e..63672a3d22 100644\n> --- a/hash.c\n> +++ b/hash.c\n> @@ -283,6 +283,18 @@ void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)\n>  \tctx->algop->final_oid_fn(oid, ctx);\n>  }\n>  \n> +void git_hash_discard(struct git_hash_ctx *ctx)\n\nAs the resident naming czar: shouldn't this rather be called\n`git_hash_release()`?\n\nPatrick\n"},{"id":"547071","messageId":"akecoptZrCq1PcFV@pks.im","threadId":"65911","inReplyTo":"20260702080319.GD2029434@coredump.intra.peff.net","subject":"Re: [PATCH 4/9] csum-file: provide a function to release checkpoints","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-03T11:27:30Z","receivedAt":"2026-07-03T11:27:35Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 04:03:19AM -0400, Jeff King wrote:\n> A hashfile_checkpoint struct is basically just a copy of the hash_ctx\n> state at a given point in the file. As such, it contains its own\n> git_hash_ctx which may (depending on the underlying hash implementation)\n> need to be discarded when we're done with it.\n> \n> Let's add a \"release\" function which cleans up the hash context it\n> holds. I chose \"release\" here and not \"discard\" because you'd use this\n> to clean up every checkpoint, whether you used it or not. As opposed to\n> git_hash_discard(), which is needed only if you didn't call\n> git_hash_final().\n\nOkay, I was wondering about that a bit. With this explanation I'm also\nsomewhat fine with the `git_hash_discard()` name. It's still a function\nthat has release semantics, but you want to convey more intent than\nthat.\n\nOne thing I was wondering: is it safe to have a `git_hash_discard()`\nthat is being called on a potentially-already-discarded hash? If so, we\nwouldn't have to discern whether the hash context was used successfully\nor not.\n\nPatrick\n"},{"id":"547072","messageId":"akecqPq4F702E8Cq@pks.im","threadId":"65911","inReplyTo":"20260702080707.GG2029434@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-03T11:27:36Z","receivedAt":"2026-07-03T11:27:44Z","isPatch":true,"body":"On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:\n> The flag handling could be removed if the hash-discard function were\n> idempotent. This could be done easily-ish by having the underlying\n> hash functions (like the ones in sha256/openssl.h) set the context\n> pointer to NULL after free-ing. But it's something that every platform\n> implementation would have to remember to do, and the benefit for the\n> callers is not that huge (it would let us shave a few lines here and\n> probably in a few other spots).\n\nThis answers an earlier question of mine. It would indeed be great if it\nwas idempotent -- I've been bitten by interfaces like this once too\nmuch, where you have to be very careful to manage the lifetime of a\nspecific object. The prime example of this are (were? I don't quite\nrecall whether we fixed that interface) reference transactions, and that\ncaused a bunch of bugs in the past.\n\nPatrick\n"},{"id":"547094","messageId":"ake9Wng-Q9p_sf_H@fruit.crustytoothpaste.net","threadId":"65911","inReplyTo":"akecqPq4F702E8Cq@pks.im","subject":"Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-07-03T13:47:06Z","receivedAt":"2026-07-03T13:47:08Z","isPatch":true,"body":"On 2026-07-03 at 11:27:36, Patrick Steinhardt wrote:\n> On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:\n> > The flag handling could be removed if the hash-discard function were\n> > idempotent. This could be done easily-ish by having the underlying\n> > hash functions (like the ones in sha256/openssl.h) set the context\n> > pointer to NULL after free-ing. But it's something that every platform\n> > implementation would have to remember to do, and the benefit for the\n> > callers is not that huge (it would let us shave a few lines here and\n> > probably in a few other spots).\n> \n> This answers an earlier question of mine. It would indeed be great if it\n> was idempotent -- I've been bitten by interfaces like this once too\n> much, where you have to be very careful to manage the lifetime of a\n> specific object. The prime example of this are (were? I don't quite\n> recall whether we fixed that interface) reference transactions, and that\n> caused a bunch of bugs in the past.\n\nYes, that would be fantastic.  The Rust code will need a few fixes as\nwell (which I will send on top of this one when it's picked up) and it\nreally simplifies our Drop implementation if I can just do\n`git_hash_discard`.  Otherwise, I need to keep track of whether we've\nalready called one of the final functions or not to avoid a double free.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"547172","messageId":"20260706000105.GA2301945@coredump.intra.peff.net","threadId":"65911","inReplyTo":"akecqPq4F702E8Cq@pks.im","subject":"Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-06T00:01:05Z","receivedAt":"2026-07-06T00:07:47Z","isPatch":true,"body":"On Fri, Jul 03, 2026 at 01:27:36PM +0200, Patrick Steinhardt wrote:\n\n> On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:\n> > The flag handling could be removed if the hash-discard function were\n> > idempotent. This could be done easily-ish by having the underlying\n> > hash functions (like the ones in sha256/openssl.h) set the context\n> > pointer to NULL after free-ing. But it's something that every platform\n> > implementation would have to remember to do, and the benefit for the\n> > callers is not that huge (it would let us shave a few lines here and\n> > probably in a few other spots).\n> \n> This answers an earlier question of mine. It would indeed be great if it\n> was idempotent -- I've been bitten by interfaces like this once too\n> much, where you have to be very careful to manage the lifetime of a\n> specific object. The prime example of this are (were? I don't quite\n> recall whether we fixed that interface) reference transactions, and that\n> caused a bunch of bugs in the past.\n\nThere are three tricky points I found while thinking about this.\n\nFirst: how and when do we decide to skip a discard? For most\nimplementations (like sha1dc), it's always a noop, and we can ignore it.\nFor OpenSSL, we could be setting ctx->ectx to NULL. But for gcrypt we\ntypedef their opaque structure directly. So we'd have to push that down\ninto its own struct and add an \"active\" flag.\n\nThat's not too bad, but it does put the burden on each backend. Instead,\nwe could keep a flag in the top-level ctx like this:\n\ndiff --git a/hash.c b/hash.c\nindex 55d1d41770..f4c451b20a 100644\n--- a/hash.c\n+++ b/hash.c\n@@ -285,6 +285,7 @@ void git_hash_free(struct git_hash_ctx *ctx)\n void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)\n {\n \talgop->init_fn(ctx);\n+\tctx->active = 1;\n }\n \n void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)\n@@ -300,16 +301,19 @@ void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)\n void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)\n {\n \tctx->algop->final_fn(hash, ctx);\n+\tctx->active = 0;\n }\n \n void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)\n {\n \tctx->algop->final_oid_fn(oid, ctx);\n+\tctx->active = 0;\n }\n \n void git_hash_discard(struct git_hash_ctx *ctx)\n {\n-\tctx->algop->discard_fn(ctx);\n+\tif (ctx->active)\n+\t\tctx->algop->discard_fn(ctx);\n }\n \n uint32_t hash_algo_by_name(const char *name)\ndiff --git a/hash.h b/hash.h\nindex 0a23ef4dfd..2840f20793 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -281,6 +281,7 @@ struct git_hash_ctx {\n \t\tgit_SHA_CTX_unsafe sha1_unsafe;\n \t\tgit_SHA256_CTX sha256;\n \t} state;\n+\tbool active;\n };\n \n typedef void (*git_hash_init_fn)(struct git_hash_ctx *ctx);\n\n\nThat nicely puts the responsibility in a single place, but now we have\nthe opposite problem: what if somebody calls algop->final_fn() directly?\nThen the flag gets out of sync with the underlying state. There are two\nsuch calls currently, in submodule--helper.c and test-synthesize.c.\nAFAICT there is no reason they could not just use git_hash_final().\nMaybe it would be enough to fix that spot and comment the algo function\npointers to warn people away from using them directly.\n\nThat by itself is enough to make:\n\n  algo->init_fn(&ctx);\n  git_hash_update(&ctx, ...);\n  git_hash_final(out, &ctx);\n  ...\n  git_hash_discard(&ctx);\n\nsafe.\n\n\nThe second issue is related: what should we do in other functions when\nthe active flag is not set? For example, what should this do:\n\n  algo->init_fn(&ctx);\n\n  git_hash_update(&ctx, ...);\n  git_hash_final(out, &ctx);\n\n  git_hash_update(&ctx, ...);\n  git_hash_final(out, &ctx);\n\nIn the second git_hash_update() call, there are two obvious options:\n\n  1. It should do nothing; there is no active context to add to.\n\n  2. It should automatically re-init the context (using the algo from\n     the previous init) and add the data.\n\nThe second final() call has the added bonus that it returns data, but I\nthink there are two matching options:\n\n  1. It should do nothing, and hashclr() the output (leaving it\n     uninitialized just seems insane).\n\n  2. It should automatically re-init the context (assuming there was not\n     already an update() call that did so). And then I guess return\n     whatever hash that particular algo generates for the empty string?\n\nThose all seem reasonable-ish to me and give a defined output at every\nmoment (which is better than crashing). But it kind of feels like they'd\nbe papering over potential bugs. Maybe crashing _is_ better (we don't do\nso reliably now, but a BUG() could make sense).\n\n\nAnd the third is related: do we check the active flag when initializing?\nRight now the answer must be \"no\", because the point of the init\nfunction is that the input is potentially garbage. But that means\nsomething like:\n\n  struct git_hash_ctx ctx;\n  algo->init_fn(&ctx);\n  algo->init_fn(&ctx);\n\nleaks. That's maybe OK in practice. We could do something more like:\n\n  struct git_hash_ctx = HASH_CTX_INIT;\n  git_hash_start(&ctx, algo);\n\nwhere the INIT step doesn't actually allocate anything, and start() is\nthe moment where you must promise to call final() or discard(). And then\nit would be OK for start() to BUG() when the active flag is already set.\n\n\nThat was maybe more than you wanted to read about the topic. But if the\nrequest is for safer object lifetimes in general, then I think there are\na lot of details about what that means.\n\nIf we are going to do anything, I'd be inclined to stop mostly after the\ndiff I showed above. That's the only thing I've seen that would simplify\nexisting code. The rest are mostly hypotheticals, but since Rust was\nmentioned, I wondered if you're trying to shoot for something safer.\n\nAt any rate, I would prefer to do any of this on top of the series I\nposted. I took care there to avoid double-calling final()/discard(),\nwhich could now be simplified away. But I think I'd rather see that\nsimplification its own step.\n\n-Peff\n"},{"id":"547175","messageId":"20260706004401.GA2308672@coredump.intra.peff.net","threadId":"65911","inReplyTo":"20260706000105.GA2301945@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-06T00:44:01Z","receivedAt":"2026-07-06T00:44:02Z","isPatch":true,"body":"On Sun, Jul 05, 2026 at 08:01:05PM -0400, Jeff King wrote:\n\n> That by itself is enough to make:\n> \n>   algo->init_fn(&ctx);\n>   git_hash_update(&ctx, ...);\n>   git_hash_final(out, &ctx);\n>   ...\n>   git_hash_discard(&ctx);\n> \n> safe.\n\nActually, that's not quite true. Setting the active flag happens in\ngit_hash_init() in that model. But many callers use algo->init_fn()\ndirectly instead. They'd all need to be adjusted to use git_hash_init().\nI don't think there's any reason they should avoid it, and it's mostly\nfrom inertia that they use the bare function pointer.\n\n-Peff\n"},{"id":"547185","messageId":"aktIIKuReMxJmDsi@pks.im","threadId":"65911","inReplyTo":"20260706000105.GA2301945@coredump.intra.peff.net","subject":"Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-07-06T06:16:00Z","receivedAt":"2026-07-06T06:16:07Z","isPatch":true,"body":"On Sun, Jul 05, 2026 at 08:01:05PM -0400, Jeff King wrote:\n> On Fri, Jul 03, 2026 at 01:27:36PM +0200, Patrick Steinhardt wrote:\n> > On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:\n[snip]\n> The second issue is related: what should we do in other functions when\n> the active flag is not set? For example, what should this do:\n> \n>   algo->init_fn(&ctx);\n> \n>   git_hash_update(&ctx, ...);\n>   git_hash_final(out, &ctx);\n> \n>   git_hash_update(&ctx, ...);\n>   git_hash_final(out, &ctx);\n> \n> In the second git_hash_update() call, there are two obvious options:\n> \n>   1. It should do nothing; there is no active context to add to.\n> \n>   2. It should automatically re-init the context (using the algo from\n>      the previous init) and add the data.\n\nOr 3rd: we `BUG()` when any of the functions is called on an\nuninitialized context. That to me feels like the most sensible solution.\n\n> The second final() call has the added bonus that it returns data, but I\n> think there are two matching options:\n> \n>   1. It should do nothing, and hashclr() the output (leaving it\n>      uninitialized just seems insane).\n> \n>   2. It should automatically re-init the context (assuming there was not\n>      already an update() call that did so). And then I guess return\n>      whatever hash that particular algo generates for the empty string?\n> \n> Those all seem reasonable-ish to me and give a defined output at every\n> moment (which is better than crashing). But it kind of feels like they'd\n> be papering over potential bugs. Maybe crashing _is_ better (we don't do\n> so reliably now, but a BUG() could make sense).\n\nYes, agreed.\n\n> And the third is related: do we check the active flag when initializing?\n> Right now the answer must be \"no\", because the point of the init\n> function is that the input is potentially garbage. But that means\n> something like:\n> \n>   struct git_hash_ctx ctx;\n>   algo->init_fn(&ctx);\n>   algo->init_fn(&ctx);\n> \n> leaks. That's maybe OK in practice. We could do something more like:\n> \n>   struct git_hash_ctx = HASH_CTX_INIT;\n>   git_hash_start(&ctx, algo);\n> \n> where the INIT step doesn't actually allocate anything, and start() is\n> the moment where you must promise to call final() or discard(). And then\n> it would be OK for start() to BUG() when the active flag is already set.\n\nI'd say being as strict as possible is the best way to go until we find\na case where it makes sense to be less strict.\n\n> That was maybe more than you wanted to read about the topic. But if the\n> request is for safer object lifetimes in general, then I think there are\n> a lot of details about what that means.\n> \n> If we are going to do anything, I'd be inclined to stop mostly after the\n> diff I showed above. That's the only thing I've seen that would simplify\n> existing code. The rest are mostly hypotheticals, but since Rust was\n> mentioned, I wondered if you're trying to shoot for something safer.\n> \n> At any rate, I would prefer to do any of this on top of the series I\n> posted. I took care there to avoid double-calling final()/discard(),\n> which could now be simplified away. But I think I'd rather see that\n> simplification its own step.\n\nFully agreed.\n\nThanks!\n\nPatrick\n"}]}