Re: [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe

[Johannes Schindelin <Johannes.Schindelin@xxxxxx>]

Hi Matheus,

On Fri, 26 Jun 2020, Matheus Tavares wrote:

> @@ -136,12 +137,49 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
>  	return hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);
>  }
>
> +struct hexbuf_array {
> +	int idx;
> +	char bufs[4][GIT_MAX_HEXSZ + 1];
> +};
> +
> +#ifdef HAVE_THREADS
> +static pthread_key_t hexbuf_array_key;
> +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;
> +
> +static void init_hexbuf_array_key(void)
> +{
> +	if (pthread_key_create(&hexbuf_array_key, free))
> +		die(_("failed to initialize threads' key for hash to hex conversion"));
> +}
> +
> +#else
> +static struct hexbuf_array default_hexbuf_array;
> +#endif
> +
>  char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *algop)
>  {
> -	static int bufno;
> -	static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
> -	bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
> -	return hash_to_hex_algop_r(hexbuffer[bufno], hash, algop);
> +	struct hexbuf_array *ha;
> +
> +#ifdef HAVE_THREADS
> +	void *value;
> +
> +	if (pthread_once(&hexbuf_array_once, init_hexbuf_array_key))
> +		die(_("failed to initialize threads' key for hash to hex conversion"));
> +
> +	value = pthread_getspecific(hexbuf_array_key);
> +	if (value) {
> +		ha = (struct hexbuf_array *) value;
> +	} else {
> +		ha = xmalloc(sizeof(*ha));

I just realized (while trying to debug something independent) that this
leaves `ha->idx` uninitialized. So you will need at least this patch to
fix a bug that currently haunts `seen`'s CI builds (you can use
`--valgrind`, like I did, to identify the problem):

-- snip --
diff --git a/hex.c b/hex.c
index 4f2f163d5e7..365ba94ab11 100644
--- a/hex.c
+++ b/hex.c
@@ -171,6 +171,7 @@ char *hash_to_hex_algop(const unsigned char *hash, const struct git_hash_algo *a
 		ha = (struct hexbuf_array *) value;
 	} else {
 		ha = xmalloc(sizeof(*ha));
+		ha->idx = 0;
 		if (pthread_setspecific(hexbuf_array_key, (void *)ha))
 			die(_("failed to set thread buffer for hash to hex conversion"));
 	}
-- snap --

But as I mentioned before, I would be much more in favor of abandoning
this thread-local idea (because it is _still_ fragile, as the same thread
could try to make use of more than four hex values in the same `printf()`,
for example) and instead using Coccinelle to convert all those
`oid_to_hex()` calls to `oid_to_hex_r()` calls.

Now, I am _far_ from knowing what I'm doing with Coccinelle, but I think
this here semantic patch should get you going:

-- snipsnap --
@@
expression E;
@@
  {
++   char hex[GIT_MAX_HEXSZ + 1];
     ...
-    oid_to_hex(E)
+    oid_to_hex_r(hex, E)
     ...
  }

@@
expression E1, E2;
@@
  {
++   char hex1[GIT_MAX_HEXSZ + 1], hex2[GIT_MAX_HEXSZ + 1];
     ...
-    oid_to_hex(E1)
+    oid_to_hex_r(hex1, E1)
     ...
-    oid_to_hex(E2)
+    oid_to_hex_r(hex2, E2)
     ...
  }