Re-work and bugfix the concurrent use of symmetric cipher handles.

The previous implementation was not thread safe.
Now use pthread keys to create a handle for each object in each
thread, and then retrieve them in the same thread.

src/vmod_gcrypt.c

@@ -32,6 +32,8 @@
 
 #include <gcrypt.h>
 #include <string.h>
+#include <pthread.h>
+#include <errno.h>
 
 #include "vcl.h"
 #include "cache/cache.h"
@@ -63,11 +65,14 @@
 struct vmod_gcrypt_symmetric {
 	unsigned		magic;
 #define VMOD_GCRYPT_SYMMETRIC_MAGIC 0x82c7ffe2
-	gcry_cipher_hd_t	hd;
+	pthread_key_t		hdk;
 	char			*vcl_name;
-	size_t			blocklen;
+	void			*key;
+	size_t			keylen;
 	enum padding		padding;
+	int			algo;
 	int			mode;
+	unsigned int		flags;
 };
 
 static const char *gcrypt_version = NULL;
@@ -215,6 +220,18 @@ vmod_init(VRT_CTX, VCL_ENUM cmd, VCL_BYTES n)
 
 /* Object symmetric */
 
+/* Destructor function for cipher handles pointed to by pthread keys */
+static void
+hdk_fini(void *p)
+{
+	gcry_cipher_hd_t *hd;
+
+	AN(p);
+	hd = (gcry_cipher_hd_t *)p;
+	gcry_cipher_close(*hd);
+	free(p);
+}
+
 VCL_VOID
 vmod_symmetric__init(VRT_CTX, struct vmod_gcrypt_symmetric **symmetricp,
 		     const char *vcl_name, VCL_ENUM ciphers, VCL_ENUM modes,
@@ -228,6 +245,7 @@ vmod_symmetric__init(VRT_CTX, struct vmod_gcrypt_symmetric **symmetricp,
 	enum padding padding = _MAX_PADDING;
 	gcry_error_t err = GPG_ERR_NO_ERROR;
 	size_t len;
+	pthread_key_t hdk;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	AN(symmetricp);
@@ -269,7 +287,7 @@ vmod_symmetric__init(VRT_CTX, struct vmod_gcrypt_symmetric **symmetricp,
 		     len, ciphers, vcl_name);
 		return;
 	}
-	if ((len = gcry_cipher_get_algo_blklen(algo)) == 0) {
+	if (gcry_cipher_get_algo_blklen(algo) == 0) {
 		VERR(ctx, "Cannot determine block length for %s cipher in %s "
 		     "constructor", ciphers, vcl_name);
 		return;
@@ -294,28 +312,58 @@ vmod_symmetric__init(VRT_CTX, struct vmod_gcrypt_symmetric **symmetricp,
 	if (cbc_cts)
 		flags |= GCRY_CIPHER_CBC_CTS;
 
+	/* Create a handle to test for errors */
 	if ((err = gcry_cipher_open(&hd, algo, mode, flags))
 	    != GPG_ERR_NO_ERROR) {
 		VERR(ctx, "Cannot open cipher in %s constructor: %s/%s",
 		     vcl_name, gcry_strsource(err), gcry_strerror(err));
 		return;
 	}
-	if ((err = gcry_cipher_setkey(hd, key->priv, key->len))
-	    != GPG_ERR_NO_ERROR) {
+	err = gcry_cipher_setkey(hd, key->priv, key->len);
+	gcry_cipher_close(hd);
+	if (err != GPG_ERR_NO_ERROR) {
 		VERR(ctx, "Cannot set key in %s constructor: %s/%s",
 		     vcl_name, gcry_strsource(err), gcry_strerror(err));
 		return;
 	}
 
+	if (pthread_key_create(&hdk, hdk_fini) != 0) {
+		if (errno == EAGAIN)
+			VERR(ctx, "pthread key allocation exhausted "
+			     "(PTHREAD_KEYS_MAX=%d) in %s constructor, "
+			     "discard old VCL instances or restart Varnish",
+			     PTHREAD_KEYS_MAX, vcl_name);
+		else
+			VERR(ctx, "unknown error creating pthread key in %s "
+			     "constructor (errno=%d, %s)", vcl_name, errno,
+			     strerror(errno));
+		return;
+	}
+
 	ALLOC_OBJ(symmetric, VMOD_GCRYPT_SYMMETRIC_MAGIC);
 	AN(symmetric);
 	*symmetricp = symmetric;
-	memcpy(&symmetric->hd, &hd, sizeof(hd));
+	symmetric->hdk = hdk;
+	if (secure)
+		symmetric->key = gcry_malloc_secure(key->len);
+	else
+		symmetric->key = malloc(key->len);
+	if (symmetric->key == NULL) {
+		VERRNOMEM(ctx, "copying key in %s constructor", vcl_name);
+		return;
+	}
 	symmetric->vcl_name = strdup(vcl_name);
-	AN(symmetric->vcl_name);
-	symmetric->blocklen = len;
+	if (symmetric->vcl_name == NULL) {
+		VERRNOMEM(ctx, "copying object name in %s constructor",
+			  vcl_name);
+		return;
+	}
+	memcpy(symmetric->key, key->priv, key->len);
+	symmetric->keylen = key->len;
 	symmetric->padding = padding;
+	symmetric->algo = algo;
 	symmetric->mode = mode;
+	symmetric->flags = flags;
 }
 
 VCL_VOID
@@ -323,28 +371,77 @@ vmod_symmetric__fini(struct vmod_gcrypt_symmetric **symmetricp)
 {
 	struct vmod_gcrypt_symmetric *symmetric;
 
-	if (symmetricp == NULL)
+	if (symmetricp == NULL || *symmetricp == NULL)
 		return;
 	symmetric = *symmetricp;
-	if (symmetric == NULL)
-		return;
 	CHECK_OBJ(symmetric, VMOD_GCRYPT_SYMMETRIC_MAGIC);
+	if (symmetric->key != NULL) {
+		if (symmetric->flags & GCRY_CIPHER_SECURE)
+			gcry_free(symmetric->key);
+		else
+			free(symmetric->key);
+	}
 	if (symmetric->vcl_name != NULL)
 		free(symmetric->vcl_name);
-	gcry_cipher_close(symmetric->hd);
+	AZ(pthread_key_delete(symmetric->hdk));
 	FREE_OBJ(symmetric);
 }
 
+static gcry_cipher_hd_t *
+get_symmetric_hd(VRT_CTX,
+		 const struct vmod_gcrypt_symmetric * const restrict symmetric,
+		 const char * const restrict caller)
+{
+	void *p;
+	gcry_cipher_hd_t *hd;
+	gcry_error_t err = GPG_ERR_NO_ERROR;
+
+	p = pthread_getspecific(symmetric->hdk);
+	if (p != NULL) {
+		hd = (gcry_cipher_hd_t *)p;
+		if ((err = gcry_cipher_reset(*hd)) != GPG_ERR_NO_ERROR) {
+			VERR(ctx, "Cannot reset cipher handle in %s.%s(): "
+			     "%s/%s", symmetric->vcl_name, caller,
+			     gcry_strsource(err), gcry_strerror(err));
+			return NULL;
+		}
+		return hd;
+	}
+
+	hd = malloc(sizeof(hd));
+	if (hd == NULL) {
+		VERRNOMEM(ctx, "Allocating cipher handle in %s.%s()",
+			  symmetric->vcl_name, caller);
+		return NULL;
+	}
+	if ((err = gcry_cipher_open(hd, symmetric->algo, symmetric->mode,
+				    symmetric->flags)) != GPG_ERR_NO_ERROR) {
+		VERR(ctx, "Cannot open cipher in %s.%s(): %s/%s",
+		     symmetric->vcl_name, caller, gcry_strsource(err),
+		     gcry_strerror(err));
+		return NULL;
+	}
+	if ((err = gcry_cipher_setkey(*hd, symmetric->key, symmetric->keylen))
+	    != GPG_ERR_NO_ERROR) {
+		VERR(ctx, "Cannot set key in %s.%s(): %s/%s",
+		     symmetric->vcl_name, caller, gcry_strsource(err),
+		     gcry_strerror(err));
+		return NULL;
+	}
+	pthread_setspecific(symmetric->hdk, (void *)hd);
+	return hd;
+}
+
 VCL_BLOB
 vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		       VCL_BLOB plainblob, VCL_BLOB iv, VCL_BLOB ctr)
 {
-	size_t len;
+	size_t len, blocklen;
 	uintptr_t snap;
 	void *plaintext;
 	struct vmod_priv *ciphertext;
 	gcry_error_t err = GPG_ERR_NO_ERROR;
-	gcry_cipher_hd_t hd;
+	gcry_cipher_hd_t *hd;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(symmetric, VMOD_GCRYPT_SYMMETRIC_MAGIC);
@@ -360,10 +457,11 @@ vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			  symmetric->vcl_name);
 		return NULL;
 	}
+	blocklen = gcry_cipher_get_algo_blklen(symmetric->algo);
+	AN(blocklen);
 	if (symmetric->padding != NONE) {
 		plaintext = (padf[symmetric->padding])(ctx->ws, plainblob->priv,
-						       plainblob->len,
-						       symmetric->blocklen,
+						       plainblob->len, blocklen,
 						       &len);
 		if (plaintext == NULL) {
 			VERRNOMEM(ctx, "Allocating padded plaintext in "
@@ -383,7 +481,11 @@ vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		return NULL;
 	}
 
-	memcpy(&hd, &symmetric->hd, sizeof(hd));
+	hd = get_symmetric_hd(ctx, symmetric, "encrypt");
+	if (hd == NULL) {
+		WS_Reset(ctx->ws, snap);
+		return NULL;
+	}
 	if (need_iv[symmetric->mode]) {
 		if (iv == NULL) {
 			VERR(ctx, "Required initialization vector is NULL in "
@@ -395,7 +497,7 @@ vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		/* A NULL iv of length 0 is permitted. */
 		if (iv->priv == NULL)
 			assert(iv->len == 0);
-		if ((err = gcry_cipher_setiv(hd, iv->priv, iv->len))
+		if ((err = gcry_cipher_setiv(*hd, iv->priv, iv->len))
 		    != GPG_ERR_NO_ERROR) {
 			VERR(ctx, "Cannot set initialization vector in "
 			     "%s.encrypt(): %s/%s", symmetric->vcl_name,
@@ -412,7 +514,7 @@ vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			return NULL;
 		}
 		assert(ctr->len >= 0);
-		if ((err = gcry_cipher_setctr(hd, ctr->priv, ctr->len))
+		if ((err = gcry_cipher_setctr(*hd, ctr->priv, ctr->len))
 		    != GPG_ERR_NO_ERROR) {
 			VERR(ctx, "Cannot set counter vector in %s.encrypt(): "
 			     "%s/%s", symmetric->vcl_name, gcry_strsource(err),
@@ -421,7 +523,7 @@ vmod_symmetric_encrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			return NULL;
 		}
 	}
-	if ((err = gcry_cipher_encrypt(hd, ciphertext->priv, len, plaintext,
+	if ((err = gcry_cipher_encrypt(*hd, ciphertext->priv, len, plaintext,
 				       len))
 	    != GPG_ERR_NO_ERROR) {
 		VERR(ctx, "in %s.encrypt(): %s/%s", symmetric->vcl_name,
@@ -441,7 +543,8 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 	uintptr_t snap;
 	struct vmod_priv *plaintext;
 	gcry_error_t err = GPG_ERR_NO_ERROR;
-	gcry_cipher_hd_t hd;
+	gcry_cipher_hd_t *hd;
+	size_t blocklen;
 
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	CHECK_OBJ_NOTNULL(symmetric, VMOD_GCRYPT_SYMMETRIC_MAGIC);
@@ -467,7 +570,9 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		return NULL;
 	}
 
-	memcpy(&hd, &symmetric->hd, sizeof(hd));
+	hd = get_symmetric_hd(ctx, symmetric, "decrypt");
+	if (hd == NULL)
+		goto fail;
 	if (need_iv[symmetric->mode]) {
 		if (iv == NULL) {
 			VERR(ctx, "Required initialization vector is NULL in "
@@ -478,7 +583,7 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		/* A NULL iv of length 0 is permitted. */
 		if (iv->priv == NULL)
 			assert(iv->len == 0);
-		if ((err = gcry_cipher_setiv(hd, iv->priv, iv->len))
+		if ((err = gcry_cipher_setiv(*hd, iv->priv, iv->len))
 		    != GPG_ERR_NO_ERROR) {
 			VERR(ctx, "Cannot set initialization vector in "
 			     "%s.decrypt(): %s/%s", symmetric->vcl_name,
@@ -493,7 +598,7 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			goto fail;
 		}
 		assert(ctr->len >= 0);
-		if ((err = gcry_cipher_setctr(hd, ctr->priv, ctr->len))
+		if ((err = gcry_cipher_setctr(*hd, ctr->priv, ctr->len))
 		    != GPG_ERR_NO_ERROR) {
 			VERR(ctx, "Cannot set counter vector in %s.decrypt(): "
 			     "%s/%s", symmetric->vcl_name, gcry_strsource(err),
@@ -501,7 +606,7 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 			goto fail;
 		}
 	}
-	if ((err = gcry_cipher_decrypt(hd, plaintext->priv, ciphertext->len,
+	if ((err = gcry_cipher_decrypt(*hd, plaintext->priv, ciphertext->len,
 				       ciphertext->priv, ciphertext->len))
 	    != GPG_ERR_NO_ERROR) {
 		VERR(ctx, "in %s.decrypt(): %s/%s", symmetric->vcl_name,
@@ -509,18 +614,20 @@ vmod_symmetric_decrypt(VRT_CTX, struct vmod_gcrypt_symmetric *symmetric,
 		goto fail;
 	}
 	plaintext->len = ciphertext->len;
+	blocklen = gcry_cipher_get_algo_blklen(symmetric->algo);
+	AN(blocklen);
 	if (symmetric->padding != NONE) {
-		if (plaintext->len % symmetric->blocklen != 0) {
+		if (plaintext->len % blocklen != 0) {
 			VERR(ctx, "in %s.decrypt(): padding is required, but "
 			     "plaintext length %d is not a multiple of block "
 			     "length %d", symmetric->vcl_name, plaintext->len,
-			     symmetric->blocklen);
+			     blocklen);
 			goto fail;
 		}
 		plaintext->len =
 			(unpadlenf[symmetric->padding])(plaintext->priv,
 							ciphertext->len,
-							symmetric->blocklen);
+							blocklen);
 		if (plaintext->len < 0) {
 			VERR(ctx, "in %s.decrypt(): incorrect padding",
 			     symmetric->vcl_name);