Re: [PATCH] opts: SANITIZE_ADDRESS wrongly cleared [PR108543]

[Andrew Pinski <pinskia@gmail.com>]

On Wed, Jan 25, 2023 at 3:26 PM Marek Polacek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Here we crash on a null fndecl ultimately because we haven't defined
> the built-ins described in sanitizer.def.  So
> builtin_decl_explicit (BUILT_IN_ASAN_POINTER_SUBTRACT);
> returns NULL_TREE, causing an ICE later.
>
> DEF_SANITIZER_BUILTIN only actually defines the built-ins when flag_sanitize
> has SANITIZE_ADDRESS, or some of the other SANITIZE_*, but it doesn't check
> SANITIZE_KERNEL_ADDRESS or SANITIZE_USER_ADDRESS.  Unfortunately, with
> -fsanitize=address -fno-sanitize=kernel-address
> or
> -fsanitize=kernel-address -fno-sanitize=address
> SANITIZE_ADDRESS ends up being unset from flag_sanitize even though
> _USER/_KERNEL are set.  That's because -fsanitize=address means
> SANITIZE_ADDRESS | SANITIZE_USER_ADDRESS and -fsanitize=kernel-address
> is SANITIZE_ADDRESS | SANITIZE_KERNEL_ADDRESS but parse_sanitizer_options
> does
>   flags &= ~sanitizer_opts[i].flag;
> so the subsequent -fno- unsets SANITIZE_ADDRESS.  Then no sanitizer
> built-ins are actually defined.
>
> I'm not sure why SANITIZE_ADDRESS isn't just SANITIZE_USER_ADDRESS |
> SANITIZE_KERNEL_ADDRESS, I don't think we need 3 bits.

I am trying to follow the code but I can't tell if -fsanitize=address
-fno-sanitize=address still will work?
It would make sense to add a testcase for that case too.

I suspect it does not work though, because it would still leave
SANITIZE_ADDRESS enabled ...

Thanks,
Andrew

>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk/12/11?
>
>         PR middle-end/108543
>
> gcc/ChangeLog:
>
>         * opts.cc (parse_sanitizer_options): Don't clear SANITIZE_ADDRESS if it
>         was previously set.
>
> gcc/testsuite/ChangeLog:
>
>         * c-c++-common/asan/pointer-subtract-5.c: New test.
>         * c-c++-common/asan/pointer-subtract-6.c: New test.
>         * c-c++-common/asan/pointer-subtract-7.c: New test.
>         * c-c++-common/asan/pointer-subtract-8.c: New test.
> ---
>  gcc/opts.cc                                       |  5 ++++-
>  .../c-c++-common/asan/pointer-subtract-5.c        | 15 +++++++++++++++
>  .../c-c++-common/asan/pointer-subtract-6.c        | 15 +++++++++++++++
>  .../c-c++-common/asan/pointer-subtract-7.c        | 15 +++++++++++++++
>  .../c-c++-common/asan/pointer-subtract-8.c        | 15 +++++++++++++++
>  5 files changed, 64 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-5.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-6.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-7.c
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pointer-subtract-8.c
>
> diff --git a/gcc/opts.cc b/gcc/opts.cc
> index 9ba47d7deaa..c042b03bd9f 100644
> --- a/gcc/opts.cc
> +++ b/gcc/opts.cc
> @@ -2246,7 +2246,10 @@ parse_sanitizer_options (const char *p, location_t loc, int scode,
>                   flags |= sanitizer_opts[i].flag;
>               }
>             else
> -             flags &= ~sanitizer_opts[i].flag;
> +             /* Don't clear SANITIZE_ADDRESS if it was previously set;
> +                -fsanitize=address -fno-sanitize=kernel-address should
> +                leave SANITIZE_ADDRESS set.  */
> +             flags &= (~sanitizer_opts[i].flag | SANITIZE_ADDRESS);
>             found = true;
>             break;
>           }
> diff --git a/gcc/testsuite/c-c++-common/asan/pointer-subtract-5.c b/gcc/testsuite/c-c++-common/asan/pointer-subtract-5.c
> new file mode 100644
> index 00000000000..867eda0e61e
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/pointer-subtract-5.c
> @@ -0,0 +1,15 @@
> +/* PR middle-end/108543 */
> +/* { dg-do compile  } */
> +/* { dg-options "-fsanitize=address -fno-sanitize=kernel-address -fsanitize=pointer-subtract" } */
> +
> +struct S {
> +  long _M_p;
> +};
> +
> +typedef struct S S;
> +
> +__PTRDIFF_TYPE__
> +f (S __x, S __y)
> +{
> +  return &__x._M_p - &__y._M_p;
> +}
> diff --git a/gcc/testsuite/c-c++-common/asan/pointer-subtract-6.c b/gcc/testsuite/c-c++-common/asan/pointer-subtract-6.c
> new file mode 100644
> index 00000000000..785b90b3d98
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/pointer-subtract-6.c
> @@ -0,0 +1,15 @@
> +/* PR middle-end/108543 */
> +/* { dg-do compile  } */
> +/* { dg-options "-fsanitize=kernel-address -fno-sanitize=address -fsanitize=pointer-subtract" } */
> +
> +struct S {
> +  long _M_p;
> +};
> +
> +typedef struct S S;
> +
> +__PTRDIFF_TYPE__
> +f (S __x, S __y)
> +{
> +  return &__x._M_p - &__y._M_p;
> +}
> diff --git a/gcc/testsuite/c-c++-common/asan/pointer-subtract-7.c b/gcc/testsuite/c-c++-common/asan/pointer-subtract-7.c
> new file mode 100644
> index 00000000000..11b63401b8c
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/pointer-subtract-7.c
> @@ -0,0 +1,15 @@
> +/* PR middle-end/108543 */
> +/* { dg-do compile  } */
> +/* { dg-options "-fno-sanitize=kernel-address -fsanitize=address -fsanitize=pointer-subtract" } */
> +
> +struct S {
> +  long _M_p;
> +};
> +
> +typedef struct S S;
> +
> +__PTRDIFF_TYPE__
> +f (S __x, S __y)
> +{
> +  return &__x._M_p - &__y._M_p;
> +}
> diff --git a/gcc/testsuite/c-c++-common/asan/pointer-subtract-8.c b/gcc/testsuite/c-c++-common/asan/pointer-subtract-8.c
> new file mode 100644
> index 00000000000..ac2b9c3c1d9
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/asan/pointer-subtract-8.c
> @@ -0,0 +1,15 @@
> +/* PR middle-end/108543 */
> +/* { dg-do compile  } */
> +/* { dg-options "-fno-sanitize=address -fsanitize=kernel-address -fsanitize=pointer-subtract" } */
> +
> +struct S {
> +  long _M_p;
> +};
> +
> +typedef struct S S;
> +
> +__PTRDIFF_TYPE__
> +f (S __x, S __y)
> +{
> +  return &__x._M_p - &__y._M_p;
> +}
>
> base-commit: 9fb9da3d38513d320bfea72050f7a59688595e0b
> --
> 2.39.1
>