Re: [PATCH 7/9] http: discard hash in dumb-http http_object_request

[Jeff King <peff@peff.net>]

On Fri, Jul 03, 2026 at 01:27:36PM +0200, Patrick Steinhardt wrote:

> On Thu, Jul 02, 2026 at 04:07:07AM -0400, Jeff King wrote:
> > 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).
> 
> This answers an earlier question of mine. It would indeed be great if it
> was idempotent -- I've been bitten by interfaces like this once too
> much, where you have to be very careful to manage the lifetime of a
> specific object. The prime example of this are (were? I don't quite
> recall whether we fixed that interface) reference transactions, and that
> caused a bunch of bugs in the past.

There are three tricky points I found while thinking about this.

First: how and when do we decide to skip a discard? For most
implementations (like sha1dc), it's always a noop, and we can ignore it.
For OpenSSL, we could be setting ctx->ectx to NULL. But for gcrypt we
typedef their opaque structure directly. So we'd have to push that down
into its own struct and add an "active" flag.

That's not too bad, but it does put the burden on each backend. Instead,
we could keep a flag in the top-level ctx like this:

diff --git a/hash.c b/hash.c
index 55d1d41770..f4c451b20a 100644
--- a/hash.c
+++ b/hash.c
@@ -285,6 +285,7 @@ void git_hash_free(struct git_hash_ctx *ctx)
 void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
 {
 	algop->init_fn(ctx);
+	ctx->active = 1;
 }
 
 void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
@@ -300,16 +301,19 @@ void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
 void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
 {
 	ctx->algop->final_fn(hash, ctx);
+	ctx->active = 0;
 }
 
 void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
 {
 	ctx->algop->final_oid_fn(oid, ctx);
+	ctx->active = 0;
 }
 
 void git_hash_discard(struct git_hash_ctx *ctx)
 {
-	ctx->algop->discard_fn(ctx);
+	if (ctx->active)
+		ctx->algop->discard_fn(ctx);
 }
 
 uint32_t hash_algo_by_name(const char *name)
diff --git a/hash.h b/hash.h
index 0a23ef4dfd..2840f20793 100644
--- a/hash.h
+++ b/hash.h
@@ -281,6 +281,7 @@ struct git_hash_ctx {
 		git_SHA_CTX_unsafe sha1_unsafe;
 		git_SHA256_CTX sha256;
 	} state;
+	bool active;
 };
 
 typedef void (*git_hash_init_fn)(struct git_hash_ctx *ctx);


That nicely puts the responsibility in a single place, but now we have
the opposite problem: what if somebody calls algop->final_fn() directly?
Then the flag gets out of sync with the underlying state. There are two
such calls currently, in submodule--helper.c and test-synthesize.c.
AFAICT there is no reason they could not just use git_hash_final().
Maybe it would be enough to fix that spot and comment the algo function
pointers to warn people away from using them directly.

That by itself is enough to make:

  algo->init_fn(&ctx);
  git_hash_update(&ctx, ...);
  git_hash_final(out, &ctx);
  ...
  git_hash_discard(&ctx);

safe.


The second issue is related: what should we do in other functions when
the active flag is not set? For example, what should this do:

  algo->init_fn(&ctx);

  git_hash_update(&ctx, ...);
  git_hash_final(out, &ctx);

  git_hash_update(&ctx, ...);
  git_hash_final(out, &ctx);

In the second git_hash_update() call, there are two obvious options:

  1. It should do nothing; there is no active context to add to.

  2. It should automatically re-init the context (using the algo from
     the previous init) and add the data.

The second final() call has the added bonus that it returns data, but I
think there are two matching options:

  1. It should do nothing, and hashclr() the output (leaving it
     uninitialized just seems insane).

  2. It should automatically re-init the context (assuming there was not
     already an update() call that did so). And then I guess return
     whatever hash that particular algo generates for the empty string?

Those all seem reasonable-ish to me and give a defined output at every
moment (which is better than crashing). But it kind of feels like they'd
be papering over potential bugs. Maybe crashing _is_ better (we don't do
so reliably now, but a BUG() could make sense).


And the third is related: do we check the active flag when initializing?
Right now the answer must be "no", because the point of the init
function is that the input is potentially garbage. But that means
something like:

  struct git_hash_ctx ctx;
  algo->init_fn(&ctx);
  algo->init_fn(&ctx);

leaks. That's maybe OK in practice. We could do something more like:

  struct git_hash_ctx = HASH_CTX_INIT;
  git_hash_start(&ctx, algo);

where the INIT step doesn't actually allocate anything, and start() is
the moment where you must promise to call final() or discard(). And then
it would be OK for start() to BUG() when the active flag is already set.


That was maybe more than you wanted to read about the topic. But if the
request is for safer object lifetimes in general, then I think there are
a lot of details about what that means.

If we are going to do anything, I'd be inclined to stop mostly after the
diff I showed above. That's the only thing I've seen that would simplify
existing code. The rest are mostly hypotheticals, but since Rust was
mentioned, I wondered if you're trying to shoot for something safer.

At any rate, I would prefer to do any of this on top of the series I
posted. I took care there to avoid double-calling final()/discard(),
which could now be simplified away. But I think I'd rather see that
simplification its own step.

-Peff