Discussion:
[PATCH] Enable -fsanitize-recover for KASan
Yury Gribov
2014-09-05 06:54:27 UTC
Permalink
Hi all,

This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop

Bootstrapped and regtested on x64.

Ok to commit?

-Y
Dmitry Vyukov
2014-09-05 07:32:19 UTC
Permalink
Post by Yury Gribov
Hi all,
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline instrumentation.
This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
Bootstrapped and regtested on x64.
Ok to commit?
Can we do it w/o doubling number of runtime entry points?
Looks like a wrong way to pass global options to runtime. If we need
to pass several other options, number of entry points will sky rocket.
I vaguely remember there are some globals that compiler uses to
communicate configuration (version, shadow base) to runtime. Can we
use them to enable recover mode in runtime?
Yury Gribov
2014-09-05 08:12:51 UTC
Permalink
Post by Dmitry Vyukov
Can we do it w/o doubling number of runtime entry points?
I didn't find a good way to achieve this. See, normal asan reporting
functions
have a noreturn attribute (defined in sanitizer.def) which can't be changed
depending on cmdline flag.
Post by Dmitry Vyukov
Looks like a wrong way to pass global options to runtime.
If we need
to pass several other options, number of entry points will sky rocket.
We only need new definitions if functions look significantly different
for compiler
(noreturn, etc.).
Post by Dmitry Vyukov
I vaguely remember there are some globals that compiler uses to
communicate configuration (version, shadow base) to runtime.
This would make the recovery setting global then.
It may be desirable to have per-object (or even per-function) options
(or maybe not).

-Y
Dmitry Vyukov
2014-09-05 08:23:25 UTC
Permalink
Post by Yury Gribov
Post by Dmitry Vyukov
Can we do it w/o doubling number of runtime entry points?
I didn't find a good way to achieve this. See, normal asan reporting
functions
have a noreturn attribute (defined in sanitizer.def) which can't be changed
depending on cmdline flag.
I have not looked at the code in detail. But it looks weird to me that
in a general-purpose programming language we can't alter an attribute
of an in-memory object. I would expect that the attribute is just a
field in a struct describing the functionm and the field can be freely
set/reset. Is not it the case?
Post by Yury Gribov
Post by Dmitry Vyukov
Looks like a wrong way to pass global options to runtime.
If we need
to pass several other options, number of entry points will sky rocket.
We only need new definitions if functions look significantly different for
compiler
(noreturn, etc.).
Post by Dmitry Vyukov
I vaguely remember there are some globals that compiler uses to
communicate configuration (version, shadow base) to runtime.
This would make the recovery setting global then.
It may be desirable to have per-object (or even per-function) options (or
maybe not).
This makes sense.
Leaving for others to chime in.
Yury Gribov
2014-09-05 08:33:54 UTC
Permalink
Post by Dmitry Vyukov
Post by Yury Gribov
I didn't find a good way to achieve this. See, normal asan reporting
functions
have a noreturn attribute (defined in sanitizer.def) which can't be changed
depending on cmdline flag.
I have not looked at the code in detail. But it looks weird to me that
in a general-purpose programming language we can't alter an attribute
of an in-memory object.
Well, builtins are described by metadata in .def files
(those are not written in general purpose language).
Post-hacking generated trees sounds ugly...

-Y
Jakub Jelinek
2014-09-05 09:28:10 UTC
Permalink
Post by Yury Gribov
Post by Dmitry Vyukov
Post by Yury Gribov
I didn't find a good way to achieve this. See, normal asan reporting
functions
have a noreturn attribute (defined in sanitizer.def) which can't be changed
depending on cmdline flag.
I have not looked at the code in detail. But it looks weird to me that
in a general-purpose programming language we can't alter an attribute
of an in-memory object.
Well, builtins are described by metadata in .def files
(those are not written in general purpose language).
Post-hacking generated trees sounds ugly...
Note, in ubsan you also have __ubsan_something and __ubsan_something_abort
entrypoints, the latter is noreturn and terminates program, the former
is not and is used for -fsanitize-recover.

