| Message ID | 20220111131044.1947091-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | tree-optimization/pr103961: Never compute offset for -1 size | expand |
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
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
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
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