From: Qing Zhao <qing.zhao@oracle.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Joseph Myers <joseph@codesourcery.com>,
"Sandra@codesourcery.com" <Sandra@codesourcery.com>,
Siddhesh Poyarekar <siddhesh@gotplt.org>,
Richard Biener <richard.guenther@gmail.com>,
Kees Cook <keescook@chromium.org>,
gcc Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [V5][PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]
Date: Mon, 27 Mar 2023 14:22:33 +0000 [thread overview]
Message-ID: <93D8254A-2611-47BE-9881-F25C64A7F078@oracle.com> (raw)
In-Reply-To: <95180085-897F-4B87-BE0E-78ACF1808326@oracle.com>
Hi, Jakub,
Could you please review the middle end part of the changes of this patch? (The C FE part changes were Okayed by Joseph already).
The major change is in tree-object-size.cc (addr_object_size). (To use the new TYPE_INCLUDE_FLEXARRAY info).
This patch is to fix PR101832(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101832), and is needed for Linux Kernel security. It’s better to be put into GCC13.
Thanks a lot!
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614101.html
https://gcc.gnu.org/pipermail/gcc-patches/2023-March/614511.html
> On Mar 23, 2023, at 9:03 AM, Qing Zhao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
> Ping…
>
> Please let me know if you have any further comments on the patch.
>
> thanks.
>
> Qing
>
>
> Begin forwarded message:
>
> From: Qing Zhao <qing.zhao@oracle.com<mailto:qing.zhao@oracle.com>>
> Subject: [V5][PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832]
> Date: March 16, 2023 at 5:47:14 PM EDT
> To: joseph@codesourcery.com<mailto:joseph@codesourcery.com>, jakub@redhat.com<mailto:jakub@redhat.com>, sandra@codesourcery.com<mailto:sandra@codesourcery.com>
> Cc: rguenther@suse.de<mailto:rguenther@suse.de>, siddhesh@gotplt.org<mailto:siddhesh@gotplt.org>, keescook@chromium.org<mailto:keescook@chromium.org>, gcc-patches@gcc.gnu.org<mailto:gcc-patches@gcc.gnu.org>, Qing Zhao <qing.zhao@oracle.com<mailto:qing.zhao@oracle.com>>
>
> GCC extension accepts the case when a struct with a flexible array member
> is embedded into another struct or union (possibly recursively).
> __builtin_object_size should treat such struct as flexible size per
> -fstrict-flex-arrays.
>
> gcc/c/ChangeLog:
>
> PR tree-optimization/101832
> * c-decl.cc<http://c-decl.cc> (finish_struct): Set TYPE_INCLUDE_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_INCLUDE_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_include_flexarray.
> * tree-core.h (struct tree_type_common): Use bit no_named_args_stdarg_p
> as type_include_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_INCLUDE_FLEXARRAY): New macro TYPE_INCLUDE_FLEXARRAY.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/101832
> * gcc.dg/builtin-object-size-pr101832.c: New test.
> ---
> 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 e537d33f398..14c54809b9d 100644
> --- a/gcc/c/c-decl.cc<http://c-decl.cc>
> +++ b/gcc/c/c-decl.cc<http://c-decl.cc>
> @@ -9258,6 +9258,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_INCLUDE_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_INCLUDE_FLEXARRAY (t)
> + = is_last_field && flexible_array_member_type_p (TREE_TYPE (x));
> + /* Recursively set TYPE_INCLUDE_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_INCLUDE_FLEXARRAY (t)
> + = is_last_field && TYPE_INCLUDE_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 882dd8971a4..9dde7118266 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_INCLUDE_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 1f3afcbbc86..efacdb7686f 100644
> --- a/gcc/print-tree.cc<http://print-tree.cc>
> +++ b/gcc/print-tree.cc<http://print-tree.cc>
> @@ -631,6 +631,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_INCLUDE_FLEXARRAY (node))
> + fputs (" include-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 fd2be57b78c..1d94c9a8ce2 100644
> --- a/gcc/tree-core.h
> +++ b/gcc/tree-core.h
> @@ -1717,6 +1717,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_INCLUDE_FLEXARRAY
> + for RECORD_TYPE and UNION_TYPE. */
> unsigned no_named_args_stdarg_p : 1;
> unsigned spare : 15;
>
> 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..1619d144ecd 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_INCLUDE_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 d4dc30f048f..56add06a70a 100644
> --- a/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>
> +++ b/gcc/tree-streamer-in.cc<http://tree-streamer-in.cc>
> @@ -398,7 +398,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_INCLUDE_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 d107229da5c..64f6c8bbabb 100644
> --- a/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>
> +++ b/gcc/tree-streamer-out.cc<http://tree-streamer-out.cc>
> @@ -365,7 +365,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_INCLUDE_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 91375f9652f..18c9f416758 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
> + at the last field recursively. */
> +#define TYPE_INCLUDE_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
>
>
next prev parent reply other threads:[~2023-03-27 14:22 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-16 21:47 [V5][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
2023-03-16 21:47 ` [V5][PATCH 1/2] Handle component_ref to a structre/union field including flexible array member [PR101832] Qing Zhao
2023-03-23 13:03 ` Fwd: " Qing Zhao
2023-03-23 18:55 ` Joseph Myers
2023-03-27 13:31 ` Qing Zhao
2023-03-27 14:22 ` Qing Zhao [this message]
2023-03-16 21:47 ` [V5][PATCH 2/2] Update documentation to clarify a GCC extension Qing Zhao
2023-03-23 13:05 ` Fwd: " Qing Zhao
2023-03-23 20:14 ` Joseph Myers
2023-03-27 13:38 ` Qing Zhao
2023-03-27 14:34 ` Xi Ruoyao
2023-03-27 15:37 ` Qing Zhao
2023-03-27 15:43 ` Jakub Jelinek
2023-03-27 15:57 ` Qing Zhao
2023-03-27 16:06 ` Jakub Jelinek
2023-03-27 16:22 ` Qing Zhao
2023-03-27 16:31 ` Jakub Jelinek
2023-03-27 16:48 ` Qing Zhao
2023-03-27 17:29 ` Qing Zhao
2023-03-23 13:02 ` Fwd: [V5][PATCH 0/2] Handle component_ref to a structure/union field including FAM for builtin_object_size Qing Zhao
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=93D8254A-2611-47BE-9881-F25C64A7F078@oracle.com \
--to=qing.zhao@oracle.com \
--cc=Sandra@codesourcery.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=joseph@codesourcery.com \
--cc=keescook@chromium.org \
--cc=richard.guenther@gmail.com \
--cc=siddhesh@gotplt.org \
/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).