| Message ID | 0104cd9c763aee220a2df357834c79b10695ee35.1593115455.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make oid_to_hex() thread-safe | expand |
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)
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
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.
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> > ---
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)
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(-)