On Mon, Jan 28, 2019 at 03:13:46PM +0000, Alberto Bertogli wrote:
I'll dig in a bit more and see if I can fix libeatmydata, but it might
end up being too invasive and time consuming. I'll send another
message once I know more.
As far as I can see, this is caused by a bug/race in libeatmydata's
initialization, as described before.
It is not a problem in libfiu, or its tests.
The attached patch for libeatmydata fixes the issue, but it's mostly for
illustration since I don't know how the maintainers would prefer to fix
this, and I have not tested it thoroughly (for example there's a chance
of infinite recursion in some very odd conditions, but it might be
better to leave it on purpose to ease debuggability).
https://blitiri.com.ar/tmp/eatmydata-safe-init.patch
Thanks!
Alberto
>From 72a074fde1b722a3e784a3648a34cd58c3196598 Mon Sep 17 00:00:00 2001
From: Alberto Bertogli <albert...@blitiri.com.ar>
Date: Mon, 28 Jan 2019 15:39:31 +0000
Subject: [PATCH] Make initialization thread-safe, and support early callers
The current initialization code is not thread-safe, and assumes that the
only possible caller of open() before initialization completes is
dlsym(), which is not the case if there are other preloaded libraries.
This patch makes the initialization thread-safe by using thread-local
variables, and adjusts the recursive checking logic to support early
callers.
See Debian bug #918520 for more background:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=918520
---
configure.ac | 2 +
libeatmydata/libeatmydata.c | 39 ++++++++++---------
m4/ax_tls.m4 | 74 +++++++++++++++++++++++++++++++++++++
3 files changed, 98 insertions(+), 17 deletions(-)
create mode 100644 m4/ax_tls.m4
diff --git a/configure.ac b/configure.ac
index 7343e0a..140387a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -51,6 +51,8 @@ AC_CHECK_DECLS(sync_file_range)
AC_CHECK_FUNCS(sync_file_range)
AC_CHECK_FUNCS(open64)
+AX_TLS(:,:)
+
AC_CONFIG_FILES(Makefile libeatmydata.spec)
AC_OUTPUT
diff --git a/libeatmydata/libeatmydata.c b/libeatmydata/libeatmydata.c
index 991146b..b54a98b 100644
--- a/libeatmydata/libeatmydata.c
+++ b/libeatmydata/libeatmydata.c
@@ -50,19 +50,23 @@ typedef int (*libc_sync_file_range_t)(int, off64_t, off64_t, unsigned int);
typedef int (*libc_fcntl_t)(int, int, ...);
#endif
-static libc_open_t libc_open= NULL;
+/* All the following are thread-local, to avoid initialization races between
+ * threads. */
+static TLS int init_running = 0;
+static TLS int init_complete = 0;
+static TLS libc_open_t libc_open= NULL;
#ifdef HAVE_OPEN64
-static libc_open64_t libc_open64= NULL;
+static TLS libc_open64_t libc_open64= NULL;
#endif
-static libc_fsync_t libc_fsync= NULL;
-static libc_sync_t libc_sync= NULL;
-static libc_fdatasync_t libc_fdatasync= NULL;
-static libc_msync_t libc_msync= NULL;
+static TLS libc_fsync_t libc_fsync= NULL;
+static TLS libc_sync_t libc_sync= NULL;
+static TLS libc_fdatasync_t libc_fdatasync= NULL;
+static TLS libc_msync_t libc_msync= NULL;
#ifdef HAVE_SYNC_FILE_RANGE
-static libc_sync_file_range_t libc_sync_file_range= NULL;
+static TLS libc_sync_file_range_t libc_sync_file_range= NULL;
#endif
#if defined(F_FULLFSYNC) && defined(__APPLE__)
-static libc_fcntl_t libc_fcntl= NULL;
+static TLS libc_fcntl_t libc_fcntl= NULL;
#endif
#define ASSIGN_DLSYM_OR_DIE(name) \
@@ -87,7 +91,7 @@ void __attribute__ ((constructor)) eatmydata_init(void);
void __attribute__ ((constructor)) eatmydata_init(void)
{
- initing = 1;
+ init_running++;
ASSIGN_DLSYM_OR_DIE(open);
#ifdef HAVE_OPEN64
ASSIGN_DLSYM_OR_DIE(open64);
@@ -102,13 +106,14 @@ void __attribute__ ((constructor)) eatmydata_init(void)
#if defined(F_FULLFSYNC) && defined(__APPLE__)
ASSIGN_DLSYM_OR_DIE(fcntl);
#endif
- initing = 0;
+ init_running--;
+ init_complete++;
}
static int eatmydata_is_hungry(void)
{
/* Init here, as it is called before any libc functions */
- if(!libc_open)
+ if(!init_complete)
eatmydata_init();
#ifdef CHECK_FILE
@@ -164,9 +169,9 @@ int LIBEATMYDATA_API open(const char* pathname, int flags, ...)
#endif
va_end(ap);
- /* In pthread environments the dlsym() may call our open(). */
- /* We simply ignore it because libc is already loaded */
- if (initing) {
+ /* If we get called recursively during initialization (which should
+ * be rare but might happen), just fail. */
+ if (init_running > 0) {
errno = EFAULT;
return -1;
}
@@ -191,9 +196,9 @@ int LIBEATMYDATA_API open64(const char* pathname, int flags, ...)
#endif
va_end(ap);
- /* In pthread environments the dlsym() may call our open(). */
- /* We simply ignore it because libc is already loaded */
- if (initing) {
+ /* If we get called recursively during initialization (which should
+ * be rare but might happen), just fail. */
+ if (init_running > 0) {
errno = EFAULT;
return -1;
}
diff --git a/m4/ax_tls.m4 b/m4/ax_tls.m4
new file mode 100644
index 0000000..51edee8
--- /dev/null
+++ b/m4/ax_tls.m4
@@ -0,0 +1,74 @@
+# ===========================================================================
+# https://www.gnu.org/software/autoconf-archive/ax_tls.html
+# ===========================================================================
+#
+# SYNOPSIS
+#
+# AX_TLS([action-if-found], [action-if-not-found])
+#
+# DESCRIPTION
+#
+# Provides a test for the compiler support of thread local storage (TLS)
+# extensions. Defines TLS if it is found. Currently knows about C++11,
+# GCC/ICC, and MSVC. I think SunPro uses the same as GCC, and Borland
+# apparently supports either.
+#
+# LICENSE
+#
+# Copyright (c) 2008 Alan Woodland <aj...@aber.ac.uk>
+# Copyright (c) 2010 Diego Elio Petteno` <flamee...@gmail.com>
+#
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU General Public License as published by the
+# Free Software Foundation, either version 3 of the License, or (at your
+# option) any later version.
+#
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
+# Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program. If not, see <https://www.gnu.org/licenses/>.
+#
+# As a special exception, the respective Autoconf Macro's copyright owner
+# gives unlimited permission to copy, distribute and modify the configure
+# scripts that are the output of Autoconf when processing the Macro. You
+# need not follow the terms of the GNU General Public License when using
+# or distributing such scripts, even though portions of the text of the
+# Macro appear in them. The GNU General Public License (GPL) does govern
+# all other use of the material that constitutes the Autoconf Macro.
+#
+# This special exception to the GPL applies to versions of the Autoconf
+# Macro released by the Autoconf Archive. When you make and distribute a
+# modified version of the Autoconf Macro, you may extend this special
+# exception to the GPL to apply to your modified version as well.
+
+#serial 14
+
+AC_DEFUN([AX_TLS], [
+ AC_MSG_CHECKING([for thread local storage (TLS) class])
+ AC_CACHE_VAL([ac_cv_tls],
+ [for ax_tls_keyword in thread_local _Thread_local __thread '__declspec(thread)' none; do
+ AS_CASE([$ax_tls_keyword],
+ [none], [ac_cv_tls=none ; break],
+ [AC_TRY_COMPILE(
+ [#include <stdlib.h>
+ static void
+ foo(void) {
+ static ] $ax_tls_keyword [ int bar;
+ exit(1);
+ }],
+ [],
+ [ac_cv_tls=$ax_tls_keyword ; break],
+ ac_cv_tls=none
+ )])
+ done
+ ])
+ AC_MSG_RESULT([$ac_cv_tls])
+
+ AS_IF([test "$ac_cv_tls" != "none"],
+ [AC_DEFINE_UNQUOTED([TLS],[$ac_cv_tls],[If the compiler supports a TLS storage class define it to that here])
+ m4_ifnblank([$1],[$1])],
+ [m4_ifnblank([$2],[$2])])
+])
--
2.20.1