Though, for that option the default is yes for ubsan purposes, while asan
wants a default to no, so it is unclear how to solve that. Either different
option for asan recovery, or tristate option etc.

Jakub
Yury Gribov
2014-09-05 09:32:48 UTC
Permalink
Post by Jakub Jelinek
Though, for that option the default is yes for ubsan purposes, while asan
wants a default to no, so it is unclear how to solve that. Either different
option for asan recovery, or tristate option etc.
Currently I force set it on encountering -fsanitize=... (true for UBSan
and KASan, false otherwise).
Can do tristate as well.

-Y
Andrey Ryabinin
2014-09-05 08:59:00 UTC
Permalink
Post by Yury Gribov
Hi all,
This patch enables -fsanitize-recover for KASan by default. This causes KASan to continue execution after error in case of inline instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
I just add that this option is required for inline instrumentation in kernel.
There is some places in kernel where we validly touch poisoned memory
and we need to disable error reporting in runtime. For that we use per task variable and check it
__asan_report function and don't print anything if flag is raised.
So there is must be the way to return from report functions.

And how does it work if someone wants to try -fsanitize=address -fsanitize-recover.
Seems you didn't touch libsanitzer in this patch, so I guess this will cause link time error, right ?
Post by Yury Gribov
Bootstrapped and regtested on x64.
Ok to commit?
-Y
Yury Gribov
2014-09-05 09:12:52 UTC
Permalink
Post by Andrey Ryabinin
And how does it work if someone wants to try -fsanitize=address
-fsanitize-recover.
Post by Andrey Ryabinin
Seems you didn't touch libsanitzer in this patch,
so I guess this will cause link time error, right ?
Exactly, Asan team does not want recovery mode for userspace Asan.
BTW it may make sense to emit a friendly error message instead
of those link errors.

-Y
Yury Gribov
2014-09-15 09:38:42 UTC
Permalink
Post by Yury Gribov
Hi all,
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
Bootstrapped and regtested on x64.
Ok to commit?
-Y
Rebased patch to current trunk.

