It helps ASAN to detect more leaks on coroutine stacks, as found in
the following patch.

A similar work would need to be done for sigaltstack & windows fibers
to have similar coverage. Since ucontext is preferred, I didn't bother
checking the other coroutine implementations for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/compiler.h   |  4 ++++
 util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5fcc4f7ec7 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,8 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+#ifndef __has_feature
+#define __has_feature(x) 0 /* compatibility with non-clang compilers */
+#endif
+
 #endif /* COMPILER_H */
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 6621f3f692..e78eae8766 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -31,6 +31,11 @@
 #include <valgrind/valgrind.h>
 #endif
 
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#define CONFIG_ASAN 1
+#include <sanitizer/asan_interface.h>
+#endif
+
 typedef struct {
     Coroutine base;
     void *stack;
@@ -59,11 +64,37 @@ union cc_arg {
     int i[2];
 };
 
+static void finish_switch_fiber(void *fake_stack_save)
+{
+#ifdef CONFIG_ASAN
+    const void *bottom_old;
+    size_t size_old;
+
+    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
+
+    if (!leader.stack) {
+        leader.stack = (void *)bottom_old;
+        leader.stack_size = size_old;
+    }
+#endif
+}
+
+static void start_switch_fiber(void **fake_stack_save,
+                               const void *bottom, size_t size)
+{
+#ifdef CONFIG_ASAN
+    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
+#endif
+}
+
 static void coroutine_trampoline(int i0, int i1)
 {
     union cc_arg arg;
     CoroutineUContext *self;
     Coroutine *co;
+    void *fake_stack_save = NULL;
+
+    finish_switch_fiber(NULL);
 
     arg.i[0] = i0;
     arg.i[1] = i1;
@@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
 
     /* Initialize longjmp environment and switch back the caller */
     if (!sigsetjmp(self->env, 0)) {
+        start_switch_fiber(&fake_stack_save,
+                           leader.stack, leader.stack_size);
         siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
     }
 
+    finish_switch_fiber(fake_stack_save);
+
     while (true) {
         co->entry(co->entry_arg);
         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
@@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
     union cc_arg arg = {0};
+    void *fake_stack_save = NULL;
 
     /* The ucontext functions preserve signal masks which incurs a
      * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
@@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
 
     /* swapcontext() in, siglongjmp() back out */
     if (!sigsetjmp(old_env, 0)) {
+        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
         swapcontext(&old_uc, &uc);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return &co->base;
 }
 
@@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
     CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
     int ret;
+    void *fake_stack_save = NULL;
 
     current = to_;
 
     ret = sigsetjmp(from->env, 0);
     if (ret == 0) {
+        start_switch_fiber(action == COROUTINE_TERMINATE ?
+                           NULL : &fake_stack_save, to->stack, to->stack_size);
         siglongjmp(to->env, action);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return ret;
 }
 
-- 
2.15.1.355.g36791d7216

Hi Marc-André,

On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
> It helps ASAN to detect more leaks on coroutine stacks, as found in
> the following patch.
> 
> A similar work would need to be done for sigaltstack & windows fibers
> to have similar coverage. Since ucontext is preferred, I didn't bother
> checking the other coroutine implementations for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/qemu/compiler.h   |  4 ++++
>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 340e5fdc09..5fcc4f7ec7 100644
> --- a/include/qemu/compiler.h
> +++ b/include/qemu/compiler.h
> @@ -111,4 +111,8 @@
>  #define GCC_FMT_ATTR(n, m)
>  #endif
>  
> +#ifndef __has_feature
> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
> +#endif
> +
>  #endif /* COMPILER_H */
> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
> index 6621f3f692..e78eae8766 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -31,6 +31,11 @@
>  #include <valgrind/valgrind.h>
>  #endif
>  
> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> +#define CONFIG_ASAN 1
> +#include <sanitizer/asan_interface.h>
> +#endif

Sadly this fails on Travis:

$ export CONFIG="--enable-debug --enable-debug-tcg
--enable-trace-backends=log"
$ export CC=gcc
$ ./configure ${CONFIG}
C compiler        gcc
Host C compiler   cc
C++ compiler      c++
Objective-C compiler clang
CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
[...]
  CC      util/qemu-coroutine.o
  CC      util/qemu-coroutine-lock.o
  CC      util/qemu-coroutine-io.o
  CC      util/qemu-coroutine-sleep.o
  CC      util/coroutine-ucontext.o
util/coroutine-ucontext.c:36:38: fatal error:
sanitizer/asan_interface.h: No such file or directory
 #include <sanitizer/asan_interface.h>
                                      ^
compilation terminated.
make: *** [util/coroutine-ucontext.o] Error 1

You simply need to update the .travis.yml, but does that mean your
previous patch "Enable ASAN/UBSan by default if the compiler supports
it." isn't correct?

> +
>  typedef struct {
>      Coroutine base;
>      void *stack;
> @@ -59,11 +64,37 @@ union cc_arg {
>      int i[2];
>  };
>  
> +static void finish_switch_fiber(void *fake_stack_save)
> +{
> +#ifdef CONFIG_ASAN
> +    const void *bottom_old;
> +    size_t size_old;
> +
> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
> +
> +    if (!leader.stack) {
> +        leader.stack = (void *)bottom_old;
> +        leader.stack_size = size_old;
> +    }
> +#endif
> +}
> +
> +static void start_switch_fiber(void **fake_stack_save,
> +                               const void *bottom, size_t size)
> +{
> +#ifdef CONFIG_ASAN
> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
> +#endif
> +}
> +
>  static void coroutine_trampoline(int i0, int i1)
>  {
>      union cc_arg arg;
>      CoroutineUContext *self;
>      Coroutine *co;
> +    void *fake_stack_save = NULL;
> +
> +    finish_switch_fiber(NULL);
>  
>      arg.i[0] = i0;
>      arg.i[1] = i1;
> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>  
>      /* Initialize longjmp environment and switch back the caller */
>      if (!sigsetjmp(self->env, 0)) {
> +        start_switch_fiber(&fake_stack_save,
> +                           leader.stack, leader.stack_size);
>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>      }
>  
> +    finish_switch_fiber(fake_stack_save);
> +
>      while (true) {
>          co->entry(co->entry_arg);
>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>      ucontext_t old_uc, uc;
>      sigjmp_buf old_env;
>      union cc_arg arg = {0};
> +    void *fake_stack_save = NULL;
>  
>      /* The ucontext functions preserve signal masks which incurs a
>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>  
>      /* swapcontext() in, siglongjmp() back out */
>      if (!sigsetjmp(old_env, 0)) {
> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>          swapcontext(&old_uc, &uc);
>      }
> +
> +    finish_switch_fiber(fake_stack_save);
> +
>      return &co->base;
>  }
>  
> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>      int ret;
> +    void *fake_stack_save = NULL;
>  
>      current = to_;
>  
>      ret = sigsetjmp(from->env, 0);
>      if (ret == 0) {
> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>          siglongjmp(to->env, action);
>      }
> +
> +    finish_switch_fiber(fake_stack_save);
> +
>      return ret;
>  }
>  
> 

> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>> the following patch.
>>
>> A similar work would need to be done for sigaltstack & windows fibers
>> to have similar coverage. Since ucontext is preferred, I didn't bother
>> checking the other coroutine implementations for now.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/qemu/compiler.h   |  4 ++++
>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 340e5fdc09..5fcc4f7ec7 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -111,4 +111,8 @@
>>  #define GCC_FMT_ATTR(n, m)
>>  #endif
>>  
>> +#ifndef __has_feature
>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>> +#endif
>> +
>>  #endif /* COMPILER_H */
>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>> index 6621f3f692..e78eae8766 100644
>> --- a/util/coroutine-ucontext.c
>> +++ b/util/coroutine-ucontext.c
>> @@ -31,6 +31,11 @@
>>  #include <valgrind/valgrind.h>
>>  #endif
>>  
>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>> +#define CONFIG_ASAN 1
>> +#include <sanitizer/asan_interface.h>
>> +#endif
> 
> Sadly this fails on Travis:
> 
> $ export CONFIG="--enable-debug --enable-debug-tcg
> --enable-trace-backends=log"
> $ export CC=gcc
> $ ./configure ${CONFIG}
> C compiler        gcc
> Host C compiler   cc
> C++ compiler      c++
> Objective-C compiler clang
> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
> [...]
>   CC      util/qemu-coroutine.o
>   CC      util/qemu-coroutine-lock.o
>   CC      util/qemu-coroutine-io.o
>   CC      util/qemu-coroutine-sleep.o
>   CC      util/coroutine-ucontext.o
> util/coroutine-ucontext.c:36:38: fatal error:
> sanitizer/asan_interface.h: No such file or directory
>  #include <sanitizer/asan_interface.h>
>                                       ^
> compilation terminated.
> make: *** [util/coroutine-ucontext.o] Error 1
> 
> You simply need to update the .travis.yml, but does that mean your
> previous patch "Enable ASAN/UBSan by default if the compiler supports
> it." isn't correct?

Err, the previous patch subject is "build-sys: add some sanitizers when
--enable-debug if possible"

>> +
>>  typedef struct {
>>      Coroutine base;
>>      void *stack;
>> @@ -59,11 +64,37 @@ union cc_arg {
>>      int i[2];
>>  };
>>  
>> +static void finish_switch_fiber(void *fake_stack_save)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    const void *bottom_old;
>> +    size_t size_old;
>> +
>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>> +
>> +    if (!leader.stack) {
>> +        leader.stack = (void *)bottom_old;
>> +        leader.stack_size = size_old;
>> +    }
>> +#endif
>> +}
>> +
>> +static void start_switch_fiber(void **fake_stack_save,
>> +                               const void *bottom, size_t size)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>> +#endif
>> +}
>> +
>>  static void coroutine_trampoline(int i0, int i1)
>>  {
>>      union cc_arg arg;
>>      CoroutineUContext *self;
>>      Coroutine *co;
>> +    void *fake_stack_save = NULL;
>> +
>> +    finish_switch_fiber(NULL);
>>  
>>      arg.i[0] = i0;
>>      arg.i[1] = i1;
>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>  
>>      /* Initialize longjmp environment and switch back the caller */
>>      if (!sigsetjmp(self->env, 0)) {
>> +        start_switch_fiber(&fake_stack_save,
>> +                           leader.stack, leader.stack_size);
>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>      }
>>  
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      while (true) {
>>          co->entry(co->entry_arg);
>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>      ucontext_t old_uc, uc;
>>      sigjmp_buf old_env;
>>      union cc_arg arg = {0};
>> +    void *fake_stack_save = NULL;
>>  
>>      /* The ucontext functions preserve signal masks which incurs a
>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>  
>>      /* swapcontext() in, siglongjmp() back out */
>>      if (!sigsetjmp(old_env, 0)) {
>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>          swapcontext(&old_uc, &uc);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return &co->base;
>>  }
>>  
>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>      int ret;
>> +    void *fake_stack_save = NULL;
>>  
>>      current = to_;
>>  
>>      ret = sigsetjmp(from->env, 0);
>>      if (ret == 0) {
>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>          siglongjmp(to->env, action);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return ret;
>>  }
>>  
>>
> 

Hi

On Tue, Jan 9, 2018 at 12:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Marc-André,
>
> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>> the following patch.
>>
>> A similar work would need to be done for sigaltstack & windows fibers
>> to have similar coverage. Since ucontext is preferred, I didn't bother
>> checking the other coroutine implementations for now.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>>  include/qemu/compiler.h   |  4 ++++
>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+)
>>
>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>> index 340e5fdc09..5fcc4f7ec7 100644
>> --- a/include/qemu/compiler.h
>> +++ b/include/qemu/compiler.h
>> @@ -111,4 +111,8 @@
>>  #define GCC_FMT_ATTR(n, m)
>>  #endif
>>
>> +#ifndef __has_feature
>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>> +#endif
>> +
>>  #endif /* COMPILER_H */
>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>> index 6621f3f692..e78eae8766 100644
>> --- a/util/coroutine-ucontext.c
>> +++ b/util/coroutine-ucontext.c
>> @@ -31,6 +31,11 @@
>>  #include <valgrind/valgrind.h>
>>  #endif
>>
>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>> +#define CONFIG_ASAN 1
>> +#include <sanitizer/asan_interface.h>
>> +#endif
>
> Sadly this fails on Travis:
>
> $ export CONFIG="--enable-debug --enable-debug-tcg
> --enable-trace-backends=log"
> $ export CC=gcc
> $ ./configure ${CONFIG}
> C compiler        gcc
> Host C compiler   cc
> C++ compiler      c++
> Objective-C compiler clang
> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
> [...]
>   CC      util/qemu-coroutine.o
>   CC      util/qemu-coroutine-lock.o
>   CC      util/qemu-coroutine-io.o
>   CC      util/qemu-coroutine-sleep.o
>   CC      util/coroutine-ucontext.o
> util/coroutine-ucontext.c:36:38: fatal error:
> sanitizer/asan_interface.h: No such file or directory
>  #include <sanitizer/asan_interface.h>
>                                       ^
> compilation terminated.
> make: *** [util/coroutine-ucontext.o] Error 1
>
> You simply need to update the .travis.yml, but does that mean your

I think we need libgcc-6-dev.

> previous patch "Enable ASAN/UBSan by default if the compiler supports
> it." isn't correct?

No, the problematic patch is "ucontext: annotate coroutine stack for
ASAN", it should probably check for asan_interface.h before using it.

Print a warning if it's not there during configure, as ASAN
experience/reports will be inferior.



>
>> +
>>  typedef struct {
>>      Coroutine base;
>>      void *stack;
>> @@ -59,11 +64,37 @@ union cc_arg {
>>      int i[2];
>>  };
>>
>> +static void finish_switch_fiber(void *fake_stack_save)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    const void *bottom_old;
>> +    size_t size_old;
>> +
>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>> +
>> +    if (!leader.stack) {
>> +        leader.stack = (void *)bottom_old;
>> +        leader.stack_size = size_old;
>> +    }
>> +#endif
>> +}
>> +
>> +static void start_switch_fiber(void **fake_stack_save,
>> +                               const void *bottom, size_t size)
>> +{
>> +#ifdef CONFIG_ASAN
>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>> +#endif
>> +}
>> +
>>  static void coroutine_trampoline(int i0, int i1)
>>  {
>>      union cc_arg arg;
>>      CoroutineUContext *self;
>>      Coroutine *co;
>> +    void *fake_stack_save = NULL;
>> +
>> +    finish_switch_fiber(NULL);
>>
>>      arg.i[0] = i0;
>>      arg.i[1] = i1;
>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>
>>      /* Initialize longjmp environment and switch back the caller */
>>      if (!sigsetjmp(self->env, 0)) {
>> +        start_switch_fiber(&fake_stack_save,
>> +                           leader.stack, leader.stack_size);
>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>      }
>>
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      while (true) {
>>          co->entry(co->entry_arg);
>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>      ucontext_t old_uc, uc;
>>      sigjmp_buf old_env;
>>      union cc_arg arg = {0};
>> +    void *fake_stack_save = NULL;
>>
>>      /* The ucontext functions preserve signal masks which incurs a
>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>
>>      /* swapcontext() in, siglongjmp() back out */
>>      if (!sigsetjmp(old_env, 0)) {
>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>          swapcontext(&old_uc, &uc);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return &co->base;
>>  }
>>
>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>      int ret;
>> +    void *fake_stack_save = NULL;
>>
>>      current = to_;
>>
>>      ret = sigsetjmp(from->env, 0);
>>      if (ret == 0) {
>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>          siglongjmp(to->env, action);
>>      }
>> +
>> +    finish_switch_fiber(fake_stack_save);
>> +
>>      return ret;
>>  }
>>
>>
>



-- 
Marc-André Lureau

Hi,

On Tue, Jan 9, 2018 at 12:23 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Jan 9, 2018 at 12:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Marc-André,
>>
>> On 01/04/2018 01:05 PM, Marc-André Lureau wrote:
>>> It helps ASAN to detect more leaks on coroutine stacks, as found in
>>> the following patch.
>>>
>>> A similar work would need to be done for sigaltstack & windows fibers
>>> to have similar coverage. Since ucontext is preferred, I didn't bother
>>> checking the other coroutine implementations for now.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  include/qemu/compiler.h   |  4 ++++
>>>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 50 insertions(+)
>>>
>>> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
>>> index 340e5fdc09..5fcc4f7ec7 100644
>>> --- a/include/qemu/compiler.h
>>> +++ b/include/qemu/compiler.h
>>> @@ -111,4 +111,8 @@
>>>  #define GCC_FMT_ATTR(n, m)
>>>  #endif
>>>
>>> +#ifndef __has_feature
>>> +#define __has_feature(x) 0 /* compatibility with non-clang compilers */
>>> +#endif
>>> +
>>>  #endif /* COMPILER_H */
>>> diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
>>> index 6621f3f692..e78eae8766 100644
>>> --- a/util/coroutine-ucontext.c
>>> +++ b/util/coroutine-ucontext.c
>>> @@ -31,6 +31,11 @@
>>>  #include <valgrind/valgrind.h>
>>>  #endif
>>>
>>> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
>>> +#define CONFIG_ASAN 1
>>> +#include <sanitizer/asan_interface.h>
>>> +#endif
>>
>> Sadly this fails on Travis:
>>
>> $ export CONFIG="--enable-debug --enable-debug-tcg
>> --enable-trace-backends=log"
>> $ export CC=gcc
>> $ ./configure ${CONFIG}
>> C compiler        gcc
>> Host C compiler   cc
>> C++ compiler      c++
>> Objective-C compiler clang
>> CFLAGS            -Wno-maybe-uninitialized -Og -fsanitize=address -g
>> [...]
>>   CC      util/qemu-coroutine.o
>>   CC      util/qemu-coroutine-lock.o
>>   CC      util/qemu-coroutine-io.o
>>   CC      util/qemu-coroutine-sleep.o
>>   CC      util/coroutine-ucontext.o
>> util/coroutine-ucontext.c:36:38: fatal error:
>> sanitizer/asan_interface.h: No such file or directory
>>  #include <sanitizer/asan_interface.h>
>>                                       ^
>> compilation terminated.
>> make: *** [util/coroutine-ucontext.o] Error 1
>>
>> You simply need to update the .travis.yml, but does that mean your
>
> I think we need libgcc-6-dev.
>
>> previous patch "Enable ASAN/UBSan by default if the compiler supports
>> it." isn't correct?
>
> No, the problematic patch is "ucontext: annotate coroutine stack for
> ASAN", it should probably check for asan_interface.h before using it.
>
> Print a warning if it's not there during configure, as ASAN
> experience/reports will be inferior.
>

This should fix it:

(Paolo, would you like me to resend or will you review the diff &
squash? thanks)

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index e78eae8766..62fce888fd 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -32,9 +32,11 @@
 #endif

 #if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#if HAVE_ASAN_IFACE_H
 #define CONFIG_ASAN 1
 #include <sanitizer/asan_interface.h>
 #endif
+#endif

 typedef struct {
     Coroutine base;
diff --git a/configure b/configure
index d033286b48..406ccd7cb4 100755
--- a/configure
+++ b/configure
@@ -5185,6 +5185,13 @@ if compile_prog "" "" ; then
     have_utmpx=yes
 fi

+##########################################
+# check for asan header
+have_asan_iface_h=yes
+if ! check_include "sanitizer/asan_interface.h" ; then
+    have_asan_iface_h=no
+fi
+
 ##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
@@ -5198,6 +5205,10 @@ elif test "$debug" = "yes"; then
   write_c_skeleton;
   if compile_prog "-fsanitize=address" ""; then
       CFLAGS="-fsanitize=address $CFLAGS"
+      if test "$have_asan_iface_h" = "no" ; then
+          print_error "ASAN build enabled, but ASAN header missing." \
+                      "Without code annotation, the report may be inferior."
+      fi
   fi
   if compile_prog "-fsanitize=undefined" ""; then
       CFLAGS="-fsanitize=undefined $CFLAGS"
@@ -6320,6 +6331,10 @@ if test "$have_utmpx" = "yes" ; then
   echo "HAVE_UTMPX=y" >> $config_host_mak
 fi

+if test "$have_asan_iface_h" = "yes" ; then
+    echo "HAVE_ASAN_IFACE_H=y" >> $config_host_mak
+fi
+
 if test "$ivshmem" = "yes" ; then
   echo "CONFIG_IVSHMEM=y" >> $config_host_mak
 fi


diff --git a/.travis.yml b/.travis.yml
index f583839755..f2291e87a6 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,12 +13,13 @@ addons:
       - libattr1-dev
       - libbrlapi-dev
       - libcap-ng-dev
+      - libgcc-6-dev
       - libgnutls-dev
       - libgtk-3-dev
       - libiscsi-dev
       - liblttng-ust-dev
-      - libnfs-dev
       - libncurses5-dev
+      - libnfs-dev
       - libnss3-dev
       - libpixman-1-dev
       - libpng12-dev

>
>
>>
>>> +
>>>  typedef struct {
>>>      Coroutine base;
>>>      void *stack;
>>> @@ -59,11 +64,37 @@ union cc_arg {
>>>      int i[2];
>>>  };
>>>
>>> +static void finish_switch_fiber(void *fake_stack_save)
>>> +{
>>> +#ifdef CONFIG_ASAN
>>> +    const void *bottom_old;
>>> +    size_t size_old;
>>> +
>>> +    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
>>> +
>>> +    if (!leader.stack) {
>>> +        leader.stack = (void *)bottom_old;
>>> +        leader.stack_size = size_old;
>>> +    }
>>> +#endif
>>> +}
>>> +
>>> +static void start_switch_fiber(void **fake_stack_save,
>>> +                               const void *bottom, size_t size)
>>> +{
>>> +#ifdef CONFIG_ASAN
>>> +    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
>>> +#endif
>>> +}
>>> +
>>>  static void coroutine_trampoline(int i0, int i1)
>>>  {
>>>      union cc_arg arg;
>>>      CoroutineUContext *self;
>>>      Coroutine *co;
>>> +    void *fake_stack_save = NULL;
>>> +
>>> +    finish_switch_fiber(NULL);
>>>
>>>      arg.i[0] = i0;
>>>      arg.i[1] = i1;
>>> @@ -72,9 +103,13 @@ static void coroutine_trampoline(int i0, int i1)
>>>
>>>      /* Initialize longjmp environment and switch back the caller */
>>>      if (!sigsetjmp(self->env, 0)) {
>>> +        start_switch_fiber(&fake_stack_save,
>>> +                           leader.stack, leader.stack_size);
>>>          siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
>>>      }
>>>
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      while (true) {
>>>          co->entry(co->entry_arg);
>>>          qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
>>> @@ -87,6 +122,7 @@ Coroutine *qemu_coroutine_new(void)
>>>      ucontext_t old_uc, uc;
>>>      sigjmp_buf old_env;
>>>      union cc_arg arg = {0};
>>> +    void *fake_stack_save = NULL;
>>>
>>>      /* The ucontext functions preserve signal masks which incurs a
>>>       * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
>>> @@ -122,8 +158,12 @@ Coroutine *qemu_coroutine_new(void)
>>>
>>>      /* swapcontext() in, siglongjmp() back out */
>>>      if (!sigsetjmp(old_env, 0)) {
>>> +        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
>>>          swapcontext(&old_uc, &uc);
>>>      }
>>> +
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      return &co->base;
>>>  }
>>>
>>> @@ -169,13 +209,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
>>>      CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
>>>      CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
>>>      int ret;
>>> +    void *fake_stack_save = NULL;
>>>
>>>      current = to_;
>>>
>>>      ret = sigsetjmp(from->env, 0);
>>>      if (ret == 0) {
>>> +        start_switch_fiber(action == COROUTINE_TERMINATE ?
>>> +                           NULL : &fake_stack_save, to->stack, to->stack_size);
>>>          siglongjmp(to->env, action);
>>>      }
>>> +
>>> +    finish_switch_fiber(fake_stack_save);
>>> +
>>>      return ret;
>>>  }
>>>
>>>
>>
>
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

On 12/01/2018 12:15, Marc-André Lureau wrote:
>>> You simply need to update the .travis.yml, but does that mean your
>> I think we need libgcc-6-dev.
>>
>>> previous patch "Enable ASAN/UBSan by default if the compiler supports
>>> it." isn't correct?
>> No, the problematic patch is "ucontext: annotate coroutine stack for
>> ASAN", it should probably check for asan_interface.h before using it.
>>
>> Print a warning if it's not there during configure, as ASAN
>> experience/reports will be inferior.
>>
> This should fix it:
> 
> (Paolo, would you like me to resend or will you review the diff &
> squash? thanks)

I'll integrate it, thanks.

Paolo