From patchwork Wed Nov 1 09:39:16 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Uecker X-Patchwork-Id: 160493 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:abcd:0:b0:403:3b70:6f57 with SMTP id f13csp290145vqx; Wed, 1 Nov 2023 02:40:08 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGIwpS/VFbx41W2liUlnnXF050x6uMfXuR40m1sdrNL00tjP1dUd2VHs/y0OM/KykSUgERb X-Received: by 2002:ac8:5991:0:b0:41c:c205:f718 with SMTP id e17-20020ac85991000000b0041cc205f718mr21131660qte.45.1698831607852; Wed, 01 Nov 2023 02:40:07 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1698831607; cv=pass; d=google.com; s=arc-20160816; b=nGUSUh8GWYsU8aH2E+w5O9hX3HMTML9AwEVqYVoNC8mF2a+x/PFaRQEeawqlZWOPSC kGRquFJ0Pf0D+2XcTsJkEpFGILFOq4hN9Ukzp1uZaQd2YMy6qP7tdJhsdWy47wcPpp6K kacsf6xPLWo7hjUh/dyre8AFc3iDuW/7givSdFtBfsGkcyhrmcJwJnTBFPsAnU5Nxyrd Ch4doRhBWNSTShNmV/HDQTqj1sOyti0Tym+O7Em1t9d657l5vJSNUc7RBz1dl38L5zvJ ftWunnTR9mf970NUqlcRoLkgvNMEkU3sdRo5GFO1PkRzGxOMK3x3L9rpgcLg4ziTzVcc 0OyA== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=errors-to:list-subscribe:list-help:list-post:list-archive :list-unsubscribe:list-id:precedence:mime-version:user-agent :content-transfer-encoding:date:cc:to:from:subject:message-id :dkim-signature:dkim-filter:arc-filter:dmarc-filter:delivered-to; bh=ZIGkjARmOjCBZnr/9jnPknPFrS7oofV1SWsckuMELyw=; fh=o0AlagSOGyCnSWRlD1+Hg7Grd52zvAZHW8byFEcZF/g=; b=B1//OA67aY6t4EAmpMAeBYxr/5cLaDZIRuj1h3hiuIAsicA/D3hnsxc9ecLQpkYgnb 6gGVlivg0jBHLfzzTSIeT5sbG9B0k1db+4vBQeIhUceQs6cHpuDbflbbEIuAKiblmmgt czibIA8mXBd46bAkH7CTBjCEXHN+AQEOv7AkwVe/qYFUalAFYzh5YwUrbkqgmRN++wMy M2QKFeZXXvTdAQ0cmDrmdNqg4gmZYK/kjLLlj2+34t7HoNwxPv1u5C9DDuqug4oVAjY8 STecMF6McWD/9ZILOE715n6ZxqGM1HCKMMwmBdSHe+vWO+TKOSxTZFegLzA/k+n4vOUG 40xw== ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@tugraz.at header.s=mailrelay header.b=HkDnMg9r; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=tugraz.at Received: from server2.sourceware.org (server2.sourceware.org. [8.43.85.97]) by mx.google.com with ESMTPS id e1-20020ac85981000000b00418176fd045si2545497qte.148.2023.11.01.02.40.07 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Nov 2023 02:40:07 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) client-ip=8.43.85.97; Authentication-Results: mx.google.com; dkim=pass header.i=@tugraz.at header.s=mailrelay header.b=HkDnMg9r; arc=pass (i=1); spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 8.43.85.97 as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=QUARANTINE sp=NONE dis=NONE) header.from=tugraz.at Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9C8E4385840E for ; Wed, 1 Nov 2023 09:40:07 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mailrelay.tugraz.at (mailrelay.tugraz.at [129.27.2.202]) by sourceware.org (Postfix) with ESMTPS id AE66A3858D35 for ; Wed, 1 Nov 2023 09:39:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AE66A3858D35 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=tugraz.at Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=tugraz.at ARC-Filter: OpenARC Filter v1.0.0 sourceware.org AE66A3858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=129.27.2.202 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698831576; cv=none; b=k5fRPkZU5Ly80zTO5jGohVH3VW9jXkXnZZ/GAeZlNux95JkoIL6ldGOLd+ohTWsEDBn3AL4SCBcAQtao30Ov4PORVwKq5svS65UF4aatYKXy2M9ZF9lgfAby/QRhGN7LtwbpEA3f0fXgDqLOlBq8OpjTQMOn03pswewW93IKRBU= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698831576; c=relaxed/simple; bh=oRu5cjd2/7fgVfw+0q5LAZOKwjfztx8v8W5iVhvKDi0=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=VRA3CcYXFy4bo+//7HEKPn1rH5hcw0ohqQPiroC9d8dlchyokptPA1Ks9dC1f9+NI4RO10YTDBzg4Nqjw2VFL6iIPJpotPsMvLlqTFm4TKsG14fuKKBNRjO/KB4cTHSgvAKbeb5LLgUMUzX8delZ3SqK7hVHf29Galq1ILafd70= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from vra-169-216.tugraz.at (vra-169-216.tugraz.at [129.27.169.216]) by mailrelay.tugraz.at (Postfix) with ESMTPSA id 4SL25c3S59z1HNLS; Wed, 1 Nov 2023 10:39:16 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 mailrelay.tugraz.at 4SL25c3S59z1HNLS DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tugraz.at; s=mailrelay; t=1698831557; bh=ZIGkjARmOjCBZnr/9jnPknPFrS7oofV1SWsckuMELyw=; h=Subject:From:To:Cc:Date:From; b=HkDnMg9rAN7+Avxse6Uvmrb8Gg27GKqN70w1pShQN+f3YqoEBG4xwhNxRx7CDFkNg B1/2JxQ8tPYA2ygXlKeK0akyftkPD9DzTVoBFyiyCXv016EVJVqZiG+h9rynAkBk+z 5/no3EAU5cYtmkYmBdYOsFIpVHq+YOUdSGQvUhn8= Message-ID: <01ed76d1286383f7a7e0a378be6c82bec12a2fd8.camel@tugraz.at> Subject: RFC [PATCH] c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608] From: Martin Uecker To: gcc-patches@gcc.gnu.org Cc: Joseph Myers , Marek Polacek , Jakub Jelinek Date: Wed, 01 Nov 2023 10:39:16 +0100 User-Agent: Evolution 3.46.4-2 MIME-Version: 1.0 X-TUG-Backscatter-control: G/VXY7/6zeyuAY/PU2/0qw X-Spam-Scanner: SpamAssassin 3.003001 X-Spam-Score-relay: -1.9 X-Scanned-By: MIMEDefang 2.74 on 129.27.10.116 X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1781354051992576786 X-GMAIL-MSGID: 1781354051992576786 Here is a patch that adds the missing cases for vla size instrumentation. This now includes all cases where a type with size < 0 is created, which is already UB and not just cases where a VLA is allocated. But a VLA can be allocated based on an typedef, which is also now indirectly protected in this way. I moved the instrumentation from the size itself as its own term into  the expression that evaluate size expressions for side effects. This avoids confusing other warning code that looks at the size expressions (-Wvla-parameter). There is one open question though: How to to treat n == 0?  Here I preliminary changed this to n > 0 (also for the existing case), because when also detecting n == 0 this tools especially when instrumenting all the types becomes basically useless because of  the very common (and unproblematic) use of n == 0.   But strictly speaking n == 0 is also UB and as pointed out in  PR98609 the error message is then not entirely accurate because it says non-positive and not negative. I do not think it is confusing though because it is still always correct. One could consider splitting it up into vla-bound / vla-bound-strict, but changing the error message would require further upstream changes and dealing with this far exceeds the time I can afford contributing to this this. Another complication is that we ran out of bits for sanitizer flags in unsigned int, so this would also require more changes. Any advice? I think it would be important to have complete UBSan coverage for all size and bounds issues related to VM types and it would be nice to get this in GCC 14. (I find this extremely useful in my projects). Martin c: Add missing cases where vla sizes are not instrumented by UBSan [PR98608] Add vla-bound instrumentation for all VLAs including VLAs in parameters and fields, but allow zero-sized errors. Bootstrapped and regression tested on x86. PR c/98608 gcc/c: * c-decl.cc (grokdeclarator): Instrument all VLAs. gcc/c-family: * c-ubsan.cc (ubsan_instrument_vla): Do not include zero length. gcc/testsuite: * gcc.dg/ubsan/pr89608.c: New test. * gcc.dg/ubsan/vla-1.c: New test. * gcc.dg/ubsan/vla-2.c: New test. * gcc.dg/ubsan/vla-3.c: New test. * c-c++-common/ubsan/vla-1.c: Adapt. diff --git a/gcc/c-family/c-ubsan.cc b/gcc/c-family/c-ubsan.cc index b2c58c65d97..8983ede0166 100644 --- a/gcc/c-family/c-ubsan.cc +++ b/gcc/c-family/c-ubsan.cc @@ -313,7 +313,7 @@ ubsan_instrument_vla (location_t loc, tree size) tree type = TREE_TYPE (size); tree t, tt; - t = fold_build2 (LE_EXPR, boolean_type_node, size, build_int_cst (type, 0)); + t = fold_build2 (LT_EXPR, boolean_type_node, size, build_int_cst (type, 0)); if (flag_sanitize_trap & SANITIZE_VLA) tt = build_call_expr_loc (loc, builtin_decl_explicit (BUILT_IN_TRAP), 0); else diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index 7a145bed281..752a65d6729 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -7201,16 +7201,20 @@ grokdeclarator (const struct c_declarator *declarator, with known value. */ this_size_varies = size_varies = true; warn_variable_length_array (name, size); - if (sanitize_flags_p (SANITIZE_VLA) - && current_function_decl != NULL_TREE - && decl_context == NORMAL) + if (sanitize_flags_p (SANITIZE_VLA)) { /* Evaluate the array size only once. */ size = save_expr (size); size = c_fully_fold (size, false, NULL); - size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size), - ubsan_instrument_vla (loc, size), - size); + tree instr = ubsan_instrument_vla (loc, size); + /* We have to build this in the right order, so + instrumentation is done before the size can + be used in other parameters. */ + if (*expr) + *expr = build2 (COMPOUND_EXPR, TREE_TYPE (instr), + *expr, instr); + else + *expr = instr; } } diff --git a/gcc/testsuite/c-c++-common/ubsan/vla-1.c b/gcc/testsuite/c-c++-common/ubsan/vla-1.c index c97465edae1..cc441ffc80b 100644 --- a/gcc/testsuite/c-c++-common/ubsan/vla-1.c +++ b/gcc/testsuite/c-c++-common/ubsan/vla-1.c @@ -110,9 +110,7 @@ main (void) /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -5\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -3\[^\n\r]*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ -/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ /* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -6\[^\n\r]*(\n|\r\n|\r)" } */ diff --git a/gcc/testsuite/gcc.dg/ubsan/pr98608.c b/gcc/testsuite/gcc.dg/ubsan/pr98608.c new file mode 100644 index 00000000000..9f8f40c9643 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/pr98608.c @@ -0,0 +1,16 @@ +/* PR 98608 */ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound" } */ + +void f(int n, double (*x)[n]) +{ + typeof(*x) y; +} + +int main() +{ + f(-1, 0); +} + +/* { dg-output "variable length array bound evaluates to non-positive value -1" } */ + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-1.c b/gcc/testsuite/gcc.dg/ubsan/vla-1.c new file mode 100644 index 00000000000..6544698caf9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-1.c @@ -0,0 +1,27 @@ +/* { dg-do run } */ +/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */ + +void f1(int n, char buf[n]) { } +/* { dg-output "variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f2(int n, float m[n][2]) { } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f3(int n, int m, float x[n][m]) { } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f4(int n) { typedef char buf[n]; } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f5(int n) { struct { char buf[n]; } x; } +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1\[^\n\r]*(\n|\r\n|\r)" } */ +void f6(int n, struct { char buf[n]; } *p) { } /* { dg-warning "anonymous" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1" } */ + +int main() +{ + f1(-1, 0); + f2(-1, 0); + f3(-1, 1, 0); + f4(-1); + f5(-1); + f6(-1, 0); +} + + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-2.c b/gcc/testsuite/gcc.dg/ubsan/vla-2.c new file mode 100644 index 00000000000..5937f860e89 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-2.c @@ -0,0 +1,21 @@ +/* { dg-do run } */ +/* { dg-options "-Wstringop-overflow=0 -fsanitize=vla-bound -fsanitize-recover=vla-bound" } */ + +void f1(int n, char buf[n]) { } +void f2(int n, float m[n][2]) { } +void f3(int n, int m, float x[n][m]) { } +void f4(int n) { typedef char buf[n]; } +void f5(int n) { struct { char buf[n]; } x; } +void f6(int n, struct { char buf[n]; } *p) { } /* { dg-warning "anonymous" } */ + +int main() +{ + f1(0, 0); + f2(0, 0); + f3(0, 1, 0); + f4(0); + f5(0); + f6(0, 0); +} + + diff --git a/gcc/testsuite/gcc.dg/ubsan/vla-3.c b/gcc/testsuite/gcc.dg/ubsan/vla-3.c new file mode 100644 index 00000000000..9f4b0555328 --- /dev/null +++ b/gcc/testsuite/gcc.dg/ubsan/vla-3.c @@ -0,0 +1,14 @@ +/* { dg-do run } + * { dg-options "-fsanitize=vla-bound" } */ + +void foo(int n, char (*buf)[n], char p[n = 1]) +{ +} + +int main() +{ + foo(-1, 0, 0); +} + +/* { dg-output "variable length array bound evaluates to non-positive value -1" } */ +