| Message ID | 20180104160523.22995-16-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >> } >> >> >
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; }