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: Wed, 25 Sep 2013 13:23:00 -0000 [thread overview]
Message-ID: <20130925124132.GJ12296@redhat.com> (raw)
In-Reply-To: <20130912122655.GN23899@redhat.com>
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
next prev parent reply other threads:[~2013-09-25 12:41 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 [this message]
2013-10-07 20:17 ` Marek Polacek
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=20130925124132.GJ12296@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).