[v2,0/2] Make oid_to_hex() thread-safe

Message

Matheus Tavares June 26, 2020, 9:54 p.m. UTC

Some thread-unsafe functions of our codebase appear very down in the
call stack, which can be hard to notice (or avoid). Thus they are
sometimes used in threaded code unsafely. In this series we add
pthread_once() to compat/win32/ and use it in conjunction with
pthread_key to make a subset of the said functions thread-safe.

Changes since v1:

Patch 1:
- Fixed style problems.
- Wrote a more descriptive commit message, with motivation and info
  about where the code was ported from (including commit, license and
  modifications made).
- Corrected usage of InitOnceComplete() in pthread_once(). It was
  previously being called unconditionally, which lead to errors in
  subsequent calls to pthread_once(), after the initialization had
  already succeded.

Patch 2:
- Used the correct pthread_[get|set]specific() functions in hex.c, as
  Brian pointed out.
- Imported thread_utils.h to hex.c (providing the HAVE_THREADS macro)
- Added Fredrik's S-o-B
- Fixed spelling error
- Added the static identifier to init_hexbuf_array_key() in hex.c
- Provided an example in the commit message where oid_to_hex() was being
  used unsafelly.

Matheus Tavares (2):
  compat/win32/pthread: add pthread_once()
  hex: make hash_to_hex_algop() and friends thread-safe

 compat/win32/pthread.c | 18 +++++++++++++++++
 compat/win32/pthread.h |  5 +++++
 hex.c                  | 46 ++++++++++++++++++++++++++++++++++++++----
 thread-utils.c         | 11 ++++++++++
 thread-utils.h         |  7 +++++++
 5 files changed, 83 insertions(+), 4 deletions(-)

Range-diff against v1:
1:  e5a10d3f21 ! 1:  783fcddf8d compat/win32/pthread: add pthread_once()
    @@ Metadata
      ## Commit message ##
         compat/win32/pthread: add pthread_once()
     
    +    Add pthread_once() emulation for Windows. This function provides an easy
    +    way to do thread-safe one-time initializations in multithreaded code. It
    +    will be used in the next commit to make hash_to_hex_algop()
    +    thread-safe.
    +
    +    The pthread_once() implementation added comes from libav
    +    (https://git.libav.org/?p=libav.git), commit b22693b ("w32pthreads: Add
    +    pthread_once emulation", 2015-10-07). The code is licensed under
    +    LGPLv2.1 which is compatible with GPLv2. Only the section for support on
    +    Windows Vista+ has been ported, as that's the minimum version required
    +    to build Git for Windows.  Also, the code was modified to (1) check and
    +    return error codes; and (2) do not call InitOnceComplete() again after a
    +    successful initialization, as that results in an error.
    +
         Signed-off-by: Matheus Tavares <[e-mail]>
     
      ## compat/win32/pthread.c ##
    @@ compat/win32/pthread.c: pthread_t pthread_self(void)
     +	BOOL pending = FALSE;
     +	int ret = 0;
     +
    -+	if(!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {
    ++	if (!InitOnceBeginInitialize(once_control, 0, &pending, NULL)) {
     +		ret = err_win_to_posix(GetLastError());
    -+		goto out;
    -+	}
    -+
    -+	if (pending)
    ++	} else if (pending) {
     +		init_routine();
    ++		if (!InitOnceComplete(once_control, 0, NULL))
    ++			ret = err_win_to_posix(GetLastError());
    ++	}
     +
    -+	if(!InitOnceComplete(once_control, 0, NULL))
    -+		ret = err_win_to_posix(GetLastError());
    -+
    -+out:
     +	/* POSIX doesn't allow pthread_once() to return EINTR */
     +	return ret == EINTR ? EIO : ret;
     +}
    @@ thread-utils.h: int dummy_pthread_join(pthread_t pthread, void **retval);
      int dummy_pthread_init(void *);
      
     +#define PTHREAD_ONCE_INIT 0
    ++#define pthread_once(once, routine) nothreads_pthread_once(once, routine)
    ++
     +int nothreads_pthread_once(pthread_once_t *once_control,
     +			   void (*init_routine)(void));
    -+#define pthread_once(once, routine) nothreads_pthread_once(once, routine)
     +
      #endif
      
2:  0104cd9c76 ! 2:  b47445fa1c hex: make hash_to_hex_algop() and friends thread-safe
    @@ Commit message
         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():
    +    grep.c:grep_source_load_oid(), for example, uses the thread-unsafe
    +    oid_to_hex() (on errors) despite being called in threaded code. And
    +    oid_to_hex() -- which calls hash_to_hex_algop() -- is used in many other
    +    places, as well:
     
         $ 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.
    +    Although hash_to_hex_algop() and its wrappers 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 by replicating the static buffer in each thread's
    +    local storage.
     
         Original-patch-by: Fredrik Kuivinen <[e-mail]>
    +    Signed-off-by: Fredrik Kuivinen <[e-mail]>
         Signed-off-by: Matheus Tavares <[e-mail]>
     
      ## hex.c ##
    +@@
    + #include "cache.h"
    ++#include "thread-utils.h"
    + 
    + const signed char hexval_table[256] = {
    + 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 00-07 */
     @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)
      	return hash_to_hex_algop_r(buffer, oid->hash, the_hash_algo);
      }
    @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)
     +static pthread_key_t hexbuf_array_key;
     +static pthread_once_t hexbuf_array_once = PTHREAD_ONCE_INIT;
     +
    -+void init_hexbuf_array_key(void)
    ++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"));
    @@ hex.c: char *oid_to_hex_r(char *buffer, const struct object_id *oid)
     +	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);
    ++	value = pthread_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))
    ++		if (pthread_setspecific(hexbuf_array_key, (void *)ha))
     +			die(_("failed to set thread buffer for hash to hex conversion"));
     +	}
     +#else