Re: [PATCH 7/7] hash: check ctx->active flag in all wrapper functions

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> It only makes sense to call git_hash_update(), etc, on a hash context
> that has been initialized but not yet finalized or discarded. This is an
> unlikely error to make, but it's easy for us to catch it and complain.
>
> It's especially important because it would quietly "work" for many hash
> backends (like sha1dc, which is just manipulating some bytes) but would
> cause undefined behavior with others (like OpenSSL, which puts the
> context onto the heap). Checking the flag lets us catch problems
> consistently on every build.
>
> Note that we can't do the same for git_init_hash(). Even though it would
> cause a leak to call it twice (without an intervening final/discard),
> the point of the function is that the contents of the struct are
> undefined before the call. But calling it twice is an even less likely
> error to make, so not covering it is OK.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  hash.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Among the four we see here, I agree that calling _clone and _update
on an already discarded or finalized context should be caught as an
error. As I alluded to earlier, though, I am not sure about
_final. The asymmetry in a design that allows _discard after _final
but not _final after _final disturbs me slightly, but perhaps that
is only because my morning caffeine has not yet kicked in. 

>
> diff --git a/hash.c b/hash.c
> index b1296f0018..82f7e24404 100644
> --- a/hash.c
> +++ b/hash.c
> @@ -290,22 +290,32 @@ void git_hash_init(struct git_hash_ctx *ctx, const struct git_hash_algo *algop)
>  
>  void git_hash_clone(struct git_hash_ctx *dst, const struct git_hash_ctx *src)
>  {
> +	if (!src->active)
> +		BUG("attempt to copy from an inactive hash context");
> +	if (!dst->active)
> +		BUG("attempt to copy to an inactive hash context");
>  	src->algop->clone_fn(dst, src);
>  }
>  
>  void git_hash_update(struct git_hash_ctx *ctx, const void *in, size_t len)
>  {
> +	if (!ctx->active)
> +		BUG("attempt to update an inactive hash context");
>  	ctx->algop->update_fn(ctx, in, len);
>  }
>  
>  void git_hash_final(unsigned char *hash, struct git_hash_ctx *ctx)
>  {
> +	if (!ctx->active)
> +		BUG("attempt to finalize an inactive hash context");
>  	ctx->algop->final_fn(hash, ctx);
>  	ctx->active = false;
>  }
>  
>  void git_hash_final_oid(struct object_id *oid, struct git_hash_ctx *ctx)
>  {
> +	if (!ctx->active)
> +		BUG("attempt to finalize an inactive hash context");
>  	ctx->algop->final_oid_fn(oid, ctx);
>  	ctx->active = false;
>  }