git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Matheus Tavares <matheus.bernardino@usp.br>
Date
Jun 26, 2020, 21:54 UTC
Message-ID
<cover.1593208411.git.matheus.bernardino@usp.br>
In-Reply-To
<cover.1593115455.git.matheus.bernardino@usp.br>

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 <matheus.bernardino@usp.br>
     
      ## 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 <frekui@gmail.com>
    +    Signed-off-by: Fredrik Kuivinen <frekui@gmail.com>
         Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
     
      ## 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
-- 
2.26.2
Previous: Matheus Tavares BernardinoNext: Matheus Tavares
Message 15 of 25 in “[RFC] Thread safety in some low-level functions”
  1. Matheus Tavares BernardinoJun 24, 2020
  2. Matheus Tavares BernardinoJun 24, 2020
  3. brian m. carlsonJun 25, 2020
  4. 0/2 Make oid_to_hex() thread-safeMatheus Tavares, Jun 25, 2020
  5. 1/2 compat/win32/pthread: add pthread_once()Matheus Tavares, Jun 25, 2020
  6. Christian CouderJun 26, 2020
  7. Matheus Tavares BernardinoJun 26, 2020
  8. 2/2 hex: make hash_to_hex_algop() and friends thread-safeMatheus Tavares, Jun 25, 2020
  9. brian m. carlsonJun 25, 2020
  10. Matheus Tavares BernardinoJun 25, 2020
  11. Matheus Tavares BernardinoJun 26, 2020
  12. Christian CouderJun 26, 2020
  13. Christian CouderJun 26, 2020
  14. Matheus Tavares BernardinoJun 26, 2020
  15. 0/2 Make oid_to_hex() thread-safeMatheus Tavares, Jun 26, 2020
  16. 1/2 compat/win32/pthread: add pthread_once()Matheus Tavares, Jun 26, 2020
  17. 2/2 hex: make hash_to_hex_algop() and friends thread-safeMatheus Tavares, Jun 26, 2020
  18. Johannes SchindelinJun 29, 2020
  19. Matheus Tavares BernardinoJun 30, 2020
  20. Johannes SchindelinJul 1, 2020
  21. Johannes SchindelinJul 16, 2020
  22. Matheus Tavares BernardinoJul 18, 2020
  23. Johannes SchindelinAug 10, 2020
  24. Matheus TavaresJul 18, 2020
  25. René ScharfeJul 26, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.