[lxc-devel] [PATCH] valgrind drd tool shows conflicting stores happening at lxc_global_config_value at src/lxc/utils.c (v2)
Serge Hallyn
serge.hallyn at ubuntu.com
Fri Nov 1 21:37:51 UTC 2013
Quoting S.Çağlar Onur (caglar at 10ur.org):
> 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 at 10ur.org>
Thanks.
Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
(Just a note - you appear to have expand-tab set in your editor,
as you're replacing some tabs with spaces in this patch.)
> ---
> configure.ac | 9 ++++++
> src/lxc/cgroup.c | 2 +-
> src/lxc/lxclock.c | 17 +++++++++--
> src/lxc/start.c | 2 +-
> src/lxc/utils.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> src/lxc/utils.h | 5 +++-
> 6 files changed, 115 insertions(+), 10 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 9fedf55..6004b35 100644
> --- 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 01ed040..1e1e72a 100644
> --- 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 d403bcc..3857ff0 100644
> --- 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 1cadc09..58e1194 100644
> --- 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 9e2e326..590482e 100644
> --- 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 fc46760..9c47560 100644
> --- 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
> --
> 1.8.3.2
>
>
> ------------------------------------------------------------------------------
> Android is increasing in popularity, but the open development platform that
> developers love is also attractive to malware creators. Download this white
> paper to learn more about secure code signing practices that can help keep
> Android apps secure.
> http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
> _______________________________________________
> Lxc-devel mailing list
> Lxc-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel
More information about the lxc-devel
mailing list