valgrind drd tool shows conflicting stores happening at lxc_global_config_value@src...

Conflict occurs between following lines

[...]
269         if (values[i])
270                 return values[i];
[...]

and

[...]
309         /* could not find value, use default */
310         values[i] = (*ptr)[1];
[...]

fix it using a specific lock dedicated to that problem as Serge suggested.

Also introduce a new autoconf parameter (--enable-mutex-debugging) to convert mutexes to error reporting type and to provide a stacktrace when locking fails.

Signed-off-by: S.Çağlar Onur <caglar@10ur.org>
Signed-off-by: Serge Hallyn <serge.hallyn@ubuntu.com>

diff --git a/configure.ac b/configure.ac
index 9fedf55a1bdd8c64223880e6b01ed8b7b3a414df..6004b356b7cb96a1b52c23456bb848ce67899cb1 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -178,6 +178,15 @@ AM_COND_IF([ENABLE_PYTHON],
        PKG_CHECK_MODULES([PYTHONDEV], [python3 >= 3.2],[],[AC_MSG_ERROR([You must install python3-dev])])
        AC_DEFINE_UNQUOTED([ENABLE_PYTHON], 1, [Python3 is available])])
 
+# Enable dumping stack traces
+AC_ARG_ENABLE([mutex-debugging],
+       [AC_HELP_STRING([--enable-mutex-debugging], [Makes mutexes to report error and provide stack trace])],
+       [enable_mutex_debugging=yes], [enable_mutex_debugging=no])
+AM_CONDITIONAL([MUTEX_DEBUGGING], [test "x$enable_mutex_debugging" = "xyes"])
+
+AM_COND_IF([MUTEX_DEBUGGING],
+       AC_DEFINE_UNQUOTED([MUTEX_DEBUGGING], 1, [Enabling mutex debugging]))
+
 # Not in older autoconf versions
 # AS_VAR_COPY(DEST, SOURCE)
 # -------------------------

diff --git a/src/lxc/cgroup.c b/src/lxc/cgroup.c
index 01ed04092ff0309a199a9a2156717214afe6cf0c..8be0ebf42dbf41b73739047dcc5fb10b3229003a 100644 (file)
--- a/src/lxc/cgroup.c
+++ b/src/lxc/cgroup.c
@@ -91,7 +91,7 @@ struct cgroup_meta_data *lxc_cgroup_load_meta()
        int saved_errno;
 
        errno = 0;
-       cgroup_use = lxc_global_config_value("cgroup.use");
+       cgroup_use = default_cgroup_use();
        if (!cgroup_use && errno != 0)
                return NULL;
        if (cgroup_use) {

diff --git a/src/lxc/lxclock.c b/src/lxc/lxclock.c
index d403bcc1647f5d27e6dff7e53f1a353819fa471d..3857ff06cbec1f894de41cb9c62afa3e773b7a24 100644 (file)
--- a/src/lxc/lxclock.c
+++ b/src/lxc/lxclock.c
@@ -18,15 +18,15 @@
  *  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
-#include <pthread.h>
+#define _GNU_SOURCE
 #include "lxclock.h"
 #include <malloc.h>
 #include <stdio.h>
 #include <errno.h>
 #include <unistd.h>
 #include <fcntl.h>
-#define _GNU_SOURCE
 #include <stdlib.h>
+#include <pthread.h>
 #include <lxc/utils.h>
 #include <lxc/log.h>
 #include <lxc/lxccontainer.h>
@@ -38,7 +38,11 @@
 
 lxc_log_define(lxc_lock, lxc);
 
+#ifdef MUTEX_DEBUGGING
+pthread_mutex_t thread_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+#else
 pthread_mutex_t thread_mutex = PTHREAD_MUTEX_INITIALIZER;
+#endif
 
 static char *lxclock_name(const char *p, const char *n)
 {
@@ -267,13 +271,20 @@ void process_lock(void)
 
        if ((ret = pthread_mutex_lock(&thread_mutex)) != 0) {
                ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+               dump_stacktrace();
                exit(1);
        }
 }
 
 void process_unlock(void)
 {
-       pthread_mutex_unlock(&thread_mutex);
+       int ret;
+
+       if ((ret = pthread_mutex_unlock(&thread_mutex)) != 0) {
+               ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
+               dump_stacktrace();
+               exit(1);
+       }
 }
 
 int container_mem_lock(struct lxc_container *c)

diff --git a/src/lxc/start.c b/src/lxc/start.c
index 1cadc0963f772aa7451b50a5479fe0997caff284..3b2ba8fbd06f0cb9b0bbf7da578be0bd54401d0c 100644 (file)
--- a/src/lxc/start.c
+++ b/src/lxc/start.c
@@ -695,7 +695,7 @@ int lxc_spawn(struct lxc_handler *handler)
         * default value is available
         */
        if (getuid() == 0)
-               cgroup_pattern = lxc_global_config_value("cgroup.pattern");
+               cgroup_pattern = default_cgroup_pattern();
        if (!cgroup_pattern)
                cgroup_pattern = "%n";
 

diff --git a/src/lxc/utils.c b/src/lxc/utils.c
index 9e2e326146835c4c734eb5737e4cb7750a0bbd5e..590482e0b310aa49818955404fecf13b613c3511 100644 (file)
--- a/src/lxc/utils.c
+++ b/src/lxc/utils.c
@@ -21,7 +21,8 @@
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#define _GNU_SOURCE
+#include "config.h"
+
 #include <errno.h>
 #include <unistd.h>
 #include <stdlib.h>
@@ -38,6 +39,8 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <assert.h>
+#include <pthread.h>
+#include <execinfo.h>
 
 #ifndef HAVE_GETLINE
 #ifdef HAVE_FGETLN
@@ -49,8 +52,61 @@
 #include "log.h"
 #include "lxclock.h"
 
+#define MAX_STACKDEPTH 25
+
 lxc_log_define(lxc_utils, lxc);
 
+
+#ifdef MUTEX_DEBUGGING
+static pthread_mutex_t static_mutex = PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP;
+
+inline void dump_stacktrace(void)
+{
+       void *array[MAX_STACKDEPTH];
+       size_t size;
+       char **strings;
+       size_t i;
+
+       size = backtrace(array, MAX_STACKDEPTH);
+       strings = backtrace_symbols(array, size);
+
+       // Using fprintf here as our logging module is not thread safe
+       fprintf(stderr, "\tObtained %zd stack frames.\n", size);
+
+       for (i = 0; i < size; i++)
+               fprintf(stderr, "\t\t%s\n", strings[i]);
+
+       free (strings);
+}
+#else
+static pthread_mutex_t static_mutex = PTHREAD_MUTEX_INITIALIZER;
+
+inline void dump_stacktrace(void) {;}
+#endif
+
+/* Protects static const values inside the lxc_global_config_value funtion */
+static void static_lock(void)
+{
+       int ret;
+
+       if ((ret = pthread_mutex_lock(&static_mutex)) != 0) {
+               ERROR("pthread_mutex_lock returned:%d %s", ret, strerror(ret));
+               dump_stacktrace();
+               exit(1);
+       }
+}
+
+static void static_unlock(void)
+{
+       int ret;
+
+       if ((ret = pthread_mutex_unlock(&static_mutex)) != 0) {
+               ERROR("pthread_mutex_unlock returned:%d %s", ret, strerror(ret));
+               dump_stacktrace();
+               exit(1);
+       }
+}
+
 static int _recursive_rmdir_onedev(char *dirname, dev_t pdev)
 {
        struct dirent dirent, *direntp;
@@ -252,8 +308,10 @@ const char *lxc_global_config_value(const char *option_name)
                { "cgroup.use",      NULL            },
                { NULL, NULL },
        };
+       /* Protected by a mutex to eliminate conflicting load and store operations */ 
        static const char *values[sizeof(options) / sizeof(options[0])] = { 0 };
        const char *(*ptr)[2];
+       const char *value;
        size_t i;
        char buf[1024], *p, *p2;
        FILE *fin = NULL;
@@ -266,8 +324,14 @@ const char *lxc_global_config_value(const char *option_name)
                errno = EINVAL;
                return NULL;
        }
-       if (values[i])
-               return values[i];
+
+       static_lock();
+       if (values[i]) {
+               value = values[i];
+               static_unlock();
+               return value;
+       }
+       static_unlock();
 
        process_lock();
        fin = fopen_cloexec(LXC_GLOBAL_CONF, "r");
@@ -304,24 +368,31 @@ const char *lxc_global_config_value(const char *option_name)
                        while (*p && (*p == ' ' || *p == '\t')) p++;
                        if (!*p)
                                continue;
+                       static_lock();
                        values[i] = copy_global_config_value(p);
+                       static_unlock();
                        goto out;
                }
        }
        /* could not find value, use default */
+       static_lock();
        values[i] = (*ptr)[1];
        /* special case: if default value is NULL,
         * and there is no config, don't view that
         * as an error... */
        if (!values[i])
                errno = 0;
+       static_unlock();
 
 out:
        process_lock();
        if (fin)
                fclose(fin);
        process_unlock();
-       return values[i];
+       static_lock();
+       value = values[i];
+       static_unlock();
+       return value;
 }
 
 const char *default_lvm_vg(void)
@@ -338,11 +409,22 @@ const char *default_zfs_root(void)
 {
        return lxc_global_config_value("zfsroot");
 }
+
 const char *default_lxc_path(void)
 {
        return lxc_global_config_value("lxcpath");
 }
 
+const char *default_cgroup_use(void)
+{
+       return lxc_global_config_value("cgroup.use");
+}
+
+const char *default_cgroup_pattern(void)
+{
+       return lxc_global_config_value("cgroup.pattern");
+}
+
 const char *get_rundir()
 {
        const char *rundir;

diff --git a/src/lxc/utils.h b/src/lxc/utils.h
index fc4676096d6572608fff0d8b9ec7899df24d65a6..9c47560cf20f0891cd65a77158ef25777562bf54 100644 (file)
--- a/src/lxc/utils.h
+++ b/src/lxc/utils.h
@@ -49,7 +49,8 @@ extern const char *default_lxc_path(void);
 extern const char *default_zfs_root(void);
 extern const char *default_lvm_vg(void);
 extern const char *default_lvm_thin_pool(void);
-
+extern const char *default_cgroup_use(void);
+extern const char *default_cgroup_pattern(void);
 /* Define getline() if missing from the C library */
 #ifndef HAVE_GETLINE
 #ifdef HAVE_FGETLN
@@ -239,4 +240,6 @@ extern size_t lxc_array_len(void **array);
 extern void **lxc_dup_array(void **array, lxc_dup_fn element_dup_fn, lxc_free_fn element_free_fn);
 
 extern void **lxc_append_null_to_array(void **array, size_t count);
+
+extern void dump_stacktrace(void);
 #endif