Message ID | 20130925124132.GJ12296@redhat.com |
---|---|
State | New |
Headers | show |
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
Ping^2. Jason, Joseph, are you fine with the C++/C FE changes? Thanks. On Mon, Oct 07, 2013 at 10:17:38PM +0200, Marek Polacek wrote: > 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 Marek
On Tue, 15 Oct 2013, Marek Polacek wrote:
> Ping^2. Jason, Joseph, are you fine with the C++/C FE changes?
The C changes are fine with me.
On 09/25/2013 08:41 AM, Marek Polacek wrote: > + /* 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) This code is in a completely different place from the C++1y code in cp_finish_decl; they should be in the same place. I'm also concerned that doing it here will mean adding sanitization code to template definitions, but I think we want to wait to add it until instantiation time. > + /* Prevent bogus set-but-not-used warnings: we're definitely using > + the variable. */ > + if (VAR_P (size)) > + DECL_READ_P (size) = 1; Use mark_rvalue_use for this. Jason
--- 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);