Re: [Qemu-devel] [PATCH v3 15/18] ucontext: annotate coroutine stack for ASAN

[Philippe Mathieu-Daudé]

> 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 <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Reviewed-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  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;
>>  }
>>  
>>
>

signature.asc
Description: OpenPGP digital signature