Re: [PATCH] handle initialized flexible array members in __builtin_object_size [PR92815]
On Thu, 2020-04-23 at 16:05 -0600, Martin Sebor wrote: > On 4/23/20 9:42 AM, Jeff Law wrote: > > On Wed, 2020-04-22 at 15:36 -0600, Martin Sebor via Gcc-patches wrote: > > > When computing the size of an object with a flexible array member > > > the object size pass doesn't consider that the initializer of such > > > an object can result in its size being in excess of the size of > > > the enclosing type. As a result, stores into such objects by > > > string functions causes false positive warnings and can abort > > > at runtime. > > > > > > The warnings are an old regression but as more of them make use > > > of the object size results more of them are affected by the bug. > > > The abort goes back to when support for _FORTIFY_SOURCE was added. > > > > > > The same problem has already been independently fixed in GCC 10 > > > for -Warray-bounds which doesn't use the object size checking pass, > > > but the object size bug still remains. The attached patch corrects > > > it as well. > > > > > > Tested on x86_64-linux. > > Do you need to change guarding condition to use decl_init_size instead of > > DECL_SIZE_UNIT as well? > > > > else if (pt_var > > && DECL_P (pt_var) > > && tree_fits_uhwi_p (DECL_SIZE_UNIT (pt_var)) > > ^^ > > && tree_to_uhwi (DECL_SIZE_UNIT (pt_var)) < offset_limit) > > ^^ > > It doesn't see that changing it is strictly necessary. If the tests > above pass and the result doesn't satisfy the conditions because it's > either null or doesn't fit in UHWI it's handled later by returning > false. With offset_limit set to SIZE_MAX / 2, I don't think the result > can as big as that or bigger: it would imply the whole object, including > its initializer, is bigger than half the address space and GCC rejects > such objects with an error. I've added another test in the patch to > to verify this. > > I do agree it would be better to validate the final result the same > way. That makes the change a little more intrusive to avoid validating > the size multiple times, but I think it also improves the readability > of the code, so the updated patch does that. It passes testing on > x86_64-linux. > > Let me know which one of the two you prefer, or if you'd rather hold > off until stage 1. So we're in stage1, so you're good to go with the patch as-is or revamped per the discussion above. jeff >
Re: [PATCH] handle initialized flexible array members in __builtin_object_size [PR92815]
On Thu, 2020-04-23 at 16:05 -0600, Martin Sebor wrote: > On 4/23/20 9:42 AM, Jeff Law wrote: > > On Wed, 2020-04-22 at 15:36 -0600, Martin Sebor via Gcc-patches wrote: > > > When computing the size of an object with a flexible array member > > > the object size pass doesn't consider that the initializer of such > > > an object can result in its size being in excess of the size of > > > the enclosing type. As a result, stores into such objects by > > > string functions causes false positive warnings and can abort > > > at runtime. > > > > > > The warnings are an old regression but as more of them make use > > > of the object size results more of them are affected by the bug. > > > The abort goes back to when support for _FORTIFY_SOURCE was added. > > > > > > The same problem has already been independently fixed in GCC 10 > > > for -Warray-bounds which doesn't use the object size checking pass, > > > but the object size bug still remains. The attached patch corrects > > > it as well. > > > > > > Tested on x86_64-linux. > > Do you need to change guarding condition to use decl_init_size instead of > > DECL_SIZE_UNIT as well? > > > > else if (pt_var > > && DECL_P (pt_var) > > && tree_fits_uhwi_p (DECL_SIZE_UNIT (pt_var)) > > ^^ > > && tree_to_uhwi (DECL_SIZE_UNIT (pt_var)) < offset_limit) > > ^^ > > It doesn't see that changing it is strictly necessary. If the tests > above pass and the result doesn't satisfy the conditions because it's > either null or doesn't fit in UHWI it's handled later by returning > false. With offset_limit set to SIZE_MAX / 2, I don't think the result > can as big as that or bigger: it would imply the whole object, including > its initializer, is bigger than half the address space and GCC rejects > such objects with an error. I've added another test in the patch to > to verify this. > > I do agree it would be better to validate the final result the same > way. That makes the change a little more intrusive to avoid validating > the size multiple times, but I think it also improves the readability > of the code, so the updated patch does that. It passes testing on > x86_64-linux. > > Let me know which one of the two you prefer, or if you'd rather hold > off until stage 1. I think at this point we should defer. jeff >
Re: [PATCH] handle initialized flexible array members in __builtin_object_size [PR92815]
On 4/23/20 9:42 AM, Jeff Law wrote:
On Wed, 2020-04-22 at 15:36 -0600, Martin Sebor via Gcc-patches wrote:
When computing the size of an object with a flexible array member
the object size pass doesn't consider that the initializer of such
an object can result in its size being in excess of the size of
the enclosing type. As a result, stores into such objects by
string functions causes false positive warnings and can abort
at runtime.
The warnings are an old regression but as more of them make use
of the object size results more of them are affected by the bug.
The abort goes back to when support for _FORTIFY_SOURCE was added.
The same problem has already been independently fixed in GCC 10
for -Warray-bounds which doesn't use the object size checking pass,
but the object size bug still remains. The attached patch corrects
it as well.
Tested on x86_64-linux.
Do you need to change guarding condition to use decl_init_size instead of
DECL_SIZE_UNIT as well?
else if (pt_var
&& DECL_P (pt_var)
&& tree_fits_uhwi_p (DECL_SIZE_UNIT (pt_var))
^^
&& tree_to_uhwi (DECL_SIZE_UNIT (pt_var)) < offset_limit)
^^
It doesn't see that changing it is strictly necessary. If the tests
above pass and the result doesn't satisfy the conditions because it's
either null or doesn't fit in UHWI it's handled later by returning
false. With offset_limit set to SIZE_MAX / 2, I don't think the result
can as big as that or bigger: it would imply the whole object, including
its initializer, is bigger than half the address space and GCC rejects
such objects with an error. I've added another test in the patch to
to verify this.
I do agree it would be better to validate the final result the same
way. That makes the change a little more intrusive to avoid validating
the size multiple times, but I think it also improves the readability
of the code, so the updated patch does that. It passes testing on
x86_64-linux.
Let me know which one of the two you prefer, or if you'd rather hold
off until stage 1.
Martin
{
*pdecl = pt_var;
pt_var_size = DECL_SIZE_UNIT (pt_var);
}
Jeff
PR middle-end/92815 - spurious -Wstringop-overflow writing into a flexible array of an extern struct
gcc/ChangeLog:
PR middle-end/92815
* tree-object-size.c (decl_init_size): New function.
(addr_object_size): Call it.
* tree.h (last_field): Declare.
(first_field): Add attribute nonnull.
gcc/testsuite/ChangeLog:
PR middle-end/92815
* gcc.dg/Warray-bounds-56.c: Remove xfails.
* gcc.dg/builtin-object-size-20.c: New test.
* gcc.dg/builtin-object-size-21.c: New test.
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-56.c b/gcc/testsuite/gcc.dg/Warray-bounds-56.c
index 399c9b263b3..04c26a659ad 100644
--- a/gcc/testsuite/gcc.dg/Warray-bounds-56.c
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-56.c
@@ -42,8 +42,8 @@ struct Flex f3 = { 3, { 1, 2, 3 } };
NOIPA void test_strcpy_flexarray (void)
{
- T (S (0), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" { xfail *-*-*} }
- T (S (9), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" { xfail *-*-*} }
+ T (S (0), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" }
+ T (S (9), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" }
T (S (0), f1);
T (S (1), f1);// { dg-warning "\\\[-Warray-bounds" }
diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-20.c b/gcc/testsuite/gcc.dg/builtin-object-size-20.c
new file mode 100644
index 000..47821c06d76
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-20.c
@@ -0,0 +1,315 @@
+/* PR middle-end/92815 - spurious -Wstringop-overflow writing into
+ a flexible array of an extern struct
+ { dg-do compile }
+ { dg-options "-Wall -fdump-tree-optimized" } */
+
+#define ASSERT(expr) ((expr) ? (void)0 : fail (__LINE__))
+#define bos0(expr) __builtin_object_size (expr, 1)
+#define bos1(expr) __builtin_object_size (expr, 1)
+#define bos2(expr) __builtin_object_size (expr, 2)
+#define bos3(expr) __builtin_object_size (expr, 3)
+
+typedef __INT16_TYPE__ int16_t;
+typedef __INT32_TYPE__ int32_t;
+typedef __INT64_TYPE__ int64_t;
+typedef __SIZE_TYPE__ size_t;
+
+
+extern void fail (int);
+
+
+/* Verify sizes of a struct with a flexible array member and no padding. */
+
+struct ACX { char n, a[]; };
+
+struct ACX ac0 = { };
+struct ACX ac1 = { 1, { 1 } };
+struct ACX ac2 = { 2, { 1, 2 } };
+struct ACX ac3 = { 3, { 1, 2, 3 } };
+
+extern struct ACX eacx;
+
+void facx (void)
+{
+ ASSERT (bos0 () == sizeof ac0);
+ ASSERT (bos0 () == 2);
+ ASSERT (bos0 () == 3);
+ ASSERT (bos0 () == 4);
+ ASSERT (bos0 () == (size_t)-1);
+
+ ASSERT (bos1 () == sizeof ac0);
+ ASSERT (bos1 () == 2);
+ ASSERT (bos1 () == 3);
+ ASSERT (bos1 () == 4);
+ ASSERT (bos1 () == (size_t)-1);
+
+ ASSERT (bos2 () == sizeof ac0);
+ ASSERT (bos2 () ==
Re: [PATCH] handle initialized flexible array members in __builtin_object_size [PR92815]
On Wed, 2020-04-22 at 15:36 -0600, Martin Sebor via Gcc-patches wrote:
> When computing the size of an object with a flexible array member
> the object size pass doesn't consider that the initializer of such
> an object can result in its size being in excess of the size of
> the enclosing type. As a result, stores into such objects by
> string functions causes false positive warnings and can abort
> at runtime.
>
> The warnings are an old regression but as more of them make use
> of the object size results more of them are affected by the bug.
> The abort goes back to when support for _FORTIFY_SOURCE was added.
>
> The same problem has already been independently fixed in GCC 10
> for -Warray-bounds which doesn't use the object size checking pass,
> but the object size bug still remains. The attached patch corrects
> it as well.
>
> Tested on x86_64-linux.
Do you need to change guarding condition to use decl_init_size instead of
DECL_SIZE_UNIT as well?
else if (pt_var
&& DECL_P (pt_var)
&& tree_fits_uhwi_p (DECL_SIZE_UNIT (pt_var))
^^
&& tree_to_uhwi (DECL_SIZE_UNIT (pt_var)) < offset_limit)
^^
{
*pdecl = pt_var;
pt_var_size = DECL_SIZE_UNIT (pt_var);
}
Jeff
[PATCH] handle initialized flexible array members in __builtin_object_size [PR92815]
When computing the size of an object with a flexible array member
the object size pass doesn't consider that the initializer of such
an object can result in its size being in excess of the size of
the enclosing type. As a result, stores into such objects by
string functions causes false positive warnings and can abort
at runtime.
The warnings are an old regression but as more of them make use
of the object size results more of them are affected by the bug.
The abort goes back to when support for _FORTIFY_SOURCE was added.
The same problem has already been independently fixed in GCC 10
for -Warray-bounds which doesn't use the object size checking pass,
but the object size bug still remains. The attached patch corrects
it as well.
Tested on x86_64-linux.
Martin
PR middle-end/92815 - spurious -Wstringop-overflow writing into a flexible array of an extern struct
gcc/ChangeLog:
PR middle-end/92815
* tree-object-size.c (decl_init_size): New function.
(addr_object_size): Call it.
* tree.h (last_field): Declare.
(first_field): Add attribute nonnull.
gcc/testsuite/ChangeLog:
PR middle-end/92815
* gcc.dg/Warray-bounds-56.c: Remove xfails.
* gcc.dg/builtin-object-size-20.c: New test.
diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-56.c b/gcc/testsuite/gcc.dg/Warray-bounds-56.c
index 399c9b263b3..04c26a659ad 100644
--- a/gcc/testsuite/gcc.dg/Warray-bounds-56.c
+++ b/gcc/testsuite/gcc.dg/Warray-bounds-56.c
@@ -42,8 +42,8 @@ struct Flex f3 = { 3, { 1, 2, 3 } };
NOIPA void test_strcpy_flexarray (void)
{
- T (S (0), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" { xfail *-*-*} }
- T (S (9), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" { xfail *-*-*} }
+ T (S (0), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" }
+ T (S (9), fx);// { dg-bogus "\\\[-Warray-bounds" "pr92815" }
T (S (0), f1);
T (S (1), f1);// { dg-warning "\\\[-Warray-bounds" }
diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-20.c b/gcc/testsuite/gcc.dg/builtin-object-size-20.c
new file mode 100644
index 000..47821c06d76
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/builtin-object-size-20.c
@@ -0,0 +1,315 @@
+/* PR middle-end/92815 - spurious -Wstringop-overflow writing into
+ a flexible array of an extern struct
+ { dg-do compile }
+ { dg-options "-Wall -fdump-tree-optimized" } */
+
+#define ASSERT(expr) ((expr) ? (void)0 : fail (__LINE__))
+#define bos0(expr) __builtin_object_size (expr, 1)
+#define bos1(expr) __builtin_object_size (expr, 1)
+#define bos2(expr) __builtin_object_size (expr, 2)
+#define bos3(expr) __builtin_object_size (expr, 3)
+
+typedef __INT16_TYPE__ int16_t;
+typedef __INT32_TYPE__ int32_t;
+typedef __INT64_TYPE__ int64_t;
+typedef __SIZE_TYPE__ size_t;
+
+
+extern void fail (int);
+
+
+/* Verify sizes of a struct with a flexible array member and no padding. */
+
+struct ACX { char n, a[]; };
+
+struct ACX ac0 = { };
+struct ACX ac1 = { 1, { 1 } };
+struct ACX ac2 = { 2, { 1, 2 } };
+struct ACX ac3 = { 3, { 1, 2, 3 } };
+
+extern struct ACX eacx;
+
+void facx (void)
+{
+ ASSERT (bos0 () == sizeof ac0);
+ ASSERT (bos0 () == 2);
+ ASSERT (bos0 () == 3);
+ ASSERT (bos0 () == 4);
+ ASSERT (bos0 () == (size_t)-1);
+
+ ASSERT (bos1 () == sizeof ac0);
+ ASSERT (bos1 () == 2);
+ ASSERT (bos1 () == 3);
+ ASSERT (bos1 () == 4);
+ ASSERT (bos1 () == (size_t)-1);
+
+ ASSERT (bos2 () == sizeof ac0);
+ ASSERT (bos2 () == 2);
+ ASSERT (bos2 () == 3);
+ ASSERT (bos2 () == 4);
+ ASSERT (bos2 () == sizeof eacx);
+
+ ASSERT (bos3 () == sizeof ac0);
+ ASSERT (bos3 () == 2);
+ ASSERT (bos3 () == 3);
+ ASSERT (bos3 () == 4);
+ ASSERT (bos3 () == sizeof eacx);
+}
+
+
+
+/* Verify sizes of a struct with a flexible array member and 1 byte
+ of tail padding. */
+
+struct AI16CX { int16_t i; char n, a[]; };
+
+struct AI16CX ai16c0 = { 0 };
+struct AI16CX ai16c1 = { 0, 1, { 1 } };
+struct AI16CX ai16c2 = { 0, 2, { 1, 2 } };
+struct AI16CX ai16c3 = { 0, 3, { 1, 2, 3 } };
+struct AI16CX ai16c4 = { 0, 4, { 1, 2, 3, 4 } };
+struct AI16CX ai16c5 = { 0, 5, { 1, 2, 3, 4, 5 } };
+struct AI16CX ai16c6 = { 0, 6, { 1, 2, 3, 4, 5, 6 } };
+struct AI16CX ai16c7 = { 0, 7, { 1, 2, 3, 4, 5, 6, 7 } };
+struct AI16CX ai16c8 = { 0, 8, { 1, 2, 3, 4, 5, 6, 7, 8 } };
+
+extern struct AI16CX eai16cx;
+
+void fai16cx (void)
+{
+ ASSERT (bos0 () == sizeof ai16c0);
+ ASSERT (bos0 () == sizeof ai16c1);
+ ASSERT (bos0 () == sizeof ai16c2 + 1);
+ ASSERT (bos0 () == sizeof ai16c3 + 2);
+
+ ASSERT (bos0 () == sizeof ai16c4 + 3);
+ ASSERT (bos0 () == sizeof ai16c5 + 4);
+ ASSERT (bos0 () == sizeof ai16c6 + 5);
+ ASSERT (bos0 () == sizeof ai16c6 + 6);
+ ASSERT (bos0 () == sizeof ai16c6 + 7);
+
+ ASSERT (bos0 () == (size_t)-1);
+
+
+ ASSERT (bos1 () == sizeof ai16c0);
+ ASSERT (bos1 () == sizeof ai16c1);
+ ASSERT (bos1 () == sizeof ai16c2 + 1);
+ ASSERT (bos1 () == sizeof ai16c3 + 2);
+
+ ASSERT (bos1 () == sizeof ai16c4 +