-Y
Jakub Jelinek
2014-09-18 10:52:51 UTC
Permalink
--- a/gcc/builtins.def
+++ b/gcc/builtins.def
@@ -176,7 +176,7 @@ along with GCC; see the file COPYING3. If not see
DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE, \
true, true, true, ATTRS, true, \
(flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
- | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)))
+ | SANITIZE_UNDEFINED | SANITIZE_UNDEFINED_NONDEFAULT)))
This is too long line after the change.
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -8236,7 +8236,7 @@ sanitize_spec_function (int argc, const char **argv)
if (strcmp (argv[0], "thread") == 0)
return (flag_sanitize & SANITIZE_THREAD) ? "" : NULL;
if (strcmp (argv[0], "undefined") == 0)
- return ((flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT))
+ return ((flag_sanitize & (SANITIZE_UNDEFINED | SANITIZE_UNDEFINED_NONDEFAULT))
Likewise.
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1551,6 +1551,12 @@ common_handle_option (struct gcc_options *opts,
| SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
opts->x_flag_delete_null_pointer_checks = 0;
+ /* UBSan and KASan enable recovery by default. */
+ opts->x_flag_sanitize_recover
+ = !!(flag_sanitize & (SANITIZE_UNDEFINED
+ | SANITIZE_UNDEFINED_NONDEFAULT
+ | SANITIZE_KERNEL_ADDRESS));
+
Doesn't this override even user supplied -fsanitize-recover or
-fno-sanitize-recover ? Have you tried both
-fno-sanitize-recover -fsanitize=kernel-address
and
-fsanitize=kernel-address -fno-sanitize-recover
option orders?

Seems for -fdelete-null-pointer-checks we got it wrong too,
IMHO for -fsanitize={null,{,returns-}nonnull-attribute,undefined}
we want to disable it unconditionally, regardless of whether
that option appears on the command line or not.
And we handle it right for
-fdelete-null-pointer-checks -fsanitize=undefined
but not for
-fsanitize=undefined -fdelete-null-pointer-checks
Joseph, thoughts where to override it instead (I mean, after all
options are processed)?

In the -fsanitize-recover case, I'd on the other side think that
it should just override the default and not override explicit
user's decision. Which could be done here, but supposedly guarded
with if (!opts_set->x_flag_sanitize_recover)?

I don't think your proposal will work properly though,
if one compiles with
-fsanitize=undefined -fsanitize=address
you'll just get userland asan with error recovery, which is highly
undesirable (not just that it changes the behavior from how it
behaved before, but especially because libasan doesn't contain
such entrypoints at all).
-fsanitize=undefined,address
or
-fsanitize=address,undefined
is normal supported mode and thus I think you either can't reuse
-fsanitize-recover option for what you want to do, or
asan.c needs to limit it to flag_sanitize & SANITIZE_KERNEL_ADDRESS
mode only. Depends if you ever want to add recovery for userland
sanitization.

Jakub
Yury Gribov
2014-09-18 13:15:10 UTC
Permalink
Added Marek to comment on proposed UBSan option change.
Post by Jakub Jelinek
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -1551,6 +1551,12 @@ common_handle_option (struct gcc_options *opts,
| SANITIZE_RETURNS_NONNULL_ATTRIBUTE))
opts->x_flag_delete_null_pointer_checks = 0;
+ /* UBSan and KASan enable recovery by default. */
+ opts->x_flag_sanitize_recover
+ = !!(flag_sanitize & (SANITIZE_UNDEFINED
+ | SANITIZE_UNDEFINED_NONDEFAULT
+ | SANITIZE_KERNEL_ADDRESS));
+
Doesn't this override even user supplied -fsanitize-recover or
-fno-sanitize-recover ? Have you tried both
-fno-sanitize-recover -fsanitize=kernel-address
and
-fsanitize=kernel-address -fno-sanitize-recover
option orders?
I did and this worked in a seemingly logical way:
* -fsanitize=address (disable recovery)
* -fsanitize-recover -fsanitize=address (disable recovery)
* -fsanitize=address -fsanitize-recover (enable recovery)
* -fsanitize=kernel-address (enable recovery)
* -fno-sanitize-recover -fsanitize=kernel-address (enable recovery)
* -fsanitize=kernel-address -fno-sanitize-recover (enable recovery)
Post by Jakub Jelinek
Seems for -fdelete-null-pointer-checks we got it wrong too,
IMHO for -fsanitize={null,{,returns-}nonnull-attribute,undefined}
we want to disable it unconditionally, regardless of whether
that option appears on the command line or not.
My understanding is that all
-fsanitize=(address|kernel-address|undefined|you-name-it) are simply
packs of options to enable. User may override any selected option from
the pack if he so desires.
Post by Jakub Jelinek
I don't think your proposal will work properly though,
if one compiles with
-fsanitize=undefined -fsanitize=address
you'll just get userland asan with error recovery, which is highly
undesirable
Now that's a problem. Looks like I'll need a separate flag to achieve
what I need (-fasan-recover? And maybe then rename -fsanitize-recover to
-fubsan-recover for consistency?).
Post by Jakub Jelinek
or asan.c needs to limit it to flag_sanitize & SANITIZE_KERNEL_ADDRESS
mode only.
We may want to UBsanitize kernel in future and this may cause the same
problem as for userspace Asan/UBSan interaction you described above.
Post by Jakub Jelinek
Depends if you ever want to add recovery for userland
sanitization.
Also kernel developers want both recoverable (more user-friendly) and
non-recoverable (faster) Asan error handling.

-Y
Joseph S. Myers
2014-09-18 23:47:34 UTC
Permalink
Post by Jakub Jelinek
Seems for -fdelete-null-pointer-checks we got it wrong too,
IMHO for -fsanitize={null,{,returns-}nonnull-attribute,undefined}
we want to disable it unconditionally, regardless of whether
that option appears on the command line or not.
And we handle it right for
-fdelete-null-pointer-checks -fsanitize=undefined
but not for
-fsanitize=undefined -fdelete-null-pointer-checks
Joseph, thoughts where to override it instead (I mean, after all
options are processed)?
finish_options is the obvious place to do that.
--
Joseph S. Myers
***@codesourcery.com
Yury Gribov
2014-09-29 17:21:11 UTC
Permalink
Hi all,
Post by Yury Gribov
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
This is the third version of patch which renames -fsanitize-recover to
-fubsan-recover and introduces -fasan-recover (enabled by default for
KASan). It also moves flag handling to finish_options per Jakub's request.

Bootstrapped and regtested on x64.

-Y
Jakub Jelinek
2014-09-29 17:43:57 UTC
Permalink
Post by Yury Gribov
Post by Yury Gribov
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
This is the third version of patch which renames -fsanitize-recover to
-fubsan-recover and introduces -fasan-recover (enabled by default for
KASan). It also moves flag handling to finish_options per Jakub's request.
As the -fsanitize-recover option comes from clang originally, I think
this needs coordination with them (whether clang will also rename the
option), and certainly keep -fsanitize-recover as a non-documented
compat option alias for -fubsan-recover.
So, can you please talk to the clang folks about it?

Jakub
Konstantin Serebryany
2014-09-29 21:20:17 UTC
Permalink
+Alexey Samsonov
Post by Jakub Jelinek
Post by Yury Gribov
Post by Yury Gribov
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
This is the third version of patch which renames -fsanitize-recover to
-fubsan-recover and introduces -fasan-recover (enabled by default for
KASan). It also moves flag handling to finish_options per Jakub's request.
As the -fsanitize-recover option comes from clang originally, I think
this needs coordination with them (whether clang will also rename the
option), and certainly keep -fsanitize-recover as a non-documented
compat option alias for -fubsan-recover.
So, can you please talk to the clang folks about it?
Jakub
Alexey Samsonov
2014-09-29 22:37:16 UTC
Permalink
(resending in plain-text mode)

-fasan-recover doesn't look like a good idea - for instance, in Clang,
we never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale
behind splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that
purpose? Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.

On Mon, Sep 29, 2014 at 2:20 PM, Konstantin Serebryany
Post by Konstantin Serebryany
+Alexey Samsonov
Post by Jakub Jelinek
Post by Yury Gribov
Post by Yury Gribov
This patch enables -fsanitize-recover for KASan by default. This causes
KASan to continue execution after error in case of inline
instrumentation. This feature is needed because
- reports during early bootstrap won't even be printed
- needed to run all tests w/o rebooting machine for every test
- needed for interactive work on desktop
This is the third version of patch which renames -fsanitize-recover to
-fubsan-recover and introduces -fasan-recover (enabled by default for
KASan). It also moves flag handling to finish_options per Jakub's request.
As the -fsanitize-recover option comes from clang originally, I think
this needs coordination with them (whether clang will also rename the
option), and certainly keep -fsanitize-recover as a non-documented
compat option alias for -fubsan-recover.
So, can you please talk to the clang folks about it?
Jakub
--
Alexey Samsonov, Mountain View, CA
Jakub Jelinek
2014-09-29 23:17:20 UTC
Permalink
-fasan-recover doesn't look like a good idea - for instance, in Clang, we
never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale behind
splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that purpose?
Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.
The problem is that if we start using it for ASan, it needs to have a
different default, because ASan wants to abort by default, while UBSan
recover by default. -fsanitize=kernel-address w (KASan) wants to recover
by default. So, the option is either to never support recover for
-fsanitize=address, for ubsan keep -fsanitize-recover (by default) as is
and for kasan use that same switch, or have separate flags.

Jakub
Alexey Samsonov
2014-09-29 23:26:24 UTC
Permalink
Post by Jakub Jelinek
-fasan-recover doesn't look like a good idea - for instance, in Clang, we
never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale behind
splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that purpose?
Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.
The problem is that if we start using it for ASan, it needs to have a
different default, because ASan wants to abort by default, while UBSan
recover by default. -fsanitize=kernel-address w (KASan) wants to recover
by default. So, the option is either to never support recover for
-fsanitize=address, for ubsan keep -fsanitize-recover (by default) as is
and for kasan use that same switch, or have separate flags.
Jakub
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
-fsanitize=kernel-address. We can, potentially, extend
-fsanitize-recover flag to take the same values as -fsanitize= one,
so that one can specify which sanitizers are recoverable, and which
are not, but I'd try to make this a last resort - this is too complex.
--
Alexey Samsonov, Mountain View, CA
Konstantin Serebryany
2014-09-30 00:24:02 UTC
Permalink
Post by Alexey Samsonov
Post by Jakub Jelinek
-fasan-recover doesn't look like a good idea - for instance, in Clang, we
never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale behind
splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that purpose?
Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.
The problem is that if we start using it for ASan, it needs to have a
different default, because ASan wants to abort by default, while UBSan
recover by default. -fsanitize=kernel-address w (KASan) wants to recover
by default. So, the option is either to never support recover for
-fsanitize=address, for ubsan keep -fsanitize-recover (by default) as is
and for kasan use that same switch, or have separate flags.
Jakub
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
we can and probably will allow to recover from errors (glibc demands that),
but that does not require any compile-time flag.
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
Which default setting is stronger? :)
Post by Alexey Samsonov
-fsanitize=kernel-address. We can, potentially, extend
-fsanitize-recover flag to take the same values as -fsanitize= one,
so that one can specify which sanitizers are recoverable, and which
are not, but I'd try to make this a last resort - this is too complex.
--
Alexey Samsonov, Mountain View, CA
Alexey Samsonov
2014-09-30 01:05:27 UTC
Permalink
On Mon, Sep 29, 2014 at 5:24 PM, Konstantin Serebryany
Post by Konstantin Serebryany
Post by Alexey Samsonov
Post by Jakub Jelinek
-fasan-recover doesn't look like a good idea - for instance, in Clang, we
never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale behind
splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that purpose?
Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.
The problem is that if we start using it for ASan, it needs to have a
different default, because ASan wants to abort by default, while UBSan
recover by default. -fsanitize=kernel-address w (KASan) wants to recover
by default. So, the option is either to never support recover for
-fsanitize=address, for ubsan keep -fsanitize-recover (by default) as is
and for kasan use that same switch, or have separate flags.
Jakub
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
we can and probably will allow to recover from errors (glibc demands that),
but that does not require any compile-time flag.
So, if we switch to instrumentation to callbacks (which are not
necessarily noreturn),
-fsanitize-recover is the thing we plan to support?
Post by Konstantin Serebryany
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
Which default setting is stronger? :)
We can preserve the default behavior for each tool (no recovery for
ASan, recovery for UBSan)
unless recovery mode is explicitly specified.
Post by Konstantin Serebryany
Post by Alexey Samsonov
-fsanitize=kernel-address. We can, potentially, extend
-fsanitize-recover flag to take the same values as -fsanitize= one,
so that one can specify which sanitizers are recoverable, and which
are not, but I'd try to make this a last resort - this is too complex.
--
Alexey Samsonov, Mountain View, CA
--
Alexey Samsonov, Mountain View, CA
Konstantin Serebryany
2014-09-30 01:48:38 UTC
Permalink
Post by Alexey Samsonov
On Mon, Sep 29, 2014 at 5:24 PM, Konstantin Serebryany
Post by Konstantin Serebryany
Post by Alexey Samsonov
Post by Jakub Jelinek
-fasan-recover doesn't look like a good idea - for instance, in Clang, we
never use "?san"
in flag names, preferring -fsanitize-whatever. What's the rationale behind
splitting
-fsanitize-recover in two flags (ASan- and UBSan- specific)?
Is there no way to keep a single -f(no-)sanitize-recover for that purpose?
Now it works
only for UBSan checks, but we may extend it to another sanitizers as well.
The problem is that if we start using it for ASan, it needs to have a
different default, because ASan wants to abort by default, while UBSan
recover by default. -fsanitize=kernel-address w (KASan) wants to recover
by default. So, the option is either to never support recover for
-fsanitize=address, for ubsan keep -fsanitize-recover (by default) as is
and for kasan use that same switch, or have separate flags.
Jakub
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
we can and probably will allow to recover from errors (glibc demands that),
but that does not require any compile-time flag.
So, if we switch to instrumentation to callbacks (which are not
necessarily noreturn),
-fsanitize-recover is the thing we plan to support?
Yes, but this will not involve any additional compile-time flag
Post by Alexey Samsonov
Post by Konstantin Serebryany
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
Which default setting is stronger? :)
We can preserve the default behavior for each tool (no recovery for
ASan, recovery for UBSan)
unless recovery mode is explicitly specified.
Post by Konstantin Serebryany
Post by Alexey Samsonov
-fsanitize=kernel-address. We can, potentially, extend
-fsanitize-recover flag to take the same values as -fsanitize= one,
so that one can specify which sanitizers are recoverable, and which
are not, but I'd try to make this a last resort - this is too complex.
--
Alexey Samsonov, Mountain View, CA
--
Alexey Samsonov, Mountain View, CA
Jakub Jelinek
2014-09-30 05:40:27 UTC
Permalink
Post by Konstantin Serebryany
Post by Alexey Samsonov
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
The normal (non-recovery) callbacks are __attribute__((noreturn)) for
performance reasons, and you do need different callbacks and different
generated code if you want to recover (after the callback you need jump
back to a basic block after the conditional jump).
So, in that case you would need -fsanitize-recover=address.
Post by Konstantin Serebryany
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
That is fairly common case.

