On Fri, 1 Oct 2021, Jakub Jelinek wrote:
> Hi!
>
> 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.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK. Maybe the change is worth mentioning in changes.html or maybe it's
just a bugfix...
Thanks,
Richard.
> 2021-10-01 Jakub Jelinek <ja...@redhat.com>
> Richard Biener <rguent...@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.
>
> --- gcc/doc/invoke.texi.jj 2021-09-29 10:07:41.841880681 +0200
> +++ gcc/doc/invoke.texi 2021-09-30 20:21:37.736147295 +0200
> @@ -15229,7 +15229,7 @@ ISO C90 and C99, etc.
>
> @item -fsanitize=integer-divide-by-zero
> @opindex fsanitize=integer-divide-by-zero
> -Detect integer division by zero as well as @code{INT_MIN / -1} division.
> +Detect integer division by zero.
>
> @item -fsanitize=unreachable
> @opindex fsanitize=unreachable
> @@ -15261,7 +15261,8 @@ returning a value. This option works in
> @opindex fsanitize=signed-integer-overflow
> This option enables signed integer overflow checking. We check that
> the result of @code{+}, @code{*}, and both unary and binary @code{-}
> -does not overflow in the signed arithmetics. Note, integer promotion
> +does not overflow in the signed arithmetics. This also detects
> +@code{INT_MIN / -1} signed division. Note, integer promotion
> rules must be taken into account. That is, the following is not an
> overflow:
> @smallexample
> --- gcc/c-family/c-ubsan.c.jj 2021-01-04 10:25:50.355103357 +0100
> +++ gcc/c-family/c-ubsan.c 2021-09-30 20:18:53.273433013 +0200
> @@ -39,8 +39,9 @@ along with GCC; see the file COPYING3.
> tree
> ubsan_instrument_division (location_t loc, tree op0, tree op1)
> {
> - tree t, tt;
> + tree t, tt, x = NULL_TREE;
> tree type = TREE_TYPE (op0);
> + enum sanitize_code flag = SANITIZE_DIVIDE;
>
> /* At this point both operands should have the same type,
> because they are already converted to RESULT_TYPE.
> @@ -58,24 +59,42 @@ ubsan_instrument_division (location_t lo
> op1, build_int_cst (type, 0));
> else if (TREE_CODE (type) == REAL_TYPE
> && sanitize_flags_p (SANITIZE_FLOAT_DIVIDE))
> - t = fold_build2 (EQ_EXPR, boolean_type_node,
> - op1, build_real (type, dconst0));
> + {
> + t = fold_build2 (EQ_EXPR, boolean_type_node,
> + op1, build_real (type, dconst0));
> + flag = SANITIZE_FLOAT_DIVIDE;
> + }
> else
> - return NULL_TREE;
> + t = NULL_TREE;
>
> /* We check INT_MIN / -1 only for signed types. */
> if (TREE_CODE (type) == INTEGER_TYPE
> - && sanitize_flags_p (SANITIZE_DIVIDE)
> + && sanitize_flags_p (SANITIZE_SI_OVERFLOW)
> && !TYPE_UNSIGNED (type))
> {
> - tree x;
> tt = fold_build2 (EQ_EXPR, boolean_type_node, unshare_expr (op1),
> build_int_cst (type, -1));
> x = fold_build2 (EQ_EXPR, boolean_type_node, op0,
> TYPE_MIN_VALUE (type));
> x = fold_build2 (TRUTH_AND_EXPR, boolean_type_node, x, tt);
> - t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
> + if (t == NULL_TREE || integer_zerop (t))
> + {
> + t = x;
> + x = NULL_TREE;
> + flag = SANITIZE_SI_OVERFLOW;
> + }
> + else if (flag_sanitize_undefined_trap_on_error
> + || (((flag_sanitize_recover & SANITIZE_DIVIDE) == 0)
> + == ((flag_sanitize_recover & SANITIZE_SI_OVERFLOW) == 0)))
> + {
> + t = fold_build2 (TRUTH_OR_EXPR, boolean_type_node, t, x);
> + x = NULL_TREE;
> + }
> + else if (integer_zerop (x))
> + x = NULL_TREE;
> }
> + else if (t == NULL_TREE)
> + return NULL_TREE;
>
> /* If the condition was folded to 0, no need to instrument
> this expression. */
> @@ -95,7 +114,7 @@ ubsan_instrument_division (location_t lo
> NULL_TREE);
> data = build_fold_addr_expr_loc (loc, data);
> enum built_in_function bcode
> - = (flag_sanitize_recover & SANITIZE_DIVIDE)
> + = (flag_sanitize_recover & flag)
> ? BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW
> : BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW_ABORT;
> tt = builtin_decl_explicit (bcode);
> @@ -103,8 +122,20 @@ ubsan_instrument_division (location_t lo
> op1 = unshare_expr (op1);
> tt = build_call_expr_loc (loc, tt, 3, data, ubsan_encode_value (op0),
> ubsan_encode_value (op1));
> + if (x)
> + {
> + bcode = (flag_sanitize_recover & SANITIZE_SI_OVERFLOW)
> + ? BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW
> + : BUILT_IN_UBSAN_HANDLE_DIVREM_OVERFLOW_ABORT;
> + tree xt = builtin_decl_explicit (bcode);
> + op0 = unshare_expr (op0);
> + op1 = unshare_expr (op1);
> + xt = build_call_expr_loc (loc, xt, 3, data, ubsan_encode_value (op0),
> + ubsan_encode_value (op1));
> + x = fold_build3 (COND_EXPR, void_type_node, x, xt, void_node);
> + }
> }
> - t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_node);
> + t = fold_build3 (COND_EXPR, void_type_node, t, tt, x ? x : void_node);
>
> return t;
> }
> --- gcc/c/c-typeck.c.jj 2021-09-29 10:07:41.789881404 +0200
> +++ gcc/c/c-typeck.c 2021-09-30 20:50:00.990465683 +0200
> @@ -12733,7 +12733,9 @@ build_binary_op (location_t location, en
> }
>
> if (sanitize_flags_p ((SANITIZE_SHIFT
> - | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
> + | SANITIZE_DIVIDE
> + | SANITIZE_FLOAT_DIVIDE
> + | SANITIZE_SI_OVERFLOW))
> && current_function_decl != NULL_TREE
> && (doing_div_or_mod || doing_shift)
> && !require_constant_value)
> @@ -12744,7 +12746,8 @@ build_binary_op (location_t location, en
> op0 = c_fully_fold (op0, false, NULL);
> op1 = c_fully_fold (op1, false, NULL);
> if (doing_div_or_mod && (sanitize_flags_p ((SANITIZE_DIVIDE
> - | SANITIZE_FLOAT_DIVIDE))))
> + | SANITIZE_FLOAT_DIVIDE
> + | SANITIZE_SI_OVERFLOW))))
> instrument_expr = ubsan_instrument_division (location, op0, op1);
> else if (doing_shift && sanitize_flags_p (SANITIZE_SHIFT))
> instrument_expr = ubsan_instrument_shift (location, code, op0, op1);
> --- gcc/cp/typeck.c.jj 2021-09-09 10:40:26.096175666 +0200
> +++ gcc/cp/typeck.c 2021-09-30 20:50:23.806147945 +0200
> @@ -6038,7 +6038,9 @@ cp_build_binary_op (const op_location_t
> }
>
> if (sanitize_flags_p ((SANITIZE_SHIFT
> - | SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
> + | SANITIZE_DIVIDE
> + | SANITIZE_FLOAT_DIVIDE
> + | SANITIZE_SI_OVERFLOW))
> && current_function_decl != NULL_TREE
> && !processing_template_decl
> && (doing_div_or_mod || doing_shift))
> @@ -6050,7 +6052,9 @@ cp_build_binary_op (const op_location_t
> op1 = fold_non_dependent_expr (op1, complain);
> tree instrument_expr1 = NULL_TREE;
> if (doing_div_or_mod
> - && sanitize_flags_p (SANITIZE_DIVIDE | SANITIZE_FLOAT_DIVIDE))
> + && sanitize_flags_p (SANITIZE_DIVIDE
> + | SANITIZE_FLOAT_DIVIDE
> + | SANITIZE_SI_OVERFLOW))
> {
> /* For diagnostics we want to use the promoted types without
> shorten_binary_op. So convert the arguments to the
> --- gcc/testsuite/c-c++-common/ubsan/div-by-zero-3.c.jj 2020-01-12
> 11:54:37.029404115 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-3.c 2021-09-30
> 20:51:00.522636616 +0200
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-fsanitize=integer-divide-by-zero -Wno-overflow" } */
> +/* { dg-options "-fsanitize=signed-integer-overflow -Wno-overflow" } */
>
> #include <limits.h>
>
> --- gcc/testsuite/c-c++-common/ubsan/div-by-zero-4.c.jj 2020-01-12
> 11:54:37.029404115 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-4.c 2021-09-30
> 20:24:17.051933114 +0200
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-fsanitize=integer-divide-by-zero -Wno-overflow" } */
> +/* { dg-options "-fsanitize=signed-integer-overflow
> -fsanitize-undefined-trap-on-error -Wno-overflow" } */
>
> #define INT_MIN (-__INT_MAX__ - 1)
>
> --- gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c.jj 2020-01-12
> 11:54:37.029404115 +0100
> +++ gcc/testsuite/c-c++-common/ubsan/div-by-zero-5.c 2021-09-30
> 20:24:55.559397933 +0200
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-fsanitize=integer-divide-by-zero" } */
> +/* { dg-options "-fsanitize=signed-integer-overflow" } */
>
> void
> foo (void)
> --- gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-2.c.jj 2021-10-01
> 12:28:23.485086791 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/float-div-by-zero-2.c 2021-10-01
> 12:29:35.875062499 +0200
> @@ -0,0 +1,18 @@
> +/* { dg-do run } */
> +/* { dg-shouldfail "ubsan" } */
> +/* { dg-options "-fsanitize=float-divide-by-zero
> -fno-sanitize-recover=float-divide-by-zero
> -fsanitize-recover=integer-divide-by-zero" } */
> +
> +int
> +main (void)
> +{
> + volatile float a = 1.3f;
> + volatile double b = 0.0;
> + volatile int c = 4;
> + volatile float res;
> +
> + res = a / b;
> +
> + return 0;
> +}
> +
> +/* { dg-output "division by zero" } */
> --- gcc/testsuite/c-c++-common/ubsan/overflow-div-1.c.jj 2021-10-01
> 12:30:48.269038146 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/overflow-div-1.c 2021-10-01
> 12:31:49.532171295 +0200
> @@ -0,0 +1,17 @@
> +/* { dg-do run } */
> +/* { dg-options "-fsanitize=integer-divide-by-zero
> -fno-sanitize-recover=undefined,float-divide-by-zero -Wno-overflow" } */
> +
> +#include <limits.h>
> +
> +int
> +main (void)
> +{
> + volatile int min = INT_MIN;
> + volatile int zero = 0;
> +
> + INT_MIN / -1;
> + min / -1;
> + min / (10 * zero - (2 - 1));
> +
> + return 0;
> +}
> --- gcc/testsuite/c-c++-common/ubsan/overflow-div-2.c.jj 2021-10-01
> 12:34:28.814917433 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/overflow-div-2.c 2021-10-01
> 12:50:24.398395074 +0200
> @@ -0,0 +1,41 @@
> +/* { dg-do run { target *-*-linux* *-*-gnu* } } */
> +/* { dg-shouldfail "ubsan" } */
> +/* { dg-options "-fsanitize=undefined
> -fno-sanitize-recover=integer-divide-by-zero" } */
> +
> +#include <limits.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +
> +int cnt;
> +
> +__attribute__((noipa)) int
> +foo (int x, int y)
> +{
> + return x / y;
> +}
> +
> +void
> +handler (int i)
> +{
> + if (cnt++ != 0)
> + exit (0);
> + volatile int b = foo (5, 0);
> + exit (0);
> +}
> +
> +int
> +main (void)
> +{
> + struct sigaction s;
> + sigemptyset (&s.sa_mask);
> + s.sa_handler = handler;
> + s.sa_flags = 0;
> + sigaction (SIGFPE, &s, NULL);
> + volatile int a = foo (INT_MIN, -1);
> + cnt++;
> + volatile int b = foo (5, 0);
> + return 0;
> +}
> +
> +/* { dg-output "division of -2147483648 by -1 cannot be represented in type
> 'int'\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*division by zero\[^\n\r]*" } */
> --- gcc/testsuite/c-c++-common/ubsan/overflow-div-3.c.jj 2021-10-01
> 12:39:28.230680432 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/overflow-div-3.c 2021-10-01
> 12:50:58.652910346 +0200
> @@ -0,0 +1,41 @@
> +/* { dg-do run { target *-*-linux* *-*-gnu* } } */
> +/* { dg-shouldfail "ubsan" } */
> +/* { dg-options "-fsanitize=undefined
> -fno-sanitize-recover=signed-integer-overflow" } */
> +
> +#include <limits.h>
> +#include <signal.h>
> +#include <stdlib.h>
> +
> +int cnt;
> +
> +__attribute__((noipa)) int
> +foo (int x, int y)
> +{
> + return x / y;
> +}
> +
> +void
> +handler (int i)
> +{
> + if (cnt++ != 0)
> + exit (0);
> + volatile int b = foo (INT_MIN, -1);
> + exit (0);
> +}
> +
> +int
> +main (void)
> +{
> + struct sigaction s;
> + sigemptyset (&s.sa_mask);
> + s.sa_handler = handler;
> + s.sa_flags = 0;
> + sigaction (SIGFPE, &s, NULL);
> + volatile int a = foo (42, 0);
> + cnt++;
> + volatile int b = foo (INT_MIN, -1);
> + return 0;
> +}
> +
> +/* { dg-output "division by zero\[^\n\r]*(\n|\r\n|\r)" } */
> +/* { dg-output "\[^\n\r]*division of -2147483648 by -1 cannot be represented
> in type 'int'" } */
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
