From patchwork Fri Oct 1 11:24:28 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jakub Jelinek X-Patchwork-Id: 45675 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B0E6A3857C58 for ; Fri, 1 Oct 2021 11:25:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B0E6A3857C58 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1633087510; bh=gq4AtNJbNbAxJCxfUxLL49jSsIxEvFZ9aUYLmeP3X2c=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=H390BEIyHsuVPkyOc+0jo2Vt4hW/sjTer06006zm93T44rfpSQ0VSgByzzqCBj9A3 u1zNvZKNlSyaY+USlhNhKqHUIlV2g289s7RKjpbcuSkxzZPsWORLovbIeWPSP8UBL3 FZxsZ3JJeqhLJ/bkSGblRSDHHxuXVQkUgmOjrAIA= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id EA84C3858C2C for ; Fri, 1 Oct 2021 11:24:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EA84C3858C2C Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-299-RzlKfne8PeGSklczH-szuA-1; Fri, 01 Oct 2021 07:24:39 -0400 X-MC-Unique: RzlKfne8PeGSklczH-szuA-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 916D91084683; Fri, 1 Oct 2021 11:24:38 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.109]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DFD801002EF0; Fri, 1 Oct 2021 11:24:32 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 191BOTAr3322424 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Fri, 1 Oct 2021 13:24:30 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 191BOS5D3322423; Fri, 1 Oct 2021 13:24:28 +0200 Date: Fri, 1 Oct 2021 13:24:28 +0200 To: Richard Biener , Marek Polacek Subject: [PATCH] ubsan: Move INT_MIN / -1 instrumentation from -fsanitize=integer-divide-by-zero to -fsanitize=signed-integer-overflow [PR102515] Message-ID: <20211001112428.GL304296@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Jakub Jelinek via Gcc-patches From: Jakub Jelinek Reply-To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" 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? 2021-10-01 Jakub Jelinek Richard Biener 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. Jakub --- 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 --- 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 + +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 +#include +#include + +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 +#include +#include + +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'" } */