Ping * 2 : Fwd: [V9][PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]

[Qing Zhao <qing.zhao@oracle.com>]

Hi,

I’d like to ping this patch again for the Middle-end approval (on gcc/tree-object-size.cc change).
This is an important patch to Linux Kernel security. 

The patch has addressed all the comments and suggestions raised during the review process. 
The C FE, Doc changes has been approved.
Most of the Middle-end changes been reviewed by Richard Biener and have been updated based on his suggestions.

The only change that has not been reviewed is the simple change in gcc/tree-object-size.cc.  which is simple and straightforward.
Please review this change and let me know whether it’s okay for commit to trunk?

Thanks a lot!

Qing

> On Jun 5, 2023, at 11:12 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> Ping on this patch.
> 
> The C FE and Doc changes has been approved.
> Please help to review and approve the Middle-end change.
> 
> Or provide guide on how to move this patch forward.
> 
> Thanks a lot for the help.
> 
> Qing
> 
> Begin forwarded message:
> 
> From: Qing Zhao <qing.zhao@oracle.com<mailto:qing.zhao@oracle.com>>
> Subject: [V9][PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]
> Date: May 30, 2023 at 2:30:28 PM EDT
> To: joseph@codesourcery.com<mailto:joseph@codesourcery.com>, richard.guenther@gmail.com<mailto:richard.guenther@gmail.com>, jakub@redhat.com<mailto:jakub@redhat.com>, gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>
> Cc: keescook@chromium.org<mailto:keescook@chromium.org>, siddhesh@gotplt.org<mailto:siddhesh@gotplt.org>, uecker@tugraz.at<mailto:uecker@tugraz.at>, Qing Zhao <qing.zhao@oracle.com<mailto:qing.zhao@oracle.com>>
> 
> Richard or Jakub,
> 
> could you please review this patch and see whether it's Okay to commit?
> 
> thanks a lot.
> 
> Qing
> 
> ===================================
> 
> GCC extension accepts the case when a struct with a C99 flexible array member
> is embedded into another struct or union (possibly recursively) as the last
> field.
> __builtin_object_size should treat such struct as flexible size.
> 
> gcc/c/ChangeLog:
> 
> PR tree-optimization/101832
> * c-decl.cc<http://c-decl.cc> (finish_struct): Set TYPE_INCLUDES_FLEXARRAY for
> struct/union type.
> 
> gcc/lto/ChangeLog:
> 
> PR tree-optimization/101832
> * lto-common.cc<http://lto-common.cc> (compare_tree_sccs_1): Compare bit
> TYPE_NO_NAMED_ARGS_STDARG_P or TYPE_INCLUDES_FLEXARRAY properly
> for its corresponding type.
> 
> gcc/ChangeLog:
> 
> PR tree-optimization/101832
> * print-tree.cc<http://print-tree.cc> (print_node): Print new bit type_includes_flexarray.
> * tree-core.h (struct tree_type_common): Use bit no_named_args_stdarg_p
> as type_includes_flexarray for RECORD_TYPE or UNION_TYPE.
> * tree-object-size.cc<http://tree-object-size.cc> (addr_object_size): Handle structure/union type
> when it has flexible size.
> * tree-streamer-in.cc<http://tree-streamer-in.cc> (unpack_ts_type_common_value_fields): Stream
> in bit no_named_args_stdarg_p properly for its corresponding type.
> * tree-streamer-out.cc<http://tree-streamer-out.cc> (pack_ts_type_common_value_fields): Stream
> out bit no_named_args_stdarg_p properly for its corresponding type.
> * tree.h (TYPE_INCLUDES_FLEXARRAY): New macro TYPE_INCLUDES_FLEXARRAY.
> 
> gcc/testsuite/ChangeLog:
> 
> PR tree-optimization/101832
> * gcc.dg/builtin-object-size-pr101832.c: New test.
> 
> change TYPE_INCLUDES_FLEXARRAY to TYPE_INCLUDES_FLEXARRAY
> ---
> gcc/c/c-decl.cc<http://c-decl.cc>                               |  11 ++
> gcc/lto/lto-common.cc<http://lto-common.cc>                         |   5 +-
> gcc/print-tree.cc<http://print-tree.cc>                             |   5 +
> .../gcc.dg/builtin-object-size-pr101832.c     | 134 ++++++++++++++++++
> gcc/tree-core.h                               |   2 +
> gcc/tree-object-size.cc<http://tree-object-size.cc>                       |  23 ++-
> gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>                       |   5 +-
> gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>                      |   5 +-
> gcc/tree.h                                    |   7 +-
> 9 files changed, 192 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> 
> diff --git a/gcc/c/c-decl.cc<http://c-decl.cc> b/gcc/c/c-decl.cc<http://c-decl.cc>
> index b5b491cf2da..0c718151f6d 100644
> --- a/gcc/c/c-decl.cc<http://c-decl.cc>
> +++ b/gcc/c/c-decl.cc<http://c-decl.cc>
> @@ -9282,6 +9282,17 @@ finish_struct (location_t loc, tree t, tree fieldlist, tree attributes,
>      /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x.  */
>      DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, x);
> 
> +      /* Set TYPE_INCLUDES_FLEXARRAY for the context of x, t.
> + when x is an array and is the last field.  */
> +      if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE)
> + TYPE_INCLUDES_FLEXARRAY (t)
> +  = is_last_field && flexible_array_member_type_p (TREE_TYPE (x));
> +      /* Recursively set TYPE_INCLUDES_FLEXARRAY for the context of x, t
> + when x is an union or record and is the last field.  */
> +      else if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> + TYPE_INCLUDES_FLEXARRAY (t)
> +  = is_last_field && TYPE_INCLUDES_FLEXARRAY (TREE_TYPE (x));
> +
>      if (DECL_NAME (x)
> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x)))
> saw_named_field = true;
> diff --git a/gcc/lto/lto-common.cc<http://lto-common.cc> b/gcc/lto/lto-common.cc<http://lto-common.cc>
> index 537570204b3..f6b85bbc6f7 100644
> --- a/gcc/lto/lto-common.cc<http://lto-common.cc>
> +++ b/gcc/lto/lto-common.cc<http://lto-common.cc>
> @@ -1275,7 +1275,10 @@ compare_tree_sccs_1 (tree t1, tree t2, tree **map)
>      if (AGGREGATE_TYPE_P (t1))
> compare_values (TYPE_TYPELESS_STORAGE);
>      compare_values (TYPE_EMPTY_P);
> -      compare_values (TYPE_NO_NAMED_ARGS_STDARG_P);
> +      if (FUNC_OR_METHOD_TYPE_P (t1))
> + compare_values (TYPE_NO_NAMED_ARGS_STDARG_P);
> +      if (RECORD_OR_UNION_TYPE_P (t1))
> + compare_values (TYPE_INCLUDES_FLEXARRAY);
>      compare_values (TYPE_PACKED);
>      compare_values (TYPE_RESTRICT);
>      compare_values (TYPE_USER_ALIGN);
> diff --git a/gcc/print-tree.cc<http://print-tree.cc> b/gcc/print-tree.cc<http://print-tree.cc>
> index ccecd3dc6a7..62451b6cf4e 100644
> --- a/gcc/print-tree.cc<http://print-tree.cc>
> +++ b/gcc/print-tree.cc<http://print-tree.cc>
> @@ -632,6 +632,11 @@ print_node (FILE *file, const char *prefix, tree node, int indent,
> && TYPE_CXX_ODR_P (node))
> fputs (" cxx-odr-p", file);
> 
> +      if ((code == RECORD_TYPE
> +   || code == UNION_TYPE)
> +  && TYPE_INCLUDES_FLEXARRAY (node))
> + fputs (" includes-flexarray", file);
> +
>      /* The transparent-union flag is used for different things in
> different nodes.  */
>      if ((code == UNION_TYPE || code == RECORD_TYPE)
> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> new file mode 100644
> index 00000000000..60078e11634
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c
> @@ -0,0 +1,134 @@
> +/* PR 101832:
> +   GCC extension accepts the case when a struct with a C99 flexible array
> +   member is embedded into another struct (possibly recursively).
> +   __builtin_object_size will treat such struct as flexible size.
> +   However, when a structure with non-C99 flexible array member, i.e, trailing
> +   [0], [1], or [4], is embedded into anther struct, the stucture will not
> +   be treated as flexible size.  */
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +#include "builtin-object-size-common.h"
> +
> +#define expect(p, _v) do { \
> +  size_t v = _v; \
> +  if (p == v) \
> +    __builtin_printf ("ok:  %s == %zd\n", #p, p); \
> +  else {\
> +    __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \
> +    FAIL (); \
> +  } \
> +} while (0);
> +
> +
> +struct A {
> +  int n;
> +  char data[];
> +};
> +
> +struct B {
> +  int m;
> +  struct A a;
> +};
> +
> +struct C {
> +  int q;
> +  struct B b;
> +};
> +
> +struct A0 {
> +  int n;
> +  char data[0];
> +};
> +
> +struct B0 {
> +  int m;
> +  struct A0 a;
> +};
> +
> +struct C0 {
> +  int q;
> +  struct B0 b;
> +};
> +
> +struct A1 {
> +  int n;
> +  char data[1];
> +};
> +
> +struct B1 {
> +  int m;
> +  struct A1 a;
> +};
> +
> +struct C1 {
> +  int q;
> +  struct B1 b;
> +};
> +
> +struct An {
> +  int n;
> +  char data[8];
> +};
> +
> +struct Bn {
> +  int m;
> +  struct An a;
> +};
> +
> +struct Cn {
> +  int q;
> +  struct Bn b;
> +};
> +
> +volatile void *magic1, *magic2;
> +
> +int main (int argc, char *argv[])
> +{
> +  struct B *outer;
> +  struct C *outest;
> +
> +  /* Make sure optimization can't find some other object size. */
> +  outer = (void *)magic1;
> +  outest = (void *)magic2;
> +
> +  expect (__builtin_object_size (&outer->a, 1), -1);
> +  expect (__builtin_object_size (&outest->b, 1), -1);
> +  expect (__builtin_object_size (&outest->b.a, 1), -1);
> +
> +  struct B0 *outer0;
> +  struct C0 *outest0;
> +
> +  /* Make sure optimization can't find some other object size. */
> +  outer0 = (void *)magic1;
> +  outest0 = (void *)magic2;
> +
> +  expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a));
> +  expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b));
> +  expect (__builtin_object_size (&outest0->b.a, 1), sizeof (outest0->b.a));
> +
> +  struct B1 *outer1;
> +  struct C1 *outest1;
> +
> +  /* Make sure optimization can't find some other object size. */
> +  outer1 = (void *)magic1;
> +  outest1 = (void *)magic2;
> +
> +  expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a));
> +  expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b));
> +  expect (__builtin_object_size (&outest1->b.a, 1), sizeof (outest1->b.a));
> +
> +  struct Bn *outern;
> +  struct Cn *outestn;
> +
> +  /* Make sure optimization can't find some other object size. */
> +  outern = (void *)magic1;
> +  outestn = (void *)magic2;
> +
> +  expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a));
> +  expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b));
> +  expect (__builtin_object_size (&outestn->b.a, 1), sizeof (outestn->b.a));
> +
> +  DONE ();
> +  return 0;
> +}
> diff --git a/gcc/tree-core.h b/gcc/tree-core.h
> index 9d44c04bf03..b2f619c5efc 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1712,6 +1712,8 @@ struct GTY(()) tree_type_common {
>  unsigned typeless_storage : 1;
>  unsigned empty_flag : 1;
>  unsigned indivisible_p : 1;
> +  /* TYPE_NO_NAMED_ARGS_STDARG_P for a stdarg function.
> +     Or TYPE_INCLUDES_FLEXARRAY for RECORD_TYPE and UNION_TYPE.  */
>  unsigned no_named_args_stdarg_p : 1;
>  unsigned spare : 1;
> 
> diff --git a/gcc/tree-object-size.cc<http://tree-object-size.cc> b/gcc/tree-object-size.cc<http://tree-object-size.cc>
> index 9a936a91983..a62af050056 100644
> --- a/gcc/tree-object-size.cc<http://tree-object-size.cc>
> +++ b/gcc/tree-object-size.cc<http://tree-object-size.cc>
> @@ -633,11 +633,32 @@ addr_object_size (struct object_size_info *osi, const_tree ptr,
>   v = NULL_TREE;
>   break;
> case COMPONENT_REF:
> -    if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE)
> +    /* When the ref is not to an aggregate type, i.e, an array,
> +       a record or a union, it will not have flexible size,
> +       compute the object size directly.  */
> +    if (!AGGREGATE_TYPE_P (TREE_TYPE (v)))
>     {
> v = NULL_TREE;
> break;
>     }
> +    /* if the ref is to a record or union type, but the type
> +       does not include a flexible array recursively, compute
> +       the object size directly.  */
> +    if (RECORD_OR_UNION_TYPE_P (TREE_TYPE (v)))
> +      {
> + if (!TYPE_INCLUDES_FLEXARRAY (TREE_TYPE (v)))
> +  {
> +    v = NULL_TREE;
> +    break;
> +  }
> + else
> +  {
> +    v = TREE_OPERAND (v, 0);
> +    break;
> +  }
> +      }
> +    /* Now the ref is to an array type.  */
> +    gcc_assert (TREE_CODE (TREE_TYPE (v)) == ARRAY_TYPE);
>   is_flexible_array_mem_ref = array_ref_flexible_size_p (v);
>   while (v != pt_var && TREE_CODE (v) == COMPONENT_REF)
>     if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0)))
> diff --git a/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc> b/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>
> index c803800862c..be2bdbb7699 100644
> --- a/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>
> +++ b/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>
> @@ -386,7 +386,10 @@ unpack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>  if (AGGREGATE_TYPE_P (expr))
>    TYPE_TYPELESS_STORAGE (expr) = (unsigned) bp_unpack_value (bp, 1);
>  TYPE_EMPTY_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> -  TYPE_NO_NAMED_ARGS_STDARG_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> +  if (FUNC_OR_METHOD_TYPE_P (expr))
> +    TYPE_NO_NAMED_ARGS_STDARG_P (expr) = (unsigned) bp_unpack_value (bp, 1);
> +  if (RECORD_OR_UNION_TYPE_P (expr))
> +    TYPE_INCLUDES_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1);
>  TYPE_PRECISION (expr) = bp_unpack_var_len_unsigned (bp);
>  SET_TYPE_ALIGN (expr, bp_unpack_var_len_unsigned (bp));
> #ifdef ACCEL_COMPILER
> diff --git a/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc> b/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>
> index 5751f77273b..6d4a9d90da6 100644
> --- a/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>
> +++ b/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>
> @@ -355,7 +355,10 @@ pack_ts_type_common_value_fields (struct bitpack_d *bp, tree expr)
>  if (AGGREGATE_TYPE_P (expr))
>    bp_pack_value (bp, TYPE_TYPELESS_STORAGE (expr), 1);
>  bp_pack_value (bp, TYPE_EMPTY_P (expr), 1);
> -  bp_pack_value (bp, TYPE_NO_NAMED_ARGS_STDARG_P (expr), 1);
> +  if (FUNC_OR_METHOD_TYPE_P (expr))
> +    bp_pack_value (bp, TYPE_NO_NAMED_ARGS_STDARG_P (expr), 1);
> +  if (RECORD_OR_UNION_TYPE_P (expr))
> +    bp_pack_value (bp, TYPE_INCLUDES_FLEXARRAY (expr), 1);
>  bp_pack_var_len_unsigned (bp, TYPE_PRECISION (expr));
>  bp_pack_var_len_unsigned (bp, TYPE_ALIGN (expr));
> }
> diff --git a/gcc/tree.h b/gcc/tree.h
> index 0b72663e6a1..d4755317a4c 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -786,7 +786,12 @@ extern void omp_clause_range_check_failed (const_tree, const char *, int,
>   (...) prototype, where arguments can be accessed with va_start and
>   va_arg), as opposed to an unprototyped function.  */
> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \
> -  (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> +  (FUNC_OR_METHOD_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> +
> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member
> +   as the last field recursively.  */
> +#define TYPE_INCLUDES_FLEXARRAY(NODE) \
> +  (RECORD_OR_UNION_CHECK (NODE)->type_common.no_named_args_stdarg_p)
> 
> /* In an IDENTIFIER_NODE, this means that assemble_name was called with
>   this string as an argument.  */
> --
> 2.31.1
> 
>