Skip to content

openssl: fix thread-safety on non-glibc POSIX systems#4427

Merged
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/openssl-threadid
Dec 1, 2017
Merged

openssl: fix thread-safety on non-glibc POSIX systems#4427
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/openssl-threadid

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Nov 30, 2017

While the OpenSSL library provides all means to work safely in a
multi-threaded application, we fail to do so correctly. Quoting from
crypto_lock(3):

OpenSSL can safely be used in multi-threaded applications provided
that at least two callback functions are set, locking_function and
threadid_func.

We do in fact provide the means to set up the locking function via
git_openssl_set_locking(), where we initialize a set of locks by using
the POSIX threads API and set the correct callback function to lock and
unlock them.

But what we do not do is setting the threadid_func callback. This
function is being used to correctly locate thread-local data of the
OpenSSL library and should thus return per-thread identifiers. Digging
deeper into OpenSSL's documentation, the library does provide a fallback
in case that locking function is not provided by the user. On Windows
and BeOS we should be safe, as it simply "uses the system's default
thread identifying API". On other platforms though OpenSSL will fall
back to using the address of errno, assuming it is thread-local.

While this assumption holds true for glibc-based systems, POSIX in fact
does not specify whether it is thread-local or not. Quoting from
errno(3p):

It is unspecified whether errno is a macro or an identifier declared
with external linkage.

And in fact, with musl there is at least one libc implementation which
simply declares errno as a simple int without being thread-local. On
those systems, the fallback threadid function of OpenSSL will not be
thread-safe.

Fix this by setting up our own callback for this setting. As users of
libgit2 may want to set it themselves, we obviously cannot always set
that function on initialization. But as we already set up primitives for
threading in git_openssl_set_locking(), this function becomes the
obvious choice where to implement the additional setup.

While the OpenSSL library provides all means to work safely in a
multi-threaded application, we fail to do so correctly. Quoting from
crypto_lock(3):

    OpenSSL can safely be used in multi-threaded applications provided
    that at least two callback functions are set, locking_function and
    threadid_func.

We do in fact provide the means to set up the locking function via
`git_openssl_set_locking()`, where we initialize a set of locks by using
the POSIX threads API and set the correct callback function to lock and
unlock them.

But what we do not do is setting the `threadid_func` callback. This
function is being used to correctly locate thread-local data of the
OpenSSL library and should thus return per-thread identifiers. Digging
deeper into OpenSSL's documentation, the library does provide a fallback
in case that locking function is not provided by the user. On Windows
and BeOS we should be safe, as it simply "uses the system's default
thread identifying API". On other platforms though OpenSSL will fall
back to using the address of `errno`, assuming it is thread-local.

While this assumption holds true for glibc-based systems, POSIX in fact
does not specify whether it is thread-local or not. Quoting from
errno(3p):

    It is unspecified whether errno is a macro or an identifier declared
    with external linkage.

And in fact, with musl there is at least one libc implementation which
simply declares `errno` as a simple `int` without being thread-local. On
those systems, the fallback threadid function of OpenSSL will not be
thread-safe.

Fix this by setting up our own callback for this setting. As users of
libgit2 may want to set it themselves, we obviously cannot always set
that function on initialization. But as we already set up primitives for
threading in `git_openssl_set_locking()`, this function becomes the
obvious choice where to implement the additional setup.
@ethomson
Copy link
Copy Markdown
Member

ethomson commented Dec 1, 2017

Thanks for fixing this and for the excellent explanation of why this is necessary. 👍

@ethomson ethomson merged commit 344b4ea into libgit2:master Dec 1, 2017
@pks-t pks-t deleted the pks/openssl-threadid branch December 15, 2017 13:30
@pks-t pks-t added the backport label Jan 11, 2018
@pks-t pks-t mentioned this pull request Jan 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants