On Fri, Sep 27, 2024 at 04:01:33PM +0200, Jakub Jelinek wrote:
> So, I think we should go with (but so far completely untested except
> for pr78687.C which is optimized with Marek's patch and the above testcase
> which doesn't have the clearing anymore) the following patch.
That patch had a bug in type_has_padding_at_level_p and so it didn't
bootstrap.
Here is a full patch which does. It regressed the infoleak-1.c test
which I've adjusted, but I think the test had undefined behavior.
In particular the question is whether
union un_b { unsigned char j; unsigned int i; } u = {0};
leaves (or can leave) some bits uninitialized or not.
I believe it can, it is an explicit initialization of the j member
which is just 8-bit (but see my upcoming mail on padding bits in C23/C++)
and nothing in the C standard from what I can see seems to imply the padding
bits in the union beyond the actually initialized field in this case would
be initialized.
Though, looking at godbolt, clang and icc 19 and older gcc all do zero
initialize the whole union before storing the single member in there (if
non-zero, otherwise just clear).
So whether we want to do this or do it by default is another question.
Anyway, bootstrapped/regtested on x86_64-linux and i686-linux successfully.
2024-09-28 Jakub Jelinek <ja...@redhat.com>
PR c++/116416
* expr.cc (categorize_ctor_elements_1): Fix up union handling of
*p_complete. Clear it only if num_fields is 0 and the union has
at least one FIELD_DECL, set to -1 if either union has no fields
and non-zero size, or num_fields is 1 and complete_ctor_at_level_p
returned false.
* gimple-fold.cc (type_has_padding_at_level_p): Fix up UNION_TYPE
handling, return also true for UNION_TYPE with no FIELD_DECLs
and non-zero size, handle QUAL_UNION_TYPE like UNION_TYPE.
* gcc.dg/plugin/infoleak-1.c (test_union_2b, test_union_4b): Expect
diagnostics.
--- gcc/expr.cc.jj 2024-09-04 12:09:22.598808244 +0200
+++ gcc/expr.cc 2024-09-27 15:34:20.929282525 +0200
@@ -7218,7 +7218,36 @@ categorize_ctor_elements_1 (const_tree c
if (*p_complete && !complete_ctor_at_level_p (TREE_TYPE (ctor),
num_fields, elt_type))
- *p_complete = 0;
+ {
+ if (TREE_CODE (TREE_TYPE (ctor)) == UNION_TYPE
+ || TREE_CODE (TREE_TYPE (ctor)) == QUAL_UNION_TYPE)
+ {
+ /* The union case is more complicated. */
+ if (num_fields == 0)
+ {
+ /* If the CONSTRUCTOR doesn't have any elts, it is
+ incomplete if the union has at least one field. */
+ for (tree f = TYPE_FIELDS (TREE_TYPE (ctor));
+ f; f = DECL_CHAIN (f))
+ if (TREE_CODE (f) == FIELD_DECL)
+ {
+ *p_complete = 0;
+ break;
+ }
+ /* Otherwise it has padding if the union has non-zero size. */
+ if (*p_complete > 0
+ && !integer_zerop (TYPE_SIZE (TREE_TYPE (ctor))))
+ *p_complete = -1;
+ }
+ /* Otherwise the CONSTRUCTOR should have exactly one element.
+ It is then never incomplete, but if complete_ctor_at_level_p
+ returned false, it has padding. */
+ else if (*p_complete > 0)
+ *p_complete = -1;
+ }
+ else
+ *p_complete = 0;
+ }
else if (*p_complete > 0
&& type_has_padding_at_level_p (TREE_TYPE (ctor)))
*p_complete = -1;
--- gcc/gimple-fold.cc.jj 2024-09-09 11:25:43.197048840 +0200
+++ gcc/gimple-fold.cc 2024-09-27 18:51:30.036002116 +0200
@@ -4814,12 +4814,22 @@ type_has_padding_at_level_p (tree type)
return false;
}
case UNION_TYPE:
+ case QUAL_UNION_TYPE:
+ bool any_fields;
+ any_fields = false;
/* If any of the fields is smaller than the whole, there is padding. */
for (tree f = TYPE_FIELDS (type); f; f = DECL_CHAIN (f))
- if (TREE_CODE (f) == FIELD_DECL)
- if (simple_cst_equal (TYPE_SIZE (TREE_TYPE (f)),
- TREE_TYPE (type)) != 1)
- return true;
+ if (TREE_CODE (f) != FIELD_DECL)
+ continue;
+ else if (simple_cst_equal (TYPE_SIZE (TREE_TYPE (f)),
+ TYPE_SIZE (type)) != 1)
+ return true;
+ else
+ any_fields = true;
+ /* If the union doesn't have any fields and still has non-zero size,
+ all of it is padding. */
+ if (!any_fields && !integer_zerop (TYPE_SIZE (type)))
+ return true;
return false;
case ARRAY_TYPE:
case COMPLEX_TYPE:
--- gcc/testsuite/gcc.dg/plugin/infoleak-1.c.jj 2022-09-11 22:28:56.356164436
+0200
+++ gcc/testsuite/gcc.dg/plugin/infoleak-1.c 2024-09-28 08:20:25.806973480
+0200
@@ -123,9 +123,12 @@ void test_union_2a (void __user *dst, u8
void test_union_2b (void __user *dst, u8 v)
{
- union un_b u = {0};
+ union un_b u = {0}; /* { dg-message "region created on stack here" "where" }
*/
+ /* { dg-message "capacity: 4 bytes" "capacity" { target *-*-* } .-1 } */
u.j = v;
- copy_to_user(dst, &u, sizeof (union un_b));
+ copy_to_user(dst, &u, sizeof (union un_b)); /* { dg-warning "potential
exposure of sensitive information by copying uninitialized data from stack"
"warning" } */
+ /* { dg-message "3 bytes are uninitialized" "note how much" { target *-*-* }
.-1 } */
+ /* { dg-message "bytes 1 - 3 are uninitialized" "note how much" { target
*-*-* } .-2 } */
}
void test_union_3a (void __user *dst, u32 v)
@@ -150,8 +153,11 @@ void test_union_4a (void __user *dst, u8
void test_union_4b (void __user *dst, u8 v)
{
- union un_b u = {0};
- copy_to_user(dst, &u, sizeof (union un_b)); /* { dg-bogus "" } */
+ union un_b u = {0}; /* { dg-message "region created on stack here" "where" }
*/
+ /* { dg-message "capacity: 4 bytes" "capacity" { target *-*-* } .-1 } */
+ copy_to_user(dst, &u, sizeof (union un_b)); /* { dg-warning "potential
exposure of sensitive information by copying uninitialized data from stack"
"warning" } */
+ /* { dg-message "3 bytes are uninitialized" "note how much" { target *-*-* }
.-1 } */
+ /* { dg-message "bytes 1 - 3 are uninitialized" "note how much" { target
*-*-* } .-2 } */
}
struct st_union_5
Jakub
