From patchwork Fri Mar 10 00:44:53 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 66193 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 DAE5C3857C44 for ; Fri, 10 Mar 2023 00:45:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DAE5C3857C44 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1678409131; bh=fq0wQsDhJQF6jzyjDewjn2148S7cCYeRIvvjzb+9a5Q=; h=Date:To:Cc:Subject:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=tFpFaBjiC6wun+UIpZd1J4B1B8UyALqvioX48qfGEZqtpbsg7TywMPApgCOH2E0se 03q8+skmlaRKOsSRGOKg9u6D10hWjbIGv9x5ggBpz1SJcDRdrI5EqVj9BBS3rzPtYv rzqMnh+oA9PA3ymeMut30s89Lb8jd+LTfE70beQ8= 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 [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 50FF03858C60 for ; Fri, 10 Mar 2023 00:45:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 50FF03858C60 Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-645-ALthA9sXPqWV2gaPog7hEQ-1; Thu, 09 Mar 2023 19:44:56 -0500 X-MC-Unique: ALthA9sXPqWV2gaPog7hEQ-1 Received: by mail-qk1-f197.google.com with SMTP id dm13-20020a05620a1d4d00b00742a22c4239so2257386qkb.1 for ; Thu, 09 Mar 2023 16:44:56 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678409096; h=user-agent:in-reply-to:content-disposition:mime-version:references :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=fq0wQsDhJQF6jzyjDewjn2148S7cCYeRIvvjzb+9a5Q=; b=QsL0qRIaJ6ifaT5lbVdfE7lV3fiKPWNIg4BGKu/ANB09uRTFn3feA0QYbafXQpA+eJ /iI/ErtCvp8pmRD7sLGzx9ysDbq9RAW+ogiR4zA8R7LjYpmWiAuPCNH907amtrpRf9qg HoUnNs0Uwa8YBK+YXfaKiKzU0hdVkZ1EGdOQ4RZnfbE8B/qvjzaIc972r7KjrKC79ZAB 3yQ5lKJp6bQ7I4RulB+w5siwpxioKUmV3yw4RJf4XpgOLJ96yILfAQrxz6jaxMMaTfyE 2oEWQUl7d98FPSd6Vy7DeZ5ahnjvxNCRkEKbqt4HpwpBfhk19N4YiAfkyt9FuKr2gyVH Ohdw== X-Gm-Message-State: AO0yUKVNChWMQwFKtvYdpLZXre5gXn6DGw4eyj5kRuFdf5vDHkvAzj2v 4n41CEfV5l5ea0iLkeuyYZvt6w0vSmXoEQUp+Jz0aS4x9bDj3ULU2OYdk2TFbOyJGr6DodMDXIs 3tdOAdwRMhkyyBVDrEg== X-Received: by 2002:ac8:5c83:0:b0:3a8:e35:258f with SMTP id r3-20020ac85c83000000b003a80e35258fmr42060584qta.31.1678409096147; Thu, 09 Mar 2023 16:44:56 -0800 (PST) X-Google-Smtp-Source: AK7set+eDTkVU+02QfOu9Il4FiVRvDZ2XncbiayQZ37A7yTWk8vhuDB0ZeoLwNFvemT03f+3ojHjqQ== X-Received: by 2002:ac8:5c83:0:b0:3a8:e35:258f with SMTP id r3-20020ac85c83000000b003a80e35258fmr42060566qta.31.1678409095810; Thu, 09 Mar 2023 16:44:55 -0800 (PST) Received: from redhat.com (2603-7000-9500-34a5-0000-0000-0000-1db4.res6.spectrum.com. [2603:7000:9500:34a5::1db4]) by smtp.gmail.com with ESMTPSA id d2-20020ac86142000000b003a81eef14efsm400324qtm.45.2023.03.09.16.44.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Mar 2023 16:44:55 -0800 (PST) Date: Thu, 9 Mar 2023 19:44:53 -0500 To: Jakub Jelinek Cc: Richard Biener , GCC Patches Subject: [PATCH v2] ubsan: missed -fsanitize=bounds for compound ops [PR108060] Message-ID: References: <20230308210930.128620-1-polacek@redhat.com> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/2.2.9 (2022-11-12) X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Disposition: inline X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Marek Polacek via Gcc-patches From: Marek Polacek Reply-To: Marek Polacek Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" On Thu, Mar 09, 2023 at 09:44:49AM +0100, Jakub Jelinek wrote: > On Thu, Mar 09, 2023 at 08:12:47AM +0000, Richard Biener wrote: > > I think this is a reasonable way to address the regression, so OK. > > It is true that both C and C++ (including c++14_down and c++17 and later > where the latter have different ordering rules) evaluate the lhs of > MODIFY_EXPR after rhs, so conceptually this patch makes sense. Thank you both for taking a look. > But I wonder why we do in ubsan_maybe_instrument_array_ref: > if (e != NULL_TREE) > { > tree t = copy_node (*expr_p); > TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1), > e, op1); > *expr_p = t; > } > rather than modification of the ARRAY_REF's operand in place. If we > did that, we wouldn't really care about the order, shared tree would > be instrumented once, with SAVE_EXPR in there making sure we don't > compute that multiple times. Is that because the 2 copies could > have side-effects and we do want to evaluate those multiple times? I'd assumed that that was the point of the copy_node. But now that I'm actually experimenting with it, I can't trigger any problems without the copy_node. So maybe we can use the following patch, which also adds a new test, bounds-21.c, to check that side-effects are evaluated correctly. I didn't bother writing a description for this patch yet because I sort of think we should apply both patches at the same time. Regtested on x86_64-pc-linux-gnu. -- >8 -- PR sanitizer/108060 PR sanitizer/109050 gcc/c-family/ChangeLog: * c-ubsan.cc (ubsan_maybe_instrument_array_ref): Don't copy_node. gcc/testsuite/ChangeLog: * c-c++-common/ubsan/bounds-17.c: New test. * c-c++-common/ubsan/bounds-18.c: New test. * c-c++-common/ubsan/bounds-19.c: New test. * c-c++-common/ubsan/bounds-20.c: New test. * c-c++-common/ubsan/bounds-21.c: New test. --- gcc/c-family/c-ubsan.cc | 8 ++------ gcc/testsuite/c-c++-common/ubsan/bounds-17.c | 17 +++++++++++++++++ gcc/testsuite/c-c++-common/ubsan/bounds-18.c | 17 +++++++++++++++++ gcc/testsuite/c-c++-common/ubsan/bounds-19.c | 20 ++++++++++++++++++++ gcc/testsuite/c-c++-common/ubsan/bounds-20.c | 16 ++++++++++++++++ gcc/testsuite/c-c++-common/ubsan/bounds-21.c | 18 ++++++++++++++++++ 6 files changed, 90 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-17.c create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-18.c create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-19.c create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-20.c create mode 100644 gcc/testsuite/c-c++-common/ubsan/bounds-21.c base-commit: f366fdfeec0af6cda716de913c32e48f9b1e3a0e diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc index 3e24198d7bb..8ce6421b61a 100644 --- a/gcc/c-family/c-ubsan.cc +++ b/gcc/c-family/c-ubsan.cc @@ -505,12 +505,8 @@ ubsan_maybe_instrument_array_ref (tree *expr_p, bool ignore_off_by_one) tree e = ubsan_instrument_bounds (EXPR_LOCATION (*expr_p), op0, &op1, ignore_off_by_one); if (e != NULL_TREE) - { - tree t = copy_node (*expr_p); - TREE_OPERAND (t, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1), - e, op1); - *expr_p = t; - } + TREE_OPERAND (*expr_p, 1) = build2 (COMPOUND_EXPR, TREE_TYPE (op1), + e, op1); } } diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-17.c b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c new file mode 100644 index 00000000000..b727e3235b8 --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-17.c @@ -0,0 +1,17 @@ +/* PR sanitizer/108060 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=bounds" } */ +/* { dg-skip-if "" { *-*-* } "-flto" } */ +/* { dg-shouldfail "ubsan" } */ + +int a[8]; +int c; + +int +main () +{ + int b = -32768; + a[b] |= c; +} + +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-18.c b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c new file mode 100644 index 00000000000..556abc0e1c0 --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-18.c @@ -0,0 +1,17 @@ +/* PR sanitizer/108060 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=bounds" } */ +/* { dg-skip-if "" { *-*-* } "-flto" } */ +/* { dg-shouldfail "ubsan" } */ + +int a[8]; +int c; + +int +main () +{ + int b = -32768; + a[b] = a[b] | c; +} + +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-19.c b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c new file mode 100644 index 00000000000..54217ae399f --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-19.c @@ -0,0 +1,20 @@ +/* PR sanitizer/108060 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=bounds" } */ +/* { dg-skip-if "" { *-*-* } "-flto" } */ +/* { dg-shouldfail "ubsan" } */ + +int a[8]; +int a2[18]; +int c; + +int +main () +{ + int b = 0; + a[0] = (a2[b], b = -32768, a[0] | c); + b = 0; + a[b] = (a[b], b = -32768, a[0] | c); +} + +/* { dg-output "index -32768 out of bounds for type 'int \\\[8\\\]'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-20.c b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c new file mode 100644 index 00000000000..a78c67129e0 --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-20.c @@ -0,0 +1,16 @@ +/* PR sanitizer/109050 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */ +/* { dg-shouldfail "ubsan" } */ + +long a; +int b; +int +main () +{ + int c[4] = {0, 1, 2, 3}; + a = 0; + c[a - 9806816] |= b; +} + +/* { dg-output "index -9806816 out of bounds for type 'int \\\[4\\\]'" } */ diff --git a/gcc/testsuite/c-c++-common/ubsan/bounds-21.c b/gcc/testsuite/c-c++-common/ubsan/bounds-21.c new file mode 100644 index 00000000000..b9d9308849f --- /dev/null +++ b/gcc/testsuite/c-c++-common/ubsan/bounds-21.c @@ -0,0 +1,18 @@ +/* PR sanitizer/109050 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=bounds -fno-sanitize-recover=all" } */ + +int i; +int foo (void) { return ++i; } + +int +main () +{ + char a[10] = { }; + a[foo ()] = a[foo()] | 'a'; + if (i != 2) + __builtin_abort (); + a[foo()] |= 'a'; + if (i != 3) + __builtin_abort (); +}