From: Marek Polacek <polacek@redhat.com>
To: GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Jakub Jelinek <jakub@redhat.com>,
Jason Merrill <jason@redhat.com>,
"Joseph S. Myers" <joseph@codesourcery.com>
Subject: Re: [PATCH][ubsan] Add VLA bound instrumentation
Date: Mon, 07 Oct 2013 20:17:00 -0000 [thread overview]
Message-ID: <20131007201738.GC1404@redhat.com> (raw)
In-Reply-To: <20130925124132.GJ12296@redhat.com>
Ping.
On Wed, Sep 25, 2013 at 02:41:32PM +0200, Marek Polacek wrote:
> On Thu, Sep 12, 2013 at 02:26:55PM +0200, Marek Polacek wrote:
> > 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?
>
> I'd like to ping this patch; below is rebased version with the ubsan.c
> hunk omitted, since that part was already fixed by another patch.
>
> (It still doesn't contain alloca/SIZE_MAX/... checking, since that
> very much relies on libubsan. Still, it'd be felicitous to get at
> least the basic VLA checking in.)
>
> Ran ubsan testsuite + bootstrap-ubsan on x86_64-linux.
>
> 2013-09-25 Marek Polacek <polacek@redhat.com>
>
> * opts.c (common_handle_option): Handle vla-bound.
> * sanitizer.def (BUILT_IN_UBSAN_HANDLE_VLA_BOUND_NOT_POSITIVE):
> Define.
> * 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.
>
> --- gcc/opts.c.mp 2013-09-25 14:06:58.531276511 +0200
> +++ gcc/opts.c 2013-09-25 14:07:03.580294566 +0200
> @@ -1428,6 +1428,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-25 14:06:58.535276527 +0200
> +++ gcc/c-family/c-ubsan.c 2013-09-25 14:07:03.580294566 +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"
> @@ -86,8 +85,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,
> @@ -157,4 +155,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-25 14:06:58.538276539 +0200
> +++ gcc/c-family/c-ubsan.h 2013-09-25 14:07:03.595294628 +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-25 14:06:58.542276558 +0200
> +++ gcc/sanitizer.def 2013-09-25 14:07:03.628294753 +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/flag-types.h.mp 2013-09-25 14:06:58.546276575 +0200
> +++ gcc/flag-types.h 2013-09-25 14:07:03.629294757 +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-25 14:06:58.549276587 +0200
> +++ gcc/cp/decl.c 2013-09-25 14:07:20.640355737 +0200
> @@ -45,6 +45,7 @@ along with GCC; see the file COPYING3.
> #include "c-family/c-objc.h"
> #include "c-family/c-pragma.h"
> #include "c-family/c-target.h"
> +#include "c-family/c-ubsan.h"
> #include "diagnostic.h"
> #include "intl.h"
> #include "debug.h"
> @@ -8465,6 +8466,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-25 14:06:58.550276591 +0200
> +++ gcc/c/c-decl.c 2013-09-25 14:07:03.644294820 +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"
> @@ -5378,6 +5379,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-25 14:08:33.263616709 +0200
> +++ gcc/testsuite/g++.dg/ubsan/cxx1y-vla.C 2013-09-25 14:07:03.650294845 +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-25 14:08:25.364588140 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-3.c 2013-09-25 14:07:03.650294845 +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-25 14:08:23.458581265 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-2.c 2013-09-25 14:07:03.651294849 +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-25 14:08:27.367595369 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-4.c 2013-09-25 14:07:03.652294853 +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-25 14:08:21.341573677 +0200
> +++ gcc/testsuite/c-c++-common/ubsan/vla-1.c 2013-09-25 14:07:03.652294853 +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-25 14:06:58.557276623 +0200
> +++ gcc/asan.c 2013-09-25 14:07:03.653294857 +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);
>
> Marek
Marek
next prev parent reply other threads:[~2013-10-07 20:17 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-12 12:38 Marek Polacek
2013-09-12 12:48 ` Marek Polacek
2013-09-12 16:12 ` Joseph S. Myers
2013-09-12 16:20 ` Joseph S. Myers
2013-09-12 17:15 ` Marek Polacek
2013-09-13 10:29 ` Marek Polacek
2013-09-13 11:23 ` Eric Botcazou
2013-09-13 18:01 ` Joseph S. Myers
2013-09-16 11:13 ` Marek Polacek
2013-09-16 13:39 ` Florian Weimer
2013-09-12 16:29 ` Marek Polacek
2013-09-25 13:23 ` Marek Polacek
2013-10-07 20:17 ` Marek Polacek [this message]
2013-10-15 13:25 ` Marek Polacek
2013-10-15 15:01 ` Joseph S. Myers
2013-10-24 20:35 ` Jason Merrill
2013-10-25 17:38 ` Marek Polacek
2013-10-25 19:04 ` Jason Merrill
2013-10-25 19:15 ` Marek Polacek
2013-10-25 19:30 ` Jason Merrill
2013-10-30 15:16 ` Marek Polacek
2013-10-30 16:08 ` Jason Merrill
2013-10-30 16:20 ` Marek Polacek
2013-10-30 20:55 ` Mike Stump
2013-10-30 22:46 ` Marek Polacek
2013-10-30 22:50 ` Mike Stump
2013-10-31 11:12 ` Marek Polacek
2013-10-31 3:18 ` Jason Merrill
2013-10-31 19:07 ` Marek Polacek
2013-11-01 17:35 ` Jason Merrill
2013-11-01 19:10 ` Marek Polacek
2013-11-01 20:39 ` Jason Merrill
2013-11-02 13:06 ` Marek Polacek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131007201738.GC1404@redhat.com \
--to=polacek@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=jason@redhat.com \
--cc=joseph@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).