[Bug sanitizer/102515] New: UBSAN misses signed division instrumentation

[Bug sanitizer/102515] New: UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

            Bug ID: 102515
           Summary: UBSAN misses signed division instrumentation
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: rguenth at gcc dot gnu.org
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

To replace -ftrapv with -fsanitize=signed-integer-overflow
-fsanitize-undefined-trap-on-error we need to (well, might want to ...) extend
UBSAN to cover the degenerate case of signed division of the most negative
value by -1.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
ubsan already handles that in c-family/c-ubsan.c (ubsan_instrument_division).
But we don't have an ifn for that.  And it is not covered by
-fsanitize=signed-integer-overflow but -fsanitize=integer-divide-by-zero,
although both are under -fsanitize=undefined.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
I see - I wonder whether we should do || sanitize_flags_p
(SANITIZE_SI_OVERFLOW) for this specific case.

For -ftrapv I also really was looking at instrumentation from the frontend
rather than from pass_ubsan since that's quite late IMHO and so it doesn't
help in removing hysterical avoidance of late "trap" introducing by the
middle-end (though we do need to care anyway to not introduce undefined
overflow).

It also looks like handling ftrapv as Alias(fsanitize=...) doesn't work
since we need to alias to two options.  Meh ;)  Maybe add fsanitize=trapv
as a cover option for both.

That said, I'm considering to pull the trigger on -ftrapv replacement
by ubsan for GCC 12.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Btw, clang documents:

-fsanitize=signed-integer-overflow: Signed integer overflow, where the result
of a signed integer computation cannot be represented in its type. This
includes all the checks covered by -ftrapv, as well as checks for signed
division overflow (INT_MIN/-1), 

and restricts divide to divide by zero.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2021-10-01

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
I have a patch.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 51529
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51529&action=edit
gcc12-pr102515.patch

