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

Message

Matheus Tavares June 25, 2020, 8:32 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.

As a next step, I would love to make [warning|error|die]_errno()
thread-safe as well. strerror() is not safe on *nix, and there are some
thread functions today that call these (although the actual risk of a
race condition must be very small...)

My idea was to implement a xstrerror() wrapper which calls the
appropriate thread-safe function (dependant on the OS), or even call
strerror() itself but holding a lock to copy the result for a local
buffer (which should be OK as we don't expect contention in strerror).
We could also set a thread local buffer array, as in the second patch of
this series, to excuse callers from allocating/freeing memory.

One concern with this idea is the risk of an infinite recursion if
xstrerror() or any of its childs call [warning|error|die]_errno().
However, if we are only using strerror() and pthread_*() within the
wrapper, there should be no problem, right? Has anyone thought of
other problems with this approach?

Finally, should such change also come with a coccinelle patch to replace
usages of strerror() with xstrerror()? Or better not, as the change
would be too big?

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

 compat/win32/pthread.c | 22 +++++++++++++++++++++
 compat/win32/pthread.h |  5 +++++
 hex.c                  | 45 ++++++++++++++++++++++++++++++++++++++----
 thread-utils.c         | 11 +++++++++++
 thread-utils.h         |  6 ++++++
 5 files changed, 85 insertions(+), 4 deletions(-)

Comments

Christian Couder June 26, 2020, 8:22 a.m. UTC | #1

On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
<[e-mail]> wrote:
>
> 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.

Great!

> As a next step, I would love to make [warning|error|die]_errno()
> thread-safe as well. strerror() is not safe on *nix, and there are some
> thread functions today that call these (although the actual risk of a
> race condition must be very small...)
>
> My idea was to implement a xstrerror() wrapper which calls the
> appropriate thread-safe function (dependant on the OS),

Yeah, that works if there are appropriate thread-safe functions for
all the OS we are interested in, or if we can fallback to strerror()
or calling it holding a lock.

> or even call
> strerror() itself but holding a lock to copy the result for a local
> buffer (which should be OK as we don't expect contention in strerror).

I agree that it should be ok.

> We could also set a thread local buffer array, as in the second patch of
> this series, to excuse callers from allocating/freeing memory.

I don't think caller allocating/freeing memory for error strings is a
performance or code simplification issue.

> One concern with this idea is the risk of an infinite recursion if
> xstrerror() or any of its childs call [warning|error|die]_errno().
> However, if we are only using strerror() and pthread_*() within the
> wrapper, there should be no problem, right?

Yeah, I agree.

> Has anyone thought of
> other problems with this approach?
>
> Finally, should such change also come with a coccinelle patch to replace
> usages of strerror() with xstrerror()? Or better not, as the change
> would be too big?

I would agree that it's better to avoid too big changes. I am not sure
how much we want to automate and check that though.

Thanks,
Christian.

Matheus Tavares June 26, 2020, 4:22 p.m. UTC | #2

Hi, Christian

Thanks for the feedback!

On Fri, Jun 26, 2020 at 5:22 AM Christian Couder
<[e-mail]> wrote:
>
> On Fri, Jun 26, 2020 at 1:35 AM Matheus Tavares
> <[e-mail]> wrote:
> >
> > My idea was to implement a xstrerror() wrapper which calls the
> > appropriate thread-safe function (dependant on the OS),
[...]
> > We could also set a thread local buffer array, as in the second patch of
> > this series, to excuse callers from allocating/freeing memory.
>
> I don't think caller allocating/freeing memory for error strings is a
> performance or code simplification issue.

Yeah, I agree that passing a buffer to xstrerror() should be fine. The
problem is that we already have many uses of strerror() (98, according
to git-grep), which expect a static buffer in return. So the change
would be too big :(

> > Finally, should such change also come with a coccinelle patch to replace
> > usages of strerror() with xstrerror()? Or better not, as the change
> > would be too big?
>
> I would agree that it's better to avoid too big changes. I am not sure
> how much we want to automate and check that though.

Another alternative would be to replace the definition of strerror() with a:

#define strerror(errnum) xstrerror(errnum)

This way, all the 98 current strerror() callers would start using the
thread-safe version without the need to change them.... However, I
don't think I've ever seen such a replacement in our codebase before
(besides the banned functions in banned.h). Also, people might expect
strerror() to refer to the system version, and perhaps make wrong
assumptions about it? I'm not sure which is the best alternative, it's
a tricky issue :(