git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: git <git@vger.kernel.org>
Cc: Johannes Sixt <j6t@kdbg.org>, Jeff King <peff@peff.net>,
	Jonathan Tan <jonathantanmy@google.com>
Subject: [RFC] Thread safety in some low-level functions
Date: Wed, 24 Jun 2020 19:29:32 -0300	[thread overview]
Message-ID: <CAHd-oW5zh=BG29O0Z-M7R26Lgd=RHECMV2+qByF+vU6PmrEn_Q@mail.gmail.com> (raw)

Hi,

I'm working with threads in unpack-trees and noticed that
[warning|error]_errno() uses strerror(), which is not thread-safe. We
could try to avoid calling these functions in threaded code, but they
are sometimes too deep in the call stack to be noticed... (or even
avoided). The same happens with oid_to_hex(), which writes to a static
buffer.

I don't think I've ever seen a bug report involving these functions
being called racily, but this possibility is not currently excluded in
our codebase. For example, see grep_source_load_file(), which is
called by multiple threads concurrently and might call the
thread-unsafe error_errno(). (Although, I admit, the chance of a race
here must be very low...)

I still haven't been able to come up with a simple / easy change that
could make these functions thread safe, but here are my thoughts so
far:

- For strerror(), there is a thread-safe variant: strerror_r().
However IIUC, this variant is not present on Windows (although there
is strerror_s() which *seems* to be somewhat similar). Also, there are
two versions of strerror_r() on Linux: one is XSI-compliant and the
other is GNU-specific. I don't know what the situation is in other
OSes...

- Regarding, oid_to_hex(), a patch from 2010 [1] proposed a solution
using thread-local storage and pthread_once(). But as Hannes pointed
out in this other thread [2] , implementing a Windows equivalence for
pthread_once() could be tricky and voluminous. Since this thread dates
from 7 years ago, I was wondering if we would be able to implement it
nowadays with InitOnceExecuteOnce() [3].

Finally, leaving these functions thread-unsafe is also a
possibility... As I mentioned earlier, they don't seem to be causing
problems out there for now (at least not reported). But if we can find
a feasible solution to introduce thread-safety, I think it would be
great. The codebase would be more robust and we would be able to work
with threads with much more confidence.

Any thoughts?

Thanks,
Matheus

[1] https://lore.kernel.org/git/20100323173130.GC4218@fredrik-laptop/
[2] https://lore.kernel.org/git/516D5CA4.7000500@viscovery.net/
[3]: https://docs.microsoft.com/en-us/windows/win32/sync/using-one-time-initialization

             reply	other threads:[~2020-06-24 22:29 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-24 22:29 Matheus Tavares Bernardino [this message]
2020-06-24 22:52 ` Matheus Tavares Bernardino
2020-06-25  1:38   ` brian m. carlson
2020-06-25 20:32     ` [PATCH 0/2] Make oid_to_hex() thread-safe Matheus Tavares
2020-06-25 20:32       ` [PATCH 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26  5:45         ` Christian Couder
2020-06-26 14:13           ` Matheus Tavares Bernardino
2020-06-25 20:32       ` [PATCH 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-25 23:07         ` brian m. carlson
2020-06-25 23:54           ` Matheus Tavares Bernardino
2020-06-26  0:00             ` Matheus Tavares Bernardino
2020-06-26  6:02         ` Christian Couder
2020-06-26  8:22       ` [PATCH 0/2] Make oid_to_hex() thread-safe Christian Couder
2020-06-26 16:22         ` Matheus Tavares Bernardino
2020-06-26 21:54       ` [PATCH v2 " Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 1/2] compat/win32/pthread: add pthread_once() Matheus Tavares
2020-06-26 21:54         ` [PATCH v2 2/2] hex: make hash_to_hex_algop() and friends thread-safe Matheus Tavares
2020-06-29 15:11           ` Johannes Schindelin
2020-06-30 20:37             ` Matheus Tavares Bernardino
2020-07-01 11:32               ` Johannes Schindelin
2020-07-16 11:29           ` Johannes Schindelin
2020-07-18  3:09             ` Matheus Tavares Bernardino
2020-08-10 14:15               ` Johannes Schindelin
2020-07-18  3:52             ` Matheus Tavares
2020-07-26 17:41               ` René Scharfe

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHd-oW5zh=BG29O0Z-M7R26Lgd=RHECMV2+qByF+vU6PmrEn_Q@mail.gmail.com' \
    --to=matheus.bernardino@usp.br \
    --cc=git@vger.kernel.org \
    --cc=j6t@kdbg.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --subject='Re: [RFC] Thread safety in some low-level functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).