Jakub
Yury Gribov
2014-09-30 07:07:44 UTC
Permalink
Post by Jakub Jelinek
Post by Konstantin Serebryany
Post by Alexey Samsonov
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
The normal (non-recovery) callbacks are __attribute__((noreturn)) for
performance reasons, and you do need different callbacks and different
generated code if you want to recover (after the callback you need jump
back to a basic block after the conditional jump).
So, in that case you would need -fsanitize-recover=address.
Post by Konstantin Serebryany
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
That is fairly common case.
I think we can summarize:
* the current option -fsanitize-recover is misleading; it's really
-fubsan-recover
* we need a way to selectively enable/disable recovery for different
sanitizers

The most promininet solution seems to be
* allow -fsanitize-recover=tgt1,tgt2 syntax
* -fsanitize-recover wo options would still mean UBSan recovery

The question is what to do with -fno-sanitize-recover then.

-Y
Alexey Samsonov
2014-09-30 17:26:39 UTC
Permalink
Post by Yury Gribov
Post by Jakub Jelinek
Post by Konstantin Serebryany
Post by Alexey Samsonov
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
The normal (non-recovery) callbacks are __attribute__((noreturn)) for
performance reasons, and you do need different callbacks and different
generated code if you want to recover (after the callback you need jump
back to a basic block after the conditional jump).
So, in that case you would need -fsanitize-recover=address.
Post by Konstantin Serebryany
Post by Alexey Samsonov
I see no problem in enabling -fsanitize-recover by default for
-fsanitize=undefined and
This becomes more interesting when we use asan and ubsan together.
That is fairly common case.
* the current option -fsanitize-recover is misleading; it's really
-fubsan-recover
* we need a way to selectively enable/disable recovery for different
sanitizers
The most promininet solution seems to be
* allow -fsanitize-recover=tgt1,tgt2 syntax
* -fsanitize-recover wo options would still mean UBSan recovery
The question is what to do with -fno-sanitize-recover then.
We can make -f(no-)?sanitize-recover= flags accept the same values as
-f(no-)?sanitize= flags. In this case,