I have one too, even bootstrapped/regtested overnight, just didn't get to
writing new testcases that would cover the cases that need testing (i.e. that
-f{,no-}sanitize-recover=float-divide-by-zero rather than
-f{,no-}sanitize-recover=integer-divide-by-zero decides on *_abort for float
division and the behavior for -fsanitize=undefined
-fno-sanitize-recover=integer-divide-by-zero or -fsanitize=undefined
-fno-sanitize-recover=signed-integer-overflow (i.e. when integer-divide-by-zero
and signed-integer-overflow differ in the recover method).

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> Created attachment 51529 [details]
> gcc12-pr102515.patch
> 
> I have one too, even bootstrapped/regtested overnight, just didn't get to
> writing new testcases that would cover the cases that need testing (i.e. that
> -f{,no-}sanitize-recover=float-divide-by-zero rather than
> -f{,no-}sanitize-recover=integer-divide-by-zero decides on *_abort for float
> division and the behavior for -fsanitize=undefined
> -fno-sanitize-recover=integer-divide-by-zero or -fsanitize=undefined
> -fno-sanitize-recover=signed-integer-overflow (i.e. when
> integer-divide-by-zero and signed-integer-overflow differ in the recover
> method).

OK, so yours looks mostly identical to mine, even handling one minor case
better and erring on the side to not preserve divide overflow with
divide-by-zero (which I think is reasonable).

I've yet only added c-c++-common/ubsan/overflow-div-1.c as

/* { dg-do run } */
/* { dg-options "-fsanitize=signed-integer-overflow" } */
/* { dg-shouldfail "ubsan" } */

int __attribute__((noipa))
foo (int a, int b)
{
  return a / b;
}

int
main ()
{
  return foo (-__INT_MAX__ - 1, -1);
}

/* { dg-output "cannot be represented" } */

and did not add testcases for the fixed float-divide recovery (which I also
noticed and fixed).

Note I simply track whether _all_ of the used instrumentations have
recovery enabled and if not then go the abort path.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Created attachment 51530
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51530&action=edit
patch

For reference, this is my patch.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:9c1a633d96926357155d4702b66f8a0ec856a81f

commit r12-4042-g9c1a633d96926357155d4702b66f8a0ec856a81f
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Oct 1 14:27:32 2021 +0200

    ubsan: Move INT_MIN / -1 instrumentation from
-fsanitize=integer-divide-by-zero to -fsanitize=signed-integer-overflow
[PR102515]

    As noted by Richi, in clang INT_MIN / -1 is instrumented under
    -fsanitize=signed-integer-overflow rather than
    -fsanitize=integer-divide-by-zero as we did and doing it in the former
    makes more sense, as it is overflow during division rather than division
    by zero.
    I've verified on godbolt that clang behaved that way since 3.2-ish times or
    so when sanitizers were added.
    Furthermore, we've been using
    -f{,no-}sanitize-recover=integer-divide-by-zero to decide on the float
    -fsanitize=float-divide-by-zero instrumentation _abort suffix.
    The case where INT_MIN / -1 is instrumented by one sanitizer and
    x / 0 by another one when both are enabled is slightly harder if
    the
-f{,no-}sanitize-recover={integer-divide-by-zero,signed-integer-overflow}
    flags differ, then we need to emit both __ubsan_handle_divrem_overflow
    and __ubsan_handle_divrem_overflow_abort calls guarded by their respective
    checks rather than one guarded by check1 || check2.

    2021-10-01  Jakub Jelinek  <jakub@redhat.com>
                Richard Biener  <rguenther@suse.de>

            PR sanitizer/102515
    gcc/
            * doc/invoke.texi (-fsanitize=integer-divide-by-zero): Remove
            INT_MIN / -1 division detection from here ...
            (-fsanitize=signed-integer-overflow): ... and add it here.
    gcc/c-family/
            * c-ubsan.c (ubsan_instrument_division): Check the right
            flag_sanitize_recover bit, depending on which sanitization
            is done.  Sanitize INT_MIN / -1 under SANITIZE_SI_OVERFLOW
            rather than SANITIZE_DIVIDE.  If both SANITIZE_SI_OVERFLOW
            and SANITIZE_DIVIDE is enabled, neither check is known
            to be false and flag_sanitize_recover bits for those two
            aren't the same, emit both __ubsan_handle_divrem_overflow
            and __ubsan_handle_divrem_overflow_abort calls.
    gcc/c/
            * c-typeck.c (build_binary_op): Call ubsan_instrument_division
            for division even for SANITIZE_SI_OVERFLOW.
    gcc/cp/
            * typeck.c (cp_build_binary_op): Call ubsan_instrument_division
            for division even for SANITIZE_SI_OVERFLOW.
    gcc/testsuite/
            * c-c++-common/ubsan/div-by-zero-3.c: Use
            -fsanitize=signed-integer-overflow instead of
            -fsanitize=integer-divide-by-zero.
            * c-c++-common/ubsan/div-by-zero-5.c: Likewise.
            * c-c++-common/ubsan/div-by-zero-4.c: Likewise.  Add
            -fsanitize-undefined-trap-on-error.
            * c-c++-common/ubsan/float-div-by-zero-2.c: New test.
            * c-c++-common/ubsan/overflow-div-1.c: New test.
            * c-c++-common/ubsan/overflow-div-2.c: New test.
            * c-c++-common/ubsan/overflow-div-3.c: New test.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED
   Target Milestone|---                         |12.0

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed now.

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:dcf70cb0d85d04ff680a361e32d79c326ae8b3fc

commit r11-9074-gdcf70cb0d85d04ff680a361e32d79c326ae8b3fc
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Oct 1 14:27:32 2021 +0200

    ubsan: Use -fno{,-}sanitize=float-divide-by-zero for float division by zero
recovery [PR102515]

    We've been using
    -f{,no-}sanitize-recover=integer-divide-by-zero to decide on the float
    -fsanitize=float-divide-by-zero instrumentation _abort suffix.
    This patch fixes it to use -f{,no-}sanitize-recover=float-divide-by-zero
    for it instead.

    2021-10-01  Jakub Jelinek  <jakub@redhat.com>
                Richard Biener  <rguenther@suse.de>

            PR sanitizer/102515
    gcc/c-family/
            * c-ubsan.c (ubsan_instrument_division): Check the right
            flag_sanitize_recover bit, depending on which sanitization
            is done.
    gcc/testsuite/
            * c-c++-common/ubsan/float-div-by-zero-2.c: New test.

    (cherry picked from commit 9c1a633d96926357155d4702b66f8a0ec856a81f)

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:9f4f13c1d42c0eac9ccfe3f5a925389def881033

commit r10-10646-g9f4f13c1d42c0eac9ccfe3f5a925389def881033
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Oct 1 14:27:32 2021 +0200

    ubsan: Use -fno{,-}sanitize=float-divide-by-zero for float division by zero
recovery [PR102515]

    We've been using
    -f{,no-}sanitize-recover=integer-divide-by-zero to decide on the float
    -fsanitize=float-divide-by-zero instrumentation _abort suffix.
    This patch fixes it to use -f{,no-}sanitize-recover=float-divide-by-zero
    for it instead.

    2021-10-01  Jakub Jelinek  <jakub@redhat.com>
                Richard Biener  <rguenther@suse.de>

            PR sanitizer/102515
    gcc/c-family/
            * c-ubsan.c (ubsan_instrument_division): Check the right
            flag_sanitize_recover bit, depending on which sanitization
            is done.
    gcc/testsuite/
            * c-c++-common/ubsan/float-div-by-zero-2.c: New test.

    (cherry picked from commit 9c1a633d96926357155d4702b66f8a0ec856a81f)

[Bug sanitizer/102515] UBSAN misses signed division instrumentation

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102515

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:f806bea0a6c1c5b7d517c7aee053c21b4d2155c6

commit r9-10103-gf806bea0a6c1c5b7d517c7aee053c21b4d2155c6
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Fri Oct 1 14:27:32 2021 +0200

    ubsan: Use -fno{,-}sanitize=float-divide-by-zero for float division by zero
recovery [PR102515]

    We've been using
    -f{,no-}sanitize-recover=integer-divide-by-zero to decide on the float
    -fsanitize=float-divide-by-zero instrumentation _abort suffix.
    This patch fixes it to use -f{,no-}sanitize-recover=float-divide-by-zero
    for it instead.

    2021-10-01  Jakub Jelinek  <jakub@redhat.com>
                Richard Biener  <rguenther@suse.de>

            PR sanitizer/102515
    gcc/c-family/
            * c-ubsan.c (ubsan_instrument_division): Check the right
            flag_sanitize_recover bit, depending on which sanitization
            is done.
    gcc/testsuite/
            * c-c++-common/ubsan/float-div-by-zero-2.c: New test.

    (cherry picked from commit 9c1a633d96926357155d4702b66f8a0ec856a81f)