From c7728805a7107444683290cd629d13f089130a0d Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Tue, 28 Feb 2023 11:38:46 +0100 Subject: ubsan: Honor -fstrict-flex-arrays= in -fsanitize=bounds [PR108894] While this isn't really a regression, the -fstrict-flex-arrays* option is new in GCC 13 and so I think we should make -fsanitize=bounds play with it well from the beginning. The current behavior is that -fsanitize=bounds considers all trailing arrays as flexible member-like arrays and both -fsanitize=bounds and -fsanitize=bounds-strict because of a bug don't even instrument [0] arrays at all, not as trailing nor when followed by other members. I think -fstrict-flex-arrays* options can be considered as language mode changing options, by default flexible member-like arrays are handled like flexible arrays, but that option can change the set of the arrays which are treated like that. So, -fsanitize=bounds should change with that on what is considered acceptable and what isn't. While -fsanitize=bounds-strict should reject them all always to continue previous behavior. The following patch implements that. To support [0] array instrumentation, I had to change the meaning of the bounds argument to .UBSAN_BOUNDS, previously it was the TYPE_MAX_VALUE of the domain unless ignore_off_by_one (used for taking address of the array element rather than accessing it; in that case 1 is added to the bound argument) and the later lowered checks were if (index > bound) report_failure (). The problem with that is that for [0] arrays where (at least for C++) the max value is all ones, for accesses that condition will be never true; for addresses of elements it was working (in C++) correctly before. This patch changes it to add 1 + ignore_off_by_one, so -1 becomes 0 or 1 for &array_ref and changing the lowering to be if (index >= bound) report_failure (). Furthermore, as C represents the [0] arrays with NULL TYPE_MAX_VALUE, I treated those like the C++ ones. 2023-02-28 Jakub Jelinek PR sanitizer/108894 gcc/ * ubsan.cc (ubsan_expand_bounds_ifn): Emit index >= bound comparison rather than index > bound. * gimple-fold.cc (gimple_fold_call): Use tree_int_cst_lt rather than tree_int_cst_le for IFN_UBSAN_BOUND comparison. * doc/invoke.texi (-fsanitize=bounds): Document that whether flexible array member-like arrays are instrumented or not depends on -fstrict-flex-arrays* options of strict_flex_array attributes. (-fsanitize=bounds-strict): Document that flexible array members are not instrumented. gcc/c-family/ * c-common.h (c_strict_flex_array_level_of): Declare. * c-common.cc (c_strict_flex_array_level_of): New function, moved and renamed from c-decl.cc's strict_flex_array_level_of. * c-ubsan.cc (ubsan_instrument_bounds): Fix comment typo. For C check c_strict_flex_array_level_of whether a trailing array should be treated as flexible member like. Handle C [0] arrays. Add 1 + index_off_by_one rather than index_off_by_one to bounds and use tree_int_cst_lt rather than tree_int_cst_le for idx vs. bounds comparison. gcc/c/ * c-decl.cc (strict_flex_array_level_of): Move to c-common.cc and rename to c_strict_flex_array_level_of. (is_flexible_array_member_p): Adjust caller. gcc/testsuite/ * gcc.dg/ubsan/bounds-4.c: New test. * gcc.dg/ubsan/bounds-4a.c: New test. * gcc.dg/ubsan/bounds-4b.c: New test. * gcc.dg/ubsan/bounds-4c.c: New test. * gcc.dg/ubsan/bounds-4d.c: New test. * g++.dg/ubsan/bounds-1.C: New test. --- gcc/c/c-decl.cc | 31 +------------------------------ 1 file changed, 1 insertion(+), 30 deletions(-) (limited to 'gcc/c') diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc index 08078ea..9159965 100644 --- a/gcc/c/c-decl.cc +++ b/gcc/c/c-decl.cc @@ -9050,35 +9050,6 @@ finish_incomplete_vars (tree incomplete_vars, bool toplevel) } } -/* Get the LEVEL of the strict_flex_array for the ARRAY_FIELD based on the - values of attribute strict_flex_array and the flag_strict_flex_arrays. */ -static unsigned int -strict_flex_array_level_of (tree array_field) -{ - gcc_assert (TREE_CODE (array_field) == FIELD_DECL); - unsigned int strict_flex_array_level = flag_strict_flex_arrays; - - tree attr_strict_flex_array - = lookup_attribute ("strict_flex_array", DECL_ATTRIBUTES (array_field)); - /* If there is a strict_flex_array attribute attached to the field, - override the flag_strict_flex_arrays. */ - if (attr_strict_flex_array) - { - /* Get the value of the level first from the attribute. */ - unsigned HOST_WIDE_INT attr_strict_flex_array_level = 0; - gcc_assert (TREE_VALUE (attr_strict_flex_array) != NULL_TREE); - attr_strict_flex_array = TREE_VALUE (attr_strict_flex_array); - gcc_assert (TREE_VALUE (attr_strict_flex_array) != NULL_TREE); - attr_strict_flex_array = TREE_VALUE (attr_strict_flex_array); - gcc_assert (tree_fits_uhwi_p (attr_strict_flex_array)); - attr_strict_flex_array_level = tree_to_uhwi (attr_strict_flex_array); - - /* The attribute has higher priority than flag_struct_flex_array. */ - strict_flex_array_level = attr_strict_flex_array_level; - } - return strict_flex_array_level; -} - /* Determine whether the FIELD_DECL X is a flexible array member according to the following info: A. whether the FIELD_DECL X is the last field of the DECL_CONTEXT; @@ -9105,7 +9076,7 @@ is_flexible_array_member_p (bool is_last_field, bool is_one_element_array = one_element_array_type_p (TREE_TYPE (x)); bool is_flexible_array = flexible_array_member_type_p (TREE_TYPE (x)); - unsigned int strict_flex_array_level = strict_flex_array_level_of (x); + unsigned int strict_flex_array_level = c_strict_flex_array_level_of (x); switch (strict_flex_array_level) { -- cgit v1.1