"-fsanitize-recover" will be a deprecated alias of
"-fsanitize-recover=undefined", and
"-fno-sanitize-recover" will be a deprecated alias of
"-fno-sanitize-recover=undefined".
If a user provides "-fsanitize-recover=address", we can instruct the
instrumentation pass to
use recoverable instrumentation.
Post by Yury Gribov
-Y
--
Alexey Samsonov, Mountain View, CA
Jakub Jelinek
2014-09-30 17:33:40 UTC
Permalink
Post by Alexey Samsonov
Post by Yury Gribov
* the current option -fsanitize-recover is misleading; it's really
-fubsan-recover
* we need a way to selectively enable/disable recovery for different
sanitizers
The most promininet solution seems to be
* allow -fsanitize-recover=tgt1,tgt2 syntax
* -fsanitize-recover wo options would still mean UBSan recovery
The question is what to do with -fno-sanitize-recover then.
We can make -f(no-)?sanitize-recover= flags accept the same values as
-f(no-)?sanitize= flags. In this case,
"-fsanitize-recover" will be a deprecated alias of
"-fsanitize-recover=undefined", and
"-fno-sanitize-recover" will be a deprecated alias of
"-fno-sanitize-recover=undefined".
If a user provides "-fsanitize-recover=address", we can instruct the
instrumentation pass to
use recoverable instrumentation.
Would we accept -fsanitize-recover=undefined -fno-sanitize-recover=signed-integer-overflow
as recovering everything but signed integer overflows, i.e. the decision
whether to recover a particular call would check similar bitmask as
is checked whether to sanitize something at all?

Jakub
Alexey Samsonov
2014-09-30 17:36:34 UTC
Permalink
Post by Jakub Jelinek
Post by Alexey Samsonov
Post by Yury Gribov
* the current option -fsanitize-recover is misleading; it's really
-fubsan-recover
* we need a way to selectively enable/disable recovery for different
sanitizers
The most promininet solution seems to be
* allow -fsanitize-recover=tgt1,tgt2 syntax
* -fsanitize-recover wo options would still mean UBSan recovery
The question is what to do with -fno-sanitize-recover then.
We can make -f(no-)?sanitize-recover= flags accept the same values as
-f(no-)?sanitize= flags. In this case,
"-fsanitize-recover" will be a deprecated alias of
"-fsanitize-recover=undefined", and
"-fno-sanitize-recover" will be a deprecated alias of
"-fno-sanitize-recover=undefined".
If a user provides "-fsanitize-recover=address", we can instruct the
instrumentation pass to
use recoverable instrumentation.
Would we accept -fsanitize-recover=undefined -fno-sanitize-recover=signed-integer-overflow
as recovering everything but signed integer overflows, i.e. the decision
whether to recover a particular call would check similar bitmask as
is checked whether to sanitize something at all?
Yes, the logic for creating a set of recoverable sanitizers should be
the same as the logic for creating a set of enabled sanitizers.
--
Alexey Samsonov, Mountain View, CA
Jakub Jelinek
2014-09-30 17:39:13 UTC
Permalink
Post by Alexey Samsonov
Post by Jakub Jelinek
Would we accept -fsanitize-recover=undefined -fno-sanitize-recover=signed-integer-overflow
as recovering everything but signed integer overflows, i.e. the decision
whether to recover a particular call would check similar bitmask as
is checked whether to sanitize something at all?
Yes, the logic for creating a set of recoverable sanitizers should be
the same as the logic for creating a set of enabled sanitizers.
LGTM, will hack it up soon in GCC then.

Jakub
Alexey Samsonov
2014-10-01 23:21:29 UTC
Permalink
Speaking of plain -f(no-)sanitize-recover - it would probably be
better to change the semantics of this flag,
so that "-f(no-)?sanitize-recover" means "enable(disable) recovery for
all sanitizers enabled at this point".
That is, it would be pretty much like -Werror flag.

For example,
"-fsanitize=undefined -fsanitize=address -fno-sanitize-recover"
would mean "run UBSan and ASan and don't recover from errors".
Post by Jakub Jelinek
Post by Alexey Samsonov
Post by Jakub Jelinek
Would we accept -fsanitize-recover=undefined -fno-sanitize-recover=signed-integer-overflow
as recovering everything but signed integer overflows, i.e. the decision
whether to recover a particular call would check similar bitmask as
is checked whether to sanitize something at all?
Yes, the logic for creating a set of recoverable sanitizers should be
the same as the logic for creating a set of enabled sanitizers.
LGTM, will hack it up soon in GCC then.
Jakub
--
Alexey Samsonov, Mountain View, CA
Jakub Jelinek
2014-10-02 05:58:19 UTC
Permalink
Post by Alexey Samsonov
Speaking of plain -f(no-)sanitize-recover - it would probably be
better to change the semantics of this flag,
so that "-f(no-)?sanitize-recover" means "enable(disable) recovery for
all sanitizers enabled at this point".
That is, it would be pretty much like -Werror flag.
For example,
"-fsanitize=undefined -fsanitize=address -fno-sanitize-recover"
would mean "run UBSan and ASan and don't recover from errors".
That would change behavior, e.g. for
-fsanitize=undefined,address -fsanitize-recover
would suddenly enable recovery from asan errors while previously
they wouldn't be recovering.

GCC has not shipped with the -fsanitize-recover flag yet (we have just
vendor backport of it), so if you don't mind changing behavior for clang
users, I can live with that. Would the default still be
-fsanitize-recover=undefined,kernel-address -fno-sanitize-recover=address ?

Jakub
Alexey Samsonov
2014-10-03 18:54:44 UTC
Permalink
Post by Jakub Jelinek
Post by Alexey Samsonov
Speaking of plain -f(no-)sanitize-recover - it would probably be
better to change the semantics of this flag,
so that "-f(no-)?sanitize-recover" means "enable(disable) recovery for
all sanitizers enabled at this point".
That is, it would be pretty much like -Werror flag.
For example,
"-fsanitize=undefined -fsanitize=address -fno-sanitize-recover"
would mean "run UBSan and ASan and don't recover from errors".
That would change behavior, e.g. for
-fsanitize=undefined,address -fsanitize-recover
would suddenly enable recovery from asan errors while previously
they wouldn't be recovering.
GCC has not shipped with the -fsanitize-recover flag yet (we have just
vendor backport of it), so if you don't mind changing behavior for clang
users, I can live with that.
Yes, I think so. -fsanitize-recover was not documented in Clang user manual.
Post by Jakub Jelinek
Would the default still be
-fsanitize-recover=undefined,kernel-address -fno-sanitize-recover=address ?
Yes.
--
Alexey Samsonov, Mountain View, CA
Yury Gribov
2014-09-30 06:56:57 UTC
Permalink
Post by Konstantin Serebryany
Post by Alexey Samsonov
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
we can and probably will allow to recover from errors (glibc demands that),
but that does not require any compile-time flag.
I don't know details but are you absolutely sure that you won't want to
do inline instrumentation of glibc in the future? This would then
require -fasan-recover.

-Y
Yury Gribov
2014-09-30 07:14:34 UTC
Permalink
Post by Yury Gribov
Post by Konstantin Serebryany
Post by Alexey Samsonov
I don't think we ever going to support recovery for regular ASan
(Kostya, correct me if I'm wrong).
I hope so too.
Another point is that with asan-instrumentation-with-call-threshold=0
(instrumentation with callbacks)
we can and probably will allow to recover from errors (glibc demands that),
but that does not require any compile-time flag.
I don't know details but are you absolutely sure that you won't want to
do inline instrumentation of glibc in the future? This would then
require -fasan-recover.
FYI in kernel we had exactly this situation: outline instrumentation
allowed us to hide recovery inside callbacks but then turned out to be
too slow so we are now switching back to inline instrumentation (which
requires -fasan-recovery).

-Y
Loading...