[PATCH 8/9] hash: fix memory leak copying sha256 gcrypt handles

[Jeff King <peff@peff.net>]

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>
---
 sha256/gcrypt.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sha256/gcrypt.h b/sha256/gcrypt.h
index 17a90f1052..694a2b70a1 100644
--- a/sha256/gcrypt.h
+++ b/sha256/gcrypt.h
@@ -27,6 +27,7 @@ static inline void gcrypt_SHA256_Final(unsigned char *digest, gcrypt_SHA256_CTX
 
 static inline void gcrypt_SHA256_Clone(gcrypt_SHA256_CTX *dst, const gcrypt_SHA256_CTX *src)
 {
+	gcry_md_close(*dst);
 	gcry_md_copy(dst, *src);
 }
 
-- 
2.55.0.418.g37da59dd42