From patchwork Thu Sep 12 12:26:55 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 274542 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 4D7932C011C for ; Thu, 12 Sep 2013 22:27:22 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; q=dns; s=default; b=Q+bhNowj8GXmfPWnmJjnkgCKIjHJsP1r+Vpl9jaGqs570oUXqk qn4hbEG0hdNqt0d9Dpi4mKANxny/f485PC5rtkcIx0am3MuJUSTR4ZsP7SjLQ8CY EzaGU3HRaSbT+SZD4hbI5I8Y41zgBs/22CbDyDfvCTt2ihho36P8v8jKM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; s= default; bh=TMZY5rM4ipcmhapeWe75pizAzN4=; b=G9eQfEjWemg9/cmoX7m0 1Sftuzt7RrMVYZTifn2/zlxrf8Cm3pNDmQK/hlSrT2sVYsRy0t2b/PlCtyJnz3QU q7LjhhelxLLrw0ixDvWrLcCPczuFGG2j0905fiyghl4K//0ZCCkYm9jZGnWWnplY 5py6nLzrzXmSyekfgvPzU/E= Received: (qmail 18646 invoked by alias); 12 Sep 2013 12:27:15 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 18636 invoked by uid 89); 12 Sep 2013 12:27:15 -0000 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 12 Sep 2013 12:27:15 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.4 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r8CCRAjf005814 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 12 Sep 2013 08:27:10 -0400 Received: from redhat.com (ovpn-116-91.ams2.redhat.com [10.36.116.91]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r8CCQuCD026640 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Thu, 12 Sep 2013 08:27:03 -0400 Date: Thu, 12 Sep 2013 14:26:55 +0200 From: Marek Polacek To: GCC Patches Cc: Jakub Jelinek , Jason Merrill , "Joseph S. Myers" Subject: [PATCH][ubsan] Add VLA bound instrumentation Message-ID: <20130912122655.GN23899@redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) This patch adds the instrumentation of VLA bounds. Basically, it just checks that the size of a VLA is positive. I.e., We also issue an error if the size of the VLA is 0. It catches e.g. int i = 1; int a[i][i - 2]; It is pretty straightforward, but I had issues in the C++ FE, mainly choosing the right spot where to instrument... Hopefully I picked up the right one. Also note that in C++1y we throw an exception when the size of a VLA is negative; hence no need to perform the instrumentation if -std=c++1y is in effect. Regtested/ran bootstrap-ubsan on x86_64-linux, also make check -C gcc RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} ubsan.exp' passes. Ok for trunk? 2013-09-12 Marek Polacek * opts.c (common_handle_option): Handle vla-bound. * sanitizer.def (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE): Define. * ubsan.c (ubsan_type_descriptor): Handle IDENTIFIER_NODEs. * flag-types.h (enum sanitize_code): Add SANITIZE_VLA. * asan.c (initialize_sanitizer_builtins): Build BT_FN_VOID_PTR_PTR. c-family/ * c-ubsan.c: Don't include hash-table.h. (ubsan_instrument_vla): New function. * c-ubsan.h: Declare it. cp/ * decl.c (create_array_type_for_decl): Add VLA instrumentation. c/ * c-decl.c (grokdeclarator): Add VLA instrumentation. testsuite/ * g++.dg/ubsan/cxx1y-vla.C: New test. * c-c++-common/ubsan/vla-3.c: New test. * c-c++-common/ubsan/vla-2.c: New test. * c-c++-common/ubsan/vla-4.c: New test. * c-c++-common/ubsan/vla-1.c: New test. Marek --- gcc/opts.c.mp 2013-09-12 13:30:53.299113222 +0200 +++ gcc/opts.c 2013-09-12 13:31:31.496263290 +0200 @@ -1426,6 +1426,7 @@ common_handle_option (struct gcc_options { "undefined", SANITIZE_UNDEFINED, sizeof "undefined" - 1 }, { "unreachable", SANITIZE_UNREACHABLE, sizeof "unreachable" - 1 }, + { "vla-bound", SANITIZE_VLA, sizeof "vla-bound" - 1 }, { NULL, 0, 0 } }; const char *comma; --- gcc/c-family/c-ubsan.c.mp 2013-09-12 13:30:17.306967720 +0200 +++ gcc/c-family/c-ubsan.c 2013-09-12 13:31:31.469263169 +0200 @@ -25,7 +25,6 @@ along with GCC; see the file COPYING3. #include "alloc-pool.h" #include "cgraph.h" #include "gimple.h" -#include "hash-table.h" #include "output.h" #include "toplev.h" #include "ubsan.h" @@ -89,8 +88,7 @@ ubsan_instrument_division (location_t lo return t; } -/* Instrument left and right shifts. If not instrumenting, return - NULL_TREE. */ +/* Instrument left and right shifts. */ tree ubsan_instrument_shift (location_t loc, enum tree_code code, @@ -155,4 +153,23 @@ ubsan_instrument_shift (location_t loc, t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_zero_node); return t; +} + +/* Instrument variable length array bound. */ + +tree +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)); + tree data = ubsan_create_data ("__ubsan_vla_data", + loc, ubsan_type_descriptor (type), NULL_TREE); + data = build_fold_addr_expr_loc (loc, data); + tt = builtin_decl_explicit (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE); + tt = build_call_expr_loc (loc, tt, 2, data, ubsan_encode_value (size)); + t = fold_build3 (COND_EXPR, void_type_node, t, tt, void_zero_node); + + return t; } --- gcc/c-family/c-ubsan.h.mp 2013-09-12 13:30:25.609000661 +0200 +++ gcc/c-family/c-ubsan.h 2013-09-12 13:31:31.475263194 +0200 @@ -23,5 +23,6 @@ along with GCC; see the file COPYING3. extern tree ubsan_instrument_division (location_t, tree, tree); extern tree ubsan_instrument_shift (location_t, enum tree_code, tree, tree); +extern tree ubsan_instrument_vla (location_t, tree); #endif /* GCC_C_UBSAN_H */ --- gcc/sanitizer.def.mp 2013-09-12 13:30:59.667138181 +0200 +++ gcc/sanitizer.def 2013-09-12 13:31:31.497263294 +0200 @@ -297,3 +297,7 @@ DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HAN "__ubsan_handle_builtin_unreachable", BT_FN_VOID_PTR, ATTR_COLD_NORETURN_NOTHROW_LEAF_LIST) +DEF_SANITIZER_BUILTIN(BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE, + "__ubsan_handle_vla_bound_not_positive", + BT_FN_VOID_PTR_PTR, + ATTR_COLD_NOTHROW_LEAF_LIST) --- gcc/ubsan.c.mp 2013-09-12 13:31:06.197163836 +0200 +++ gcc/ubsan.c 2013-09-12 13:31:31.498263298 +0200 @@ -261,7 +261,9 @@ ubsan_type_descriptor (tree type) /* At least for INTEGER_TYPE/REAL_TYPE/COMPLEX_TYPE, this should work. ??? For e.g. type_unsigned_for (type), the TYPE_NAME would be NULL. */ - if (TYPE_NAME (type) != NULL) + if (TREE_CODE (TYPE_NAME (type)) == IDENTIFIER_NODE) + tname = IDENTIFIER_POINTER (TYPE_NAME (type)); + else if (TYPE_NAME (type) != NULL) tname = IDENTIFIER_POINTER (DECL_NAME (TYPE_NAME (type))); else tname = ""; --- gcc/flag-types.h.mp 2013-09-12 13:30:47.130090269 +0200 +++ gcc/flag-types.h 2013-09-12 13:31:31.495263285 +0200 @@ -201,7 +201,9 @@ enum sanitize_code { SANITIZE_SHIFT = 1 << 2, SANITIZE_DIVIDE = 1 << 3, SANITIZE_UNREACHABLE = 1 << 4, + SANITIZE_VLA = 1 << 5, SANITIZE_UNDEFINED = SANITIZE_SHIFT | SANITIZE_DIVIDE | SANITIZE_UNREACHABLE + | SANITIZE_VLA }; /* flag_vtable_verify initialization levels. */ --- gcc/cp/decl.c.mp 2013-09-12 13:30:39.641060204 +0200 +++ gcc/cp/decl.c 2013-09-12 13:31:31.488263253 +0200 @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. #include "c-family/c-common.h" #include "c-family/c-objc.h" #include "c-family/c-pragma.h" +#include "c-family/c-ubsan.h" #include "diagnostic.h" #include "intl.h" #include "debug.h" @@ -8462,6 +8463,24 @@ create_array_type_for_decl (tree name, t if (cxx_dialect >= cxx1y && array_of_runtime_bound_p (type)) pedwarn (input_location, OPT_Wvla, "array of array of runtime bound"); + /* Do the instrumentation of VLAs if desired. */ + if ((flag_sanitize & SANITIZE_VLA) + && size && !TREE_CONSTANT (size) + /* From C++1y onwards, we throw an exception on a negative length size + of an array. */ + && cxx_dialect < cxx1y) + { + /* Prevent bogus set-but-not-used warnings: we're definitely using + the variable. */ + if (VAR_P (size)) + DECL_READ_P (size) = 1; + /* Evaluate the array size only once. */ + size = cp_save_expr (size); + size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size), + ubsan_instrument_vla (input_location, size), + size); + } + /* Figure out the index type for the array. */ if (size) itype = compute_array_index_type (name, size, tf_warning_or_error); --- gcc/c/c-decl.c.mp 2013-09-12 13:30:32.352029153 +0200 +++ gcc/c/c-decl.c 2013-09-12 13:31:31.478263209 +0200 @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3. #include "c-family/c-common.h" #include "c-family/c-objc.h" #include "c-family/c-pragma.h" +#include "c-family/c-ubsan.h" #include "c-lang.h" #include "langhooks.h" #include "tree-iterator.h" @@ -5381,6 +5382,16 @@ grokdeclarator (const struct c_declarato with known value. */ this_size_varies = size_varies = true; warn_variable_length_array (name, size); + if (flag_sanitize & SANITIZE_VLA + && decl_context == NORMAL) + { + /* Evaluate the array size only once. */ + size = c_save_expr (size); + size = c_fully_fold (size, false, NULL); + size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size), + ubsan_instrument_vla (loc, size), + size); + } } if (integer_zerop (size) && !this_size_varies) --- gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C.mp 2013-09-12 13:17:55.242089503 +0200 +++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C 2013-09-12 13:27:38.649460187 +0200 @@ -0,0 +1,13 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound -w -std=c++1y" } */ +/* { dg-shouldfail "ubsan" } */ + +int +main (void) +{ + int y = -18; + int a[y]; + return 0; +} + +/* { dg-output "terminate called after throwing an instance" } */ --- gcc/testsuite/c-c++-common/ubsan/vla-3.c.mp 2013-09-12 10:48:11.719745997 +0200 +++ gcc/testsuite/c-c++-common/ubsan/vla-3.c 2013-09-12 12:06:43.178724666 +0200 @@ -0,0 +1,16 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound -w" } */ + +/* Don't instrument the arrays here. */ +int +foo (int n, int a[]) +{ + return a[n - 1]; +} + +int +main (void) +{ + int a[6] = { }; + return foo (3, a); +} --- gcc/testsuite/c-c++-common/ubsan/vla-2.c.mp 2013-09-12 10:47:56.662693753 +0200 +++ gcc/testsuite/c-c++-common/ubsan/vla-2.c 2013-09-12 12:06:28.698670292 +0200 @@ -0,0 +1,15 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound -w" } */ + +int +main (void) +{ + const int t = 0; + struct s { + int x; + /* Don't instrument this one. */ + int g[t]; + }; + + return 0; +} --- gcc/testsuite/c-c++-common/ubsan/vla-4.c.mp 2013-09-12 10:48:14.023754028 +0200 +++ gcc/testsuite/c-c++-common/ubsan/vla-4.c 2013-09-12 12:00:37.639137936 +0200 @@ -0,0 +1,13 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound" } */ + +int +main (void) +{ + int x = 1; + /* Check that the size of an array is evaluated only once. */ + int a[++x]; + if (x != 2) + __builtin_abort (); + return 0; +} --- gcc/testsuite/c-c++-common/ubsan/vla-1.c.mp 2013-09-12 10:47:54.377685875 +0200 +++ gcc/testsuite/c-c++-common/ubsan/vla-1.c 2013-09-12 11:00:37.693810414 +0200 @@ -0,0 +1,48 @@ +/* { dg-do run } */ +/* { dg-options "-fsanitize=vla-bound -w" } */ + +static int +bar (void) +{ + return -42; +} + +typedef long int V; +int +main (void) +{ + int x = -1; + double di = -3.2; + V v = -666; + + int a[x]; + int aa[x][x]; + int aaa[x][x][x]; + int b[x - 4]; + int c[(int) di]; + int d[1 + x]; + int e[1 ? x : -1]; + int f[++x]; + int g[(signed char) --x]; + int h[(++x, --x, x)]; + int i[v]; + int j[bar ()]; + + return 0; +} + +/* { dg-output "variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -5(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -3(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value 0(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -1(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -666(\n|\r\n|\r)" } */ +/* { dg-output "\[^\n\r]*variable length array bound evaluates to non-positive value -42(\n|\r\n|\r)" } */ --- gcc/asan.c.mp 2013-09-12 13:30:10.530941672 +0200 +++ gcc/asan.c 2013-09-12 13:31:31.469263169 +0200 @@ -2018,6 +2018,9 @@ initialize_sanitizer_builtins (void) tree BT_FN_VOID = build_function_type_list (void_type_node, NULL_TREE); tree BT_FN_VOID_PTR = build_function_type_list (void_type_node, ptr_type_node, NULL_TREE); + tree BT_FN_VOID_PTR_PTR + = build_function_type_list (void_type_node, ptr_type_node, + ptr_type_node, NULL_TREE); tree BT_FN_VOID_PTR_PTR_PTR = build_function_type_list (void_type_node, ptr_type_node, ptr_type_node, ptr_type_node, NULL_TREE);