[2/2] hex: make hash_to_hex_algop() and friends thread-safe
diff mbox series

Message ID 0104cd9c763aee220a2df357834c79b10695ee35.1593115455.git.matheus.bernardino@usp.br
State New
Headers show
Series
  • Make oid_to_hex() thread-safe
Related show

Commit Message

Matheus Tavares Bernardino June 25, 2020, 8:32 p.m. UTC
hash_to_hex_algop() returns a static buffer, relieving callers from the
responsibility of freeing memory after use. But the current
implementation uses the same static data for all threads and, thus, is
not thread-safe. We could avoid using this function and its wrappers
in threaded code, but they are sometimes too deep in the call stack to
be noticed or even avoided.

For example, we can take a look at the number of oid_to_hex() calls,
which calls hash_to_hex_algop():

$ git grep 'oid_to_hex(' | wc -l
818

Although these functions don't seem to be causing problems out there for
now (at least not reported), making them thread-safe makes the codebase
more robust against race conditions. We can easily do that replicating
the static buffer in each thread's local storage.

Original-patch-by: Fredrik Kuivinen <frekui@gmail.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---
 hex.c | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

Comments

brian m. carlson June 25, 2020, 11:07 p.m. UTC | #1
On 2020-06-25 at 20:32:57, Matheus Tavares wrote:
> +#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_key_getspecific(&hexbuf_array_key);

I think the portable name for this is "pthread_getspecific".  That
appears to be what POSIX specifies and what our Windows pthread compat
code uses.

> +	if (value) {
> +		ha = (struct hexbuf_array *) value;
> +	} else {
> +		ha = xmalloc(sizeof(*ha));
> +		if (pthread_key_setspecific(&hexbuf_array_key, (void *)ha))

Same thing here.

> +			die(_("failed to set thread buffer for hash to hex conversion"));
> +	}
> +#else
> +	ha = &default_hexbuf_array;
> +#endif
> +
> +	ha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);
> +	return hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);
>  }
>  
>  char *hash_to_hex(const unsigned char *hash)
Matheus Tavares Bernardino June 25, 2020, 11:54 p.m. UTC | #2
On Thu, Jun 25, 2020 at 8:08 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2020-06-25 at 20:32:57, Matheus Tavares wrote:
> > +#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_key_getspecific(&hexbuf_array_key);
>
> I think the portable name for this is "pthread_getspecific".  That
> appears to be what POSIX specifies and what our Windows pthread compat
> code uses.

Right, thanks for noticing that!

(I wonder how the Windows build passed successfully in our CI [1]...
Shouldn't the compiler have failed due to a "undefined reference"
error?)

[1]: https://github.com/matheustavares/git/runs/808649609
Matheus Tavares Bernardino June 26, 2020, midnight UTC | #3
On Thu, Jun 25, 2020 at 8:54 PM Matheus Tavares Bernardino
<matheus.bernardino@usp.br> wrote:
>
> On Thu, Jun 25, 2020 at 8:08 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >
> > On 2020-06-25 at 20:32:57, Matheus Tavares wrote:
> > > +#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_key_getspecific(&hexbuf_array_key);
> >
> > I think the portable name for this is "pthread_getspecific".  That
> > appears to be what POSIX specifies and what our Windows pthread compat
> > code uses.
>
> Right, thanks for noticing that!
>
> (I wonder how the Windows build passed successfully in our CI [1]...
> Shouldn't the compiler have failed due to a "undefined reference"
> error?)

Oh, I know why... I forgot to include "thread-utils.h", so
HAVE_THREADS is never defined and this code is always ignored (on both
Windows and Linux).  That's embarrassing... I'll correct that for v2.
Christian Couder June 26, 2020, 6:02 a.m. UTC | #4
On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
<matheus.bernardino@usp.br> wrote:

[...]

> Although these functions don't seem to be causing problems out there for
> now (at least not reported), making them thread-safe makes the codebase
> more robust against race conditions. We can easily do that replicating

Maybe s/that replicating/that by replicating/.

> the static buffer in each thread's local storage.
>
> Original-patch-by: Fredrik Kuivinen <frekui@gmail.com>

If Fredrik gave his "Signed-off-by:", maybe it's better to keep it too.

> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---

Patch
diff mbox series

diff --git a/hex.c b/hex.c
index da51e64929..1094ed25bd 100644
--- a/hex.c
+++ b/hex.c
@@ -136,12 +136,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;
+
+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_key_getspecific(&hexbuf_array_key);
+	if (value) {
+		ha = (struct hexbuf_array *) value;
+	} else {
+		ha = xmalloc(sizeof(*ha));
+		if (pthread_key_setspecific(&hexbuf_array_key, (void *)ha))
+			die(_("failed to set thread buffer for hash to hex conversion"));
+	}
+#else
+	ha = &default_hexbuf_array;
+#endif
+
+	ha->idx = (ha->idx + 1) % ARRAY_SIZE(ha->bufs);
+	return hash_to_hex_algop_r(ha->bufs[ha->idx], hash, algop);
 }
 
 char *hash_to_hex(const unsigned char *hash)