* [PATCH] tree-optimization/pr103961: Never compute offset for -1 size
@ 2022-01-11 13:10 Siddhesh Poyarekar
2022-01-11 13:34 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Siddhesh Poyarekar @ 2022-01-11 13:10 UTC (permalink / raw)
To: gcc-patches; +Cc: jakub
Never try to compute size for offset when the object size is -1, which
is either unknown maximum or uninitialized minimum irrespective of the
osi->pass number.
gcc/ChangeLog:
PR tree-optimization/pr103961
* tree-object-size.c (plus_stmt_object_size): Always avoid
computing offset for -1 size.
gcc/testsuite/ChangeLog:
PR tree-optimization/pr103961
* gcc.dg/pr103961.c: New test case.
Co-authored-by: Jakub Jelinek <jakub@redhat.com>
Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
Tested with i686 build+test, x86_64 bootstrap build+test and ubsan
bootstrap.
gcc/testsuite/gcc.dg/pr103961.c | 30 ++++++++++++++++++++++++++++++
gcc/tree-object-size.c | 11 ++++-------
2 files changed, 34 insertions(+), 7 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/pr103961.c
diff --git a/gcc/testsuite/gcc.dg/pr103961.c b/gcc/testsuite/gcc.dg/pr103961.c
new file mode 100644
index 00000000000..2cd52884e3b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr103961.c
@@ -0,0 +1,30 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+extern void abort ();
+
+extern inline __attribute__ ((__gnu_inline__)) int
+sprintf (char *restrict s, const char *restrict fmt, ...)
+{
+ return __builtin___sprintf_chk (s, 1, __builtin_object_size (s, 1),
+ fmt, __builtin_va_arg_pack ());
+}
+
+void
+cap_to_text (int c)
+{
+ char buf[1572];
+ char *p;
+ int n, t;
+ p = 20 + buf;
+ for (t = 8; t--; )
+ {
+ for (n = 0; n < c; n++)
+ p += sprintf (p, "a,");
+ p--;
+ if (__builtin_object_size (p, 1) == 0)
+ abort ();
+ }
+}
+
+/* { dg-final { scan-assembler-not "abort" } } */
diff --git a/gcc/tree-object-size.c b/gcc/tree-object-size.c
index fbaf57a20f8..f7cc323591c 100644
--- a/gcc/tree-object-size.c
+++ b/gcc/tree-object-size.c
@@ -990,13 +990,10 @@ plus_stmt_object_size (struct object_size_info *osi, tree var, gimple *stmt)
addr_object_size (osi, op0, object_size_type, &bytes, &wholesize);
}
- /* In the first pass, do not compute size for offset if either the
- maximum size is unknown or the minimum size is not initialized yet;
- the latter indicates a dependency loop and will be resolved in
- subsequent passes. We attempt to compute offset for 0 minimum size
- too because a negative offset could be within bounds of WHOLESIZE,
- giving a non-zero result for VAR. */
- if (osi->pass != 0 || !size_unknown_p (bytes, 0))
+ /* size_for_offset doesn't make sense for -1 size, but it does for size 0
+ since the wholesize could be non-zero and a negative offset could give
+ a non-zero size. */
+ if (!size_unknown_p (bytes, 0))
bytes = size_for_offset (bytes, op1, wholesize);
}
else
--
2.33.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tree-optimization/pr103961: Never compute offset for -1 size
2022-01-11 13:10 [PATCH] tree-optimization/pr103961: Never compute offset for -1 size Siddhesh Poyarekar
@ 2022-01-11 13:34 ` Jakub Jelinek
2022-01-11 14:09 ` Siddhesh Poyarekar
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2022-01-11 13:34 UTC (permalink / raw)
To: Siddhesh Poyarekar; +Cc: gcc-patches
On Tue, Jan 11, 2022 at 06:40:44PM +0530, Siddhesh Poyarekar wrote:
> Never try to compute size for offset when the object size is -1, which
> is either unknown maximum or uninitialized minimum irrespective of the
> osi->pass number.
>
> gcc/ChangeLog:
>
> PR tree-optimization/pr103961
> * tree-object-size.c (plus_stmt_object_size): Always avoid
> computing offset for -1 size.
>
> gcc/testsuite/ChangeLog:
>
> PR tree-optimization/pr103961
> * gcc.dg/pr103961.c: New test case.
Ok. Martin's executable testcase would work too but only if
it would be limited to targets with glibc with __sprintf_chk in or
it would need to be linked against -lssp, so I think it is fine as is.
> +void
> +cap_to_text (int c)
> +{
> + char buf[1572];
> + char *p;
> + int n, t;
> + p = 20 + buf;
> + for (t = 8; t--; )
> + {
> + for (n = 0; n < c; n++)
> + p += sprintf (p, "a,");
> + p--;
> + if (__builtin_object_size (p, 1) == 0)
> + abort ();
> + }
Just curious, does your PR77608 patch change this such that __bos (p, 1)
is not ~(size_t)0 but 1572?
We don't know if c isn't 0, so can't count on p += sprintf (p, "a,");
actually incrementing the pointer (and unless we try to understand more
what exactly it is doing we'd need to also assume it could e.g. return -1,
and we aren't too smart about loops anyway, but if the PR77608 patch assumes
that variable bounds aren't out of bounds, then a pointer that started
as 20 + buf can be minimum buf + 0 and maximum buf + 1572.
Note, as __bos is trying to prevent exploiting UB, perhaps assuming the
pointer arithmetics or array refs aren't out of bounds isn't perfect, but
then I'd say a __bos (p, 1) == 1572 means bigger security than __bos (p, 1)
== -1 where we punt and don't check anything.
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] tree-optimization/pr103961: Never compute offset for -1 size
2022-01-11 13:34 ` Jakub Jelinek
@ 2022-01-11 14:09 ` Siddhesh Poyarekar
0 siblings, 0 replies; 3+ messages in thread
From: Siddhesh Poyarekar @ 2022-01-11 14:09 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 11/01/2022 19:04, Jakub Jelinek wrote:
> On Tue, Jan 11, 2022 at 06:40:44PM +0530, Siddhesh Poyarekar wrote:
>> Never try to compute size for offset when the object size is -1, which
>> is either unknown maximum or uninitialized minimum irrespective of the
>> osi->pass number.
>>
>> gcc/ChangeLog:
>>
>> PR tree-optimization/pr103961
>> * tree-object-size.c (plus_stmt_object_size): Always avoid
>> computing offset for -1 size.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR tree-optimization/pr103961
>> * gcc.dg/pr103961.c: New test case.
>
> Ok. Martin's executable testcase would work too but only if
> it would be limited to targets with glibc with __sprintf_chk in or
> it would need to be linked against -lssp, so I think it is fine as is.
Thanks I'll push this then.
>> +void
>> +cap_to_text (int c)
>> +{
>> + char buf[1572];
>> + char *p;
>> + int n, t;
>> + p = 20 + buf;
>> + for (t = 8; t--; )
>> + {
>> + for (n = 0; n < c; n++)
>> + p += sprintf (p, "a,");
>> + p--;
>> + if (__builtin_object_size (p, 1) == 0)
>> + abort ();
>> + }
>
> Just curious, does your PR77608 patch change this such that __bos (p, 1)
> is not ~(size_t)0 but 1572?
> We don't know if c isn't 0, so can't count on p += sprintf (p, "a,");
> actually incrementing the pointer (and unless we try to understand more
> what exactly it is doing we'd need to also assume it could e.g. return -1,
> and we aren't too smart about loops anyway, but if the PR77608 patch assumes
> that variable bounds aren't out of bounds, then a pointer that started
> as 20 + buf can be minimum buf + 0 and maximum buf + 1572.
> Note, as __bos is trying to prevent exploiting UB, perhaps assuming the
> pointer arithmetics or array refs aren't out of bounds isn't perfect, but
> then I'd say a __bos (p, 1) == 1572 means bigger security than __bos (p, 1)
> == -1 where we punt and don't check anything.
That's a good catch; the 77608 patch actually breaks this again, but for
a different reason that the patch itself introduces. The "safe"
assumption of wholesize for p_4 is not safe enough because p_17 (which
is p_4 - 1) is assumed to have an underflow, resulting in a zero size again.
I need to rethink the 77608 fix; it won't always be as simple as
returning the wholesize.
Thanks,
Siddhesh
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-01-11 14:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-11 13:10 [PATCH] tree-optimization/pr103961: Never compute offset for -1 size Siddhesh Poyarekar
2022-01-11 13:34 ` Jakub Jelinek
2022-01-11 14:09 ` Siddhesh Poyarekar
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).