valgrind drd tool shows conflicting stores happening at lxc_global_co…

…nfig_value@src/lxc/utils.c (v2)

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>

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)
 # -------------------------

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) {

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)

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";
 

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;

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