Quoting S.Çağlar Onur (cag...@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 <cag...@10ur.org>
Thanks.
Acked-by: Serge E. Hallyn <serge.hal...@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@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lxc-devel
------------------------------------------------------------------------------
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@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/lxc-devel
