* [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
@ 2017-11-17 19:48 Martin Sebor
2017-11-18 8:06 ` Jeff Law
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-11-17 19:48 UTC (permalink / raw)
To: Gcc Patch List
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
The attached patch enhances -Wstringop-overflow to detect more
instances of buffer overflow at compile time by handling non-
constant offsets into the destination object that are known to
be in some range. The solution could be improved by handling
even more cases (e.g., anti-ranges or offsets relative to
pointers beyond the beginning of an object) but it's a start.
In addition to bootsrapping/regtesting GCC, also tested with
Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
regressions.
Martin
The top of GDB fails to compile at the moment so the validation
there was incomplete.
[-- Attachment #2: gcc-77608.diff --]
[-- Type: text/x-patch, Size: 7710 bytes --]
PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
gcc/ChangeLog:
PR middle-end/77608
* builtins.c (compute_objsize): Handle non-constant offsets.
gcc/testsuite/ChangeLog:
PR middle-end/77608
* gcc.dg/Wstringop-overflow.c: New test.
Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c (revision 254879)
+++ gcc/builtins.c (working copy)
@@ -3276,11 +3276,6 @@ compute_objsize (tree dest, int ostype)
if (compute_builtin_object_size (dest, ostype, &size))
return build_int_cst (sizetype, size);
- /* Unless computing the largest size (for memcpy and other raw memory
- functions), try to determine the size of the object from its type. */
- if (!ostype)
- return NULL_TREE;
-
if (TREE_CODE (dest) == SSA_NAME)
{
gimple *stmt = SSA_NAME_DEF_STMT (dest);
@@ -3287,13 +3282,47 @@ compute_objsize (tree dest, int ostype)
if (!is_gimple_assign (stmt))
return NULL_TREE;
+ dest = gimple_assign_rhs1 (stmt);
+
tree_code code = gimple_assign_rhs_code (stmt);
- if (code != ADDR_EXPR && code != POINTER_PLUS_EXPR)
+ if (code == POINTER_PLUS_EXPR)
+ {
+ /* compute_builtin_object_size fails for addresses with
+ non-constant offsets. Try to determine the range of
+ such an offset here and use it to adjus the constant
+ size. */
+ tree off = gimple_assign_rhs2 (stmt);
+ if (TREE_CODE (off) == SSA_NAME
+ && INTEGRAL_TYPE_P (TREE_TYPE (off)))
+ {
+ wide_int min, max;
+ enum value_range_type rng = get_range_info (off, &min, &max);
+
+ if (rng == VR_RANGE)
+ {
+ if (tree size = compute_objsize (dest, ostype))
+ {
+ wide_int wisiz = wi::to_wide (size);
+ if (wi::sign_mask (min))
+ /* ignore negative offsets for now */;
+ else if (wi::ltu_p (min, wisiz))
+ return wide_int_to_tree (TREE_TYPE (size),
+ wi::sub (wisiz, min));
+ else
+ return size_zero_node;
+ }
+ }
+ }
+ }
+ else if (code != ADDR_EXPR)
return NULL_TREE;
-
- dest = gimple_assign_rhs1 (stmt);
}
+ /* Unless computing the largest size (for memcpy and other raw memory
+ functions), try to determine the size of the object from its type. */
+ if (!ostype)
+ return NULL_TREE;
+
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
Index: gcc/testsuite/gcc.dg/Wstringop-overflow.c
===================================================================
--- gcc/testsuite/gcc.dg/Wstringop-overflow.c (nonexistent)
+++ gcc/testsuite/gcc.dg/Wstringop-overflow.c (working copy)
@@ -0,0 +1,132 @@
+/* PR middle-end/77608 - missing protection on trivially detectable runtime
+ buffer overflow
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-overflow -ftrack-macro-expansion=0" } */
+
+#define SIZE_MAX __SIZE_MAX__
+#define DIFF_MAX __PTRDIFF_MAX__
+#define DIFF_MIN (-DIFF_MAX - 1)
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+extern char* strcpy (char*, const char*);
+extern char* strncpy (char*, const char*, size_t);
+
+void sink (void*);
+
+extern size_t unsigned_value (void)
+{
+ extern volatile size_t unsigned_value_source;
+ return unsigned_value_source;
+}
+
+size_t unsigned_range (size_t min, size_t max)
+{
+ size_t val = unsigned_value ();
+ return val < min || max < val ? min : val;
+}
+
+#define UR(min, max) unsigned_range (min, max)
+
+
+char a7[7];
+
+struct MemArray { char a9[9]; char a1[1]; };
+
+void test_memcpy_array (const void *s)
+{
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ T (a7 + UR (0, 1), s, 7);
+ T (a7 + UR (0, 7), s, 7);
+ T (a7 + UR (0, 8), s, 7);
+ T (a7 + UR (0, DIFF_MAX), s, 7);
+ T (a7 + UR (0, SIZE_MAX), s, 7);
+
+ T (a7 + UR (1, 2), s, 7); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), s, 7); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ /* This is valid. */
+ char *d = a7 + 7;
+ T (d + UR (-8, -7), s, 7);
+}
+
+/* Verify the absence of warnings for memcpy writing beyond object
+ boundaries. */
+
+void test_memcpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ /* The following are valid. */
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ /* The following are invalid. Unfortunately, there is apparently enough
+ code out there that abuses memcpy to write past the end of one member
+ and into the members that follow so the following are not diagnosed
+ by design. It sure would be nice not to have to cater to hacks like
+ these... */
+ T (p->a9 + UR (1, 2), s, 9);
+ T (p->a9 + UR (1, 2), s, 123);
+}
+
+
+void test_strcpy_array (void)
+{
+#undef T
+#define T(d, s) (strcpy ((d), (s)), sink (d))
+
+ T (a7 + UR (0, 1), "012345");
+ T (a7 + UR (0, 7), "012345");
+ T (a7 + UR (0, 8), "012345");
+ T (a7 + UR (0, DIFF_MAX), "012345");
+ T (a7 + UR (0, SIZE_MAX), "012345");
+
+ T (a7 + UR (1, 2), "012345"); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), "012345"); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ char *d = a7 + 7;
+
+ T (d + UR (-8, -7), "012345");
+}
+
+void test_strncpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (strncpy ((d), (s), (n)), sink (d))
+
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ T (p->a9 + UR (1, 2), s, 9); /* { dg-warning "writing 9 bytes into a region of size 8" } */
+ T (p->a9 + UR (2, 3), s, 9); /* { dg-warning "writing 9 bytes into a region of size 7" } */
+ T (p->a9 + UR (6, 9), s, 9); /* { dg-warning "writing 9 bytes into a region of size 3" } */
+ T (p->a9 + UR (9, 10), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+ T (p->a9 + UR (10, 11), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+
+ T (p->a9 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 1); /* { dg-warning "writing 1 byte into a region of size 0" } */
+ T (p->a9 + UR (DIFF_MAX, SIZE_MAX), s, 3); /* { dg-warning "writing 3 bytes into a region of size 0" } */
+}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-17 19:48 [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608) Martin Sebor
@ 2017-11-18 8:06 ` Jeff Law
2017-11-18 16:59 ` Martin Sebor
2017-11-19 23:47 ` Martin Sebor
0 siblings, 2 replies; 12+ messages in thread
From: Jeff Law @ 2017-11-18 8:06 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 11/17/2017 12:36 PM, Martin Sebor wrote:
> The attached patch enhances -Wstringop-overflow to detect more
> instances of buffer overflow at compile time by handling non-
> constant offsets into the destination object that are known to
> be in some range.  The solution could be improved by handling
> even more cases (e.g., anti-ranges or offsets relative to
> pointers beyond the beginning of an object) but it's a start.
>
> In addition to bootsrapping/regtesting GCC, also tested with
> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
> regressions.
>
> Martin
>
> The top of GDB fails to compile at the moment so the validation
> there was incomplete.
>
> gcc-77608.diff
>
>
> PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
>
> gcc/ChangeLog:
>
> PR middle-end/77608
> * builtins.c (compute_objsize): Handle non-constant offsets.
>
> gcc/testsuite/ChangeLog:
>
> PR middle-end/77608
> * gcc.dg/Wstringop-overflow.c: New test.
The recursive call into compute_objsize passing in the ostype avoids
having to think about the whole object vs nearest containing object
issues. Right?
What's left to worry about is maximum or minimum remaining bytes in the
object. At least that's my understanding of how ostype works here.
So we get the amount remaining, ignoring the variable offset, from the
recursive call (SIZE). The space left after we account for the variable
offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
return SIZE-MIN (which you do) and for type 2/3 you have to return
SIZE-MAX which I think you get wrong (and you have to account for the
possibility that MAX or MIN is greater than SIZE and thus there's
nothing left).
The other concern here is precision of the answer and how it's going to
be used by callers. But after a lot of thought I think we're ok on this
front based on how b_o_s is supposed to handle cases where it's given a
pointer to multiple objects which all have known, but possibly different
sizes.
> + /* compute_builtin_object_size fails for addresses with
> + non-constant offsets. Try to determine the range of
> + such an offset here and use it to adjus the constant
> + size. */
> + tree off = gimple_assign_rhs2 (stmt);
> + if (TREE_CODE (off) == SSA_NAME
> + && INTEGRAL_TYPE_P (TREE_TYPE (off)))
> + {
> + wide_int min, max;
> + enum value_range_type rng = get_range_info (off, &min, &max);
> +
> + if (rng == VR_RANGE)
> + {
> + if (tree size = compute_objsize (dest, ostype))
> + {
> + wide_int wisiz = wi::to_wide (size);
> + if (wi::sign_mask (min))
> + /* ignore negative offsets for now */;
Put the comment before the condition. like this
/* Ignore negative offsets for now. */
if (wi::sign_mask (min))
;
That makes the empty if clause clearer to spot. You could even argue
that instead of an empty if clause it should be:
/* Ignore negative offsets for now. */
if (wi::sign_mask (min)
return NULL_TREE;
We're giving up at this point anyway, so why make the reader follow
further control flow to see that's what's going to ultimately happen.
> + else if (wi::ltu_p (min, wisiz))
> + return wide_int_to_tree (TREE_TYPE (size),
> + wi::sub (wisiz, min));
So for type 0,1 (max space available). So I think the condition is
else if ((ostype & 2) == 0
&& wi::ltu_p (min, wisize))
return SIZE-MIN;
Then you'll need
else if ((ostype & 2) == 2
&& (wi::ltu_p (max, wisize)
return SIZE-MAX;
else
return size_zero_node;
I'd like Jakub or Richi to chime in here to check my analysis. It's
getting late :-)
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-18 8:06 ` Jeff Law
@ 2017-11-18 16:59 ` Martin Sebor
2017-11-19 22:36 ` Jeff Law
2017-11-19 23:47 ` Martin Sebor
1 sibling, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-11-18 16:59 UTC (permalink / raw)
To: Jeff Law, Gcc Patch List; +Cc: Jakub Jelinek
On 11/18/2017 12:53 AM, Jeff Law wrote:
> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>> The attached patch enhances -Wstringop-overflow to detect more
>> instances of buffer overflow at compile time by handling non-
>> constant offsets into the destination object that are known to
>> be in some range. The solution could be improved by handling
>> even more cases (e.g., anti-ranges or offsets relative to
>> pointers beyond the beginning of an object) but it's a start.
I should have made it even more clear that this change affects
just warnings, not the runtime protection with _FORTIFY_SOURCE.
The latter is driven by the __builtin_object_size intrinsic and
by the compute_builtin_object_size GCC function.
This patch does not change what __builtin_object_size returns.
>> In addition to bootsrapping/regtesting GCC, also tested with
>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>> regressions.
>>
>> Martin
>>
>> The top of GDB fails to compile at the moment so the validation
>> there was incomplete.
>>
>> gcc-77608.diff
>>
>>
>> PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
>>
>> gcc/ChangeLog:
>>
>> PR middle-end/77608
>> * builtins.c (compute_objsize): Handle non-constant offsets.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR middle-end/77608
>> * gcc.dg/Wstringop-overflow.c: New test.
> The recursive call into compute_objsize passing in the ostype avoids
> having to think about the whole object vs nearest containing object
> issues. Right?
Yes, something like that.
> What's left to worry about is maximum or minimum remaining bytes in the
> object. At least that's my understanding of how ostype works here.
-Wstringop-overflow (i.e., warnings only) always uses the largest
object size for memxxx functions (type 0) and the smallest (type
2) for string functions by default. This can be changed but I
suspect no one does because (IME) few users understand what
the other modes mean or what good using them vs the default might
do. It was probably a mistake to expose the mode via the option.
I'll address the remaining comments separately.
Martin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-18 16:59 ` Martin Sebor
@ 2017-11-19 22:36 ` Jeff Law
0 siblings, 0 replies; 12+ messages in thread
From: Jeff Law @ 2017-11-19 22:36 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 11/18/2017 09:47 AM, Martin Sebor wrote:
> On 11/18/2017 12:53 AM, Jeff Law wrote:
>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>> The attached patch enhances -Wstringop-overflow to detect more
>>> instances of buffer overflow at compile time by handling non-
>>> constant offsets into the destination object that are known to
>>> be in some range. The solution could be improved by handling
>>> even more cases (e.g., anti-ranges or offsets relative to
>>> pointers beyond the beginning of an object) but it's a start.
>
> I should have made it even more clear that this change affects
> just warnings, not the runtime protection with _FORTIFY_SOURCE.
> The latter is driven by the __builtin_object_size intrinsic and
> by the compute_builtin_object_size GCC function.
>
> This patch does not change what __builtin_object_size returns.
Understood. But I think that the documented behavior of b_o_s directly
impacts how this routine is supposed to work.
>>
> -Wstringop-overflow (i.e., warnings only) always uses the largest
> object size for memxxx functions (type 0) and the smallest (type
> 2) for string functions by default. This can be changed but I
> suspect no one does because (IME) few users understand what
> the other modes mean or what good using them vs the default might
> do. It was probably a mistake to expose the mode via the option.
Then maybe only do the deeper analysis for max mode only? As it stands
if someone were to call it in a min mode I think they're going to be
rather surprised by the answer.
jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-18 8:06 ` Jeff Law
2017-11-18 16:59 ` Martin Sebor
@ 2017-11-19 23:47 ` Martin Sebor
2017-11-21 17:01 ` Jeff Law
1 sibling, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-11-19 23:47 UTC (permalink / raw)
To: Jeff Law, Gcc Patch List; +Cc: Jakub Jelinek
On 11/18/2017 12:53 AM, Jeff Law wrote:
> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>> The attached patch enhances -Wstringop-overflow to detect more
>> instances of buffer overflow at compile time by handling non-
>> constant offsets into the destination object that are known to
>> be in some range. The solution could be improved by handling
>> even more cases (e.g., anti-ranges or offsets relative to
>> pointers beyond the beginning of an object) but it's a start.
>>
>> In addition to bootsrapping/regtesting GCC, also tested with
>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>> regressions.
>>
>> Martin
>>
>> The top of GDB fails to compile at the moment so the validation
>> there was incomplete.
>>
>> gcc-77608.diff
>>
>>
>> PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
>>
>> gcc/ChangeLog:
>>
>> PR middle-end/77608
>> * builtins.c (compute_objsize): Handle non-constant offsets.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR middle-end/77608
>> * gcc.dg/Wstringop-overflow.c: New test.
> The recursive call into compute_objsize passing in the ostype avoids
> having to think about the whole object vs nearest containing object
> issues. Right?
>
> What's left to worry about is maximum or minimum remaining bytes in the
> object. At least that's my understanding of how ostype works here.
>
> So we get the amount remaining, ignoring the variable offset, from the
> recursive call (SIZE). The space left after we account for the variable
> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
> return SIZE-MIN (which you do) and for type 2/3 you have to return
> SIZE-MAX which I think you get wrong (and you have to account for the
> possibility that MAX or MIN is greater than SIZE and thus there's
> nothing left).
Subtracting the upper bound of the offset from the size instead
of the lower bound when the caller is asking for the minimum
object size would make the result essentially meaningless in
common cases where the offset is smaller than size_t, as in:
char a[7];
void f (const char *s, unsigned i)
{
__builtin_strcpy (a + i, s);
}
Here, i's range is [0, UINT_MAX].
IMO, it's only useful to use the lower bound here, otherwise
the result would only rarely be non-zero.
This is also what other warnings that deal with ranges do. For
-Warray-bounds only considers the lower bound (unless it's negative)
when deciding whether or not to warn for
int g (unsigned i)
{
return a[i];
}
It would be too noisy to be of practical use otherwise (even at
level 2).
Martin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-19 23:47 ` Martin Sebor
@ 2017-11-21 17:01 ` Jeff Law
2017-11-21 19:53 ` Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2017-11-21 17:01 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 11/19/2017 04:28 PM, Martin Sebor wrote:
> On 11/18/2017 12:53 AM, Jeff Law wrote:
>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>> The attached patch enhances -Wstringop-overflow to detect more
>>> instances of buffer overflow at compile time by handling non-
>>> constant offsets into the destination object that are known to
>>> be in some range. The solution could be improved by handling
>>> even more cases (e.g., anti-ranges or offsets relative to
>>> pointers beyond the beginning of an object) but it's a start.
>>>
>>> In addition to bootsrapping/regtesting GCC, also tested with
>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>> regressions.
>>>
>>> Martin
>>>
>>> The top of GDB fails to compile at the moment so the validation
>>> there was incomplete.
>>>
>>> gcc-77608.diff
>>>
>>>
>>> PR middle-end/77608 - missing protection on trivially detectable
>>> runtime buffer overflow
>>>
>>> gcc/ChangeLog:
>>>
>>> Â Â Â Â PR middle-end/77608
>>> Â Â Â Â * builtins.c (compute_objsize): Handle non-constant offsets.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> Â Â Â Â PR middle-end/77608
>>> Â Â Â Â * gcc.dg/Wstringop-overflow.c: New test.
>> The recursive call into compute_objsize passing in the ostype avoids
>> having to think about the whole object vs nearest containing object
>> issues. Right?
>>
>> What's left to worry about is maximum or minimum remaining bytes in the
>> object. At least that's my understanding of how ostype works here.
>>
>> So we get the amount remaining, ignoring the variable offset, from the
>> recursive call (SIZE). The space left after we account for the variable
>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>> SIZE-MAX which I think you get wrong (and you have to account for the
>> possibility that MAX or MIN is greater than SIZE and thus there's
>> nothing left).
>
> Subtracting the upper bound of the offset from the size instead
> of the lower bound when the caller is asking for the minimum
> object size would make the result essentially meaningless in
> common cases where the offset is smaller than size_t, as in:
>
> Â char a[7];
>
> Â void f (const char *s, unsigned i)
> Â {
> Â Â Â __builtin_strcpy (a + i, s);
> Â }
>
> Here, i's range is [0, UINT_MAX].
>
> IMO, it's only useful to use the lower bound here, otherwise
> the result would only rarely be non-zero.
But when we're asking for the minimum left, aren't we essentially asking
for "how much space am I absolutely sure I can write"? And if that is
the question, then the only conservatively correct answer is to subtract
the high bound.
>
> This is also what other warnings that deal with ranges do. For
> -Warray-bounds only considers the lower bound (unless it's negative)
> when deciding whether or not to warn for
>
> Â int g (unsigned i)
> Â {
> Â Â Â return a[i];
> Â }>
> It would be too noisy to be of practical use otherwise (even at
> level 2).
Which argues that:
1. Our ranges suck
2. We're currently avoiding dealing with that by giving answers that are
not conservatively correct.
Right?
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-21 17:01 ` Jeff Law
@ 2017-11-21 19:53 ` Martin Sebor
2017-11-23 0:24 ` Jeff Law
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-11-21 19:53 UTC (permalink / raw)
To: Jeff Law, Gcc Patch List; +Cc: Jakub Jelinek
On 11/21/2017 09:55 AM, Jeff Law wrote:
> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>>> The attached patch enhances -Wstringop-overflow to detect more
>>>> instances of buffer overflow at compile time by handling non-
>>>> constant offsets into the destination object that are known to
>>>> be in some range. The solution could be improved by handling
>>>> even more cases (e.g., anti-ranges or offsets relative to
>>>> pointers beyond the beginning of an object) but it's a start.
>>>>
>>>> In addition to bootsrapping/regtesting GCC, also tested with
>>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>>> regressions.
>>>>
>>>> Martin
>>>>
>>>> The top of GDB fails to compile at the moment so the validation
>>>> there was incomplete.
>>>>
>>>> gcc-77608.diff
>>>>
>>>>
>>>> PR middle-end/77608 - missing protection on trivially detectable
>>>> runtime buffer overflow
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> PR middle-end/77608
>>>> * builtins.c (compute_objsize): Handle non-constant offsets.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>
>>>> PR middle-end/77608
>>>> * gcc.dg/Wstringop-overflow.c: New test.
>>> The recursive call into compute_objsize passing in the ostype avoids
>>> having to think about the whole object vs nearest containing object
>>> issues. Right?
>>>
>>> What's left to worry about is maximum or minimum remaining bytes in the
>>> object. At least that's my understanding of how ostype works here.
>>>
>>> So we get the amount remaining, ignoring the variable offset, from the
>>> recursive call (SIZE). The space left after we account for the variable
>>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>>> SIZE-MAX which I think you get wrong (and you have to account for the
>>> possibility that MAX or MIN is greater than SIZE and thus there's
>>> nothing left).
>>
>> Subtracting the upper bound of the offset from the size instead
>> of the lower bound when the caller is asking for the minimum
>> object size would make the result essentially meaningless in
>> common cases where the offset is smaller than size_t, as in:
>>
>> char a[7];
>>
>> void f (const char *s, unsigned i)
>> {
>> __builtin_strcpy (a + i, s);
>> }
>>
>> Here, i's range is [0, UINT_MAX].
>>
>> IMO, it's only useful to use the lower bound here, otherwise
>> the result would only rarely be non-zero.
> But when we're asking for the minimum left, aren't we essentially asking
> for "how much space am I absolutely sure I can write"? And if that is
> the question, then the only conservatively correct answer is to subtract
> the high bound.
I suppose you could look at it that way but IME with this work
(now, and also last year when I submitted a patch actually
changing the built-in), using the upper bound is just not that
useful because it's too often way too big. There's no way to
distinguish an out-of-range upper bound that's the result of
an inadequate attempt to constrain a value from an out-of-range
upper bound that is sufficiently constrained but in a way GCC
doesn't see.
There are no clients of this API that would be affected by
the decision one way or the other (unless the user specifies
a -Wstringop-overflow= argument greater than the default 2)
so I don't think what we do now matters much, if at all.
Perhaps in the future with some of the range improvements
that you, Aldy and Andrew have been working on.
That said, if it helps us move forward with this enhancement
I'll use the upper bound -- let me know. In the future, when
it is actually used, we'll adjust it as necessary.
>> This is also what other warnings that deal with ranges do. For
>> -Warray-bounds only considers the lower bound (unless it's negative)
>> when deciding whether or not to warn for
>>
>> int g (unsigned i)
>> {
>> return a[i];
>> }>
>> It would be too noisy to be of practical use otherwise (even at
>> level 2).
> Which argues that:
>
> 1. Our ranges suck
> 2. We're currently avoiding dealing with that by giving answers that are
> not conservatively correct.
>
> Right?
I don't feel quite as strongly. Modulo the pesky bugs we get
every so often for some of the corner cases or known limitations
they seem actually reasonably accurate in most cases, and the
warnings, for the most part, strike a reasonable balance between
false positives and negatives. But I certainly agree that there
is room to improve and I look forward to taking some of the range
enhancement out for a spin.
Martin
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-21 19:53 ` Martin Sebor
@ 2017-11-23 0:24 ` Jeff Law
2017-11-30 21:47 ` Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2017-11-23 0:24 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 11/21/2017 12:07 PM, Martin Sebor wrote:
> On 11/21/2017 09:55 AM, Jeff Law wrote:
>> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>>>> The attached patch enhances -Wstringop-overflow to detect more
>>>>> instances of buffer overflow at compile time by handling non-
>>>>> constant offsets into the destination object that are known to
>>>>> be in some range. The solution could be improved by handling
>>>>> even more cases (e.g., anti-ranges or offsets relative to
>>>>> pointers beyond the beginning of an object) but it's a start.
>>>>>
>>>>> In addition to bootsrapping/regtesting GCC, also tested with
>>>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>>>> regressions.
>>>>>
>>>>> Martin
>>>>>
>>>>> The top of GDB fails to compile at the moment so the validation
>>>>> there was incomplete.
>>>>>
>>>>> gcc-77608.diff
>>>>>
>>>>>
>>>>> PR middle-end/77608 - missing protection on trivially detectable
>>>>> runtime buffer overflow
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> Â Â Â PR middle-end/77608
>>>>> Â Â Â * builtins.c (compute_objsize): Handle non-constant offsets.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> Â Â Â PR middle-end/77608
>>>>> Â Â Â * gcc.dg/Wstringop-overflow.c: New test.
>>>> The recursive call into compute_objsize passing in the ostype avoids
>>>> having to think about the whole object vs nearest containing object
>>>> issues. Right?
>>>>
>>>> What's left to worry about is maximum or minimum remaining bytes in the
>>>> object. At least that's my understanding of how ostype works here.
>>>>
>>>> So we get the amount remaining, ignoring the variable offset, from the
>>>> recursive call (SIZE). The space left after we account for the
>>>> variable
>>>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>>>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>>>> SIZE-MAX which I think you get wrong (and you have to account for the
>>>> possibility that MAX or MIN is greater than SIZE and thus there's
>>>> nothing left).
>>>
>>> Subtracting the upper bound of the offset from the size instead
>>> of the lower bound when the caller is asking for the minimum
>>> object size would make the result essentially meaningless in
>>> common cases where the offset is smaller than size_t, as in:
>>>
>>> Â char a[7];
>>>
>>> Â void f (const char *s, unsigned i)
>>> Â {
>>> Â Â Â __builtin_strcpy (a + i, s);
>>> Â }
>>>
>>> Here, i's range is [0, UINT_MAX].
>>>
>>> IMO, it's only useful to use the lower bound here, otherwise
>>> the result would only rarely be non-zero.
>> But when we're asking for the minimum left, aren't we essentially asking
>> for "how much space am I absolutely sure I can write"? And if that is
>> the question, then the only conservatively correct answer is to subtract
>> the high bound.
>
> I suppose you could look at it that way but IME with this work
> (now, and also last year when I submitted a patch actually
> changing the built-in), using the upper bound is just not that
> useful because it's too often way too big. There's no way to
> distinguish an out-of-range upper bound that's the result of
> an inadequate attempt to constrain a value from an out-of-range
> upper bound that is sufficiently constrained but in a way GCC
> doesn't see.
Understood.
So while it's reasonable to not warn in those cases where we just have
crap range information (that's always going to be the case for some code
regardless of how good my work or Andrew/Aldy's work is), we have to be
very careful and make sure that nobody acts on this information for
optimization purposes because what we're returning is not conservatively
correct.
>
> There are no clients of this API that would be affected by
> the decision one way or the other (unless the user specifies
> a -Wstringop-overflow= argument greater than the default 2)
> so I don't think what we do now matters much, if at all.
Right, but what's to stop someone without knowledge of the
implementation and its quirk of not returning the conservatively safe
result from using the results in other ways.
What would the impact be of simply not supporting those queries,
essentially returning "I don't know" rather than returning something
that isn't conservatively correct?
If we have to support both queries and we're going to return something
that is not conservatively correct, then it really needs to be
documented to avoid folks using the code in ways that are not safe.
> Perhaps in the future with some of the range improvements
> that you, Aldy and Andrew have been working on.
I think they'll help, but there's always going to be codes that can't be
analyzed, it's just inherent in the languages we use.
>
> That said, if it helps us move forward with this enhancement
> I'll use the upper bound -- let me know. In the future, when
> it is actually used, we'll adjust it as necessary.
Understood. Let's answer the questions above first. Namely, do we have
to support the other mode? I thought I saw something that indicated it
wasn't ever used that way by the current clients. So if we can just
return "don't know" for those ostype queries that's a reasonable place
to go. Else we should look at a clear comment about the limitations of
the code to help prevent (ab)use of the analysis in contexts were it's
not appropriate.
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-23 0:24 ` Jeff Law
@ 2017-11-30 21:47 ` Martin Sebor
2017-12-01 8:27 ` Jeff Law
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-11-30 21:47 UTC (permalink / raw)
To: Jeff Law, Gcc Patch List; +Cc: Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 7158 bytes --]
On 11/22/2017 05:03 PM, Jeff Law wrote:
> On 11/21/2017 12:07 PM, Martin Sebor wrote:
>> On 11/21/2017 09:55 AM, Jeff Law wrote:
>>> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>>>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>>>>> The attached patch enhances -Wstringop-overflow to detect more
>>>>>> instances of buffer overflow at compile time by handling non-
>>>>>> constant offsets into the destination object that are known to
>>>>>> be in some range. The solution could be improved by handling
>>>>>> even more cases (e.g., anti-ranges or offsets relative to
>>>>>> pointers beyond the beginning of an object) but it's a start.
>>>>>>
>>>>>> In addition to bootsrapping/regtesting GCC, also tested with
>>>>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>>>>> regressions.
>>>>>>
>>>>>> Martin
>>>>>>
>>>>>> The top of GDB fails to compile at the moment so the validation
>>>>>> there was incomplete.
>>>>>>
>>>>>> gcc-77608.diff
>>>>>>
>>>>>>
>>>>>> PR middle-end/77608 - missing protection on trivially detectable
>>>>>> runtime buffer overflow
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> PR middle-end/77608
>>>>>> * builtins.c (compute_objsize): Handle non-constant offsets.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> PR middle-end/77608
>>>>>> * gcc.dg/Wstringop-overflow.c: New test.
>>>>> The recursive call into compute_objsize passing in the ostype avoids
>>>>> having to think about the whole object vs nearest containing object
>>>>> issues. Right?
>>>>>
>>>>> What's left to worry about is maximum or minimum remaining bytes in the
>>>>> object. At least that's my understanding of how ostype works here.
>>>>>
>>>>> So we get the amount remaining, ignoring the variable offset, from the
>>>>> recursive call (SIZE). The space left after we account for the
>>>>> variable
>>>>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>>>>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>>>>> SIZE-MAX which I think you get wrong (and you have to account for the
>>>>> possibility that MAX or MIN is greater than SIZE and thus there's
>>>>> nothing left).
>>>>
>>>> Subtracting the upper bound of the offset from the size instead
>>>> of the lower bound when the caller is asking for the minimum
>>>> object size would make the result essentially meaningless in
>>>> common cases where the offset is smaller than size_t, as in:
>>>>
>>>> char a[7];
>>>>
>>>> void f (const char *s, unsigned i)
>>>> {
>>>> __builtin_strcpy (a + i, s);
>>>> }
>>>>
>>>> Here, i's range is [0, UINT_MAX].
>>>>
>>>> IMO, it's only useful to use the lower bound here, otherwise
>>>> the result would only rarely be non-zero.
>>> But when we're asking for the minimum left, aren't we essentially asking
>>> for "how much space am I absolutely sure I can write"? And if that is
>>> the question, then the only conservatively correct answer is to subtract
>>> the high bound.
>>
>> I suppose you could look at it that way but IME with this work
>> (now, and also last year when I submitted a patch actually
>> changing the built-in), using the upper bound is just not that
>> useful because it's too often way too big. There's no way to
>> distinguish an out-of-range upper bound that's the result of
>> an inadequate attempt to constrain a value from an out-of-range
>> upper bound that is sufficiently constrained but in a way GCC
>> doesn't see.
> Understood.
>
> So while it's reasonable to not warn in those cases where we just have
> crap range information (that's always going to be the case for some code
> regardless of how good my work or Andrew/Aldy's work is), we have to be
> very careful and make sure that nobody acts on this information for
> optimization purposes because what we're returning is not conservatively
> correct.
>
>
>>
>> There are no clients of this API that would be affected by
>> the decision one way or the other (unless the user specifies
>> a -Wstringop-overflow= argument greater than the default 2)
>> so I don't think what we do now matters much, if at all.
> Right, but what's to stop someone without knowledge of the
> implementation and its quirk of not returning the conservatively safe
> result from using the results in other ways.
Presumably they would find out by testing their code. But this
is a hypothetical scenario. I added the function for warnings.
I wasn't expecting it to be used for optimization, no such uses
have emerged, and I don't have the impression that anyone is
contemplating adding them (certainly not in stage 3). If you
think the function could be useful for optimization then we
should certainly consider changing it as we gain experience
with it under those conditions.
> What would the impact be of simply not supporting those queries,
> essentially returning "I don't know" rather than returning something
> that isn't conservatively correct?
Except for false negatives with -Wstringop-overflow=3 and =4
(i.e., Object Size type 2 and 3) I don't think there would be
any impact. As I said, the function isn't used for optimization
and I don't think the option is used in these modes either, so
in my mind it matters very little. I don't even think there
are any tests for it in these modes, either.
>
> If we have to support both queries and we're going to return something
> that is not conservatively correct, then it really needs to be
> documented to avoid folks using the code in ways that are not safe.
>
>
>
>> Perhaps in the future with some of the range improvements
>> that you, Aldy and Andrew have been working on.
> I think they'll help, but there's always going to be codes that can't be
> analyzed, it's just inherent in the languages we use.
>
>>
>> That said, if it helps us move forward with this enhancement
>> I'll use the upper bound -- let me know. In the future, when
>> it is actually used, we'll adjust it as necessary.
> Understood. Let's answer the questions above first. Namely, do we have
> to support the other mode? I thought I saw something that indicated it
> wasn't ever used that way by the current clients. So if we can just
> return "don't know" for those ostype queries that's a reasonable place
> to go. Else we should look at a clear comment about the limitations of
> the code to help prevent (ab)use of the analysis in contexts were it's
> not appropriate.
The -Wstringop-overflow= option supports all four object size
types. I suspect only types 0 and 1 are used but I also don't
see a pressing need to disable or compromise the others. If
you do I'm fine with changing the option either to only support
modes 0 and 1, or to remove the argument altogether (and using
the same default as today, i.e., mode 0 for memxxx functions
and mode 1 for strxxx functions). But I see no benefit in
changing the function to give up in modes 2 and 3 and have it
cause false negatives. It simply isn't necessary today.
For now, I've added comments to clarify the return value and
the purpose of the function. Let me know if that's good enough
or if you want me to make any other changes.
Martin
[-- Attachment #2: gcc-77608.diff --]
[-- Type: text/x-patch, Size: 8524 bytes --]
PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
gcc/ChangeLog:
PR middle-end/77608
* builtins.c (compute_objsize): Handle non-constant offsets.
gcc/testsuite/ChangeLog:
PR middle-end/77608
* gcc.dg/Wstringop-overflow.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 650de0d..e35c514 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3262,8 +3262,12 @@ check_sizes (int opt, tree exp, tree size, tree maxlen, tree src, tree objsize)
/* Helper to compute the size of the object referenced by the DEST
expression which must have pointer type, using Object Size type
OSTYPE (only the least significant 2 bits are used). Return
- the size of the object if successful or NULL when the size cannot
- be determined. */
+ an estimate of the size of the object if successful or NULL when
+ the size cannot be determined. When the referenced object involves
+ a non-constant offset in some range the returned value represents
+ the largest size given the smallest non-negative offset in the
+ range. The function is intended for diagnostics and is not
+ suitable for optimization. */
tree
compute_objsize (tree dest, int ostype)
@@ -3276,24 +3280,57 @@ compute_objsize (tree dest, int ostype)
if (compute_builtin_object_size (dest, ostype, &size))
return build_int_cst (sizetype, size);
- /* Unless computing the largest size (for memcpy and other raw memory
- functions), try to determine the size of the object from its type. */
- if (!ostype)
- return NULL_TREE;
-
if (TREE_CODE (dest) == SSA_NAME)
{
gimple *stmt = SSA_NAME_DEF_STMT (dest);
if (!is_gimple_assign (stmt))
return NULL_TREE;
+ dest = gimple_assign_rhs1 (stmt);
+
tree_code code = gimple_assign_rhs_code (stmt);
- if (code != ADDR_EXPR && code != POINTER_PLUS_EXPR)
- return NULL_TREE;
+ if (code == POINTER_PLUS_EXPR)
+ {
+ /* compute_builtin_object_size fails for addresses with
+ non-constant offsets. Try to determine the range of
+ such an offset here and use it to adjus the constant
+ size. */
+ tree off = gimple_assign_rhs2 (stmt);
+ if (TREE_CODE (off) == SSA_NAME
+ && INTEGRAL_TYPE_P (TREE_TYPE (off)))
+ {
+ wide_int min, max;
+ enum value_range_type rng = get_range_info (off, &min, &max);
- dest = gimple_assign_rhs1 (stmt);
+ if (rng == VR_RANGE)
+ {
+ if (tree size = compute_objsize (dest, ostype))
+ {
+ wide_int wisiz = wi::to_wide (size);
+
+ /* Ignore negative offsets for now. For others,
+ use the lower bound as the most optimistic
+ estimate of the (remaining)size. */
+ if (wi::sign_mask (min))
+ ;
+ else if (wi::ltu_p (min, wisiz))
+ return wide_int_to_tree (TREE_TYPE (size),
+ wi::sub (wisiz, min));
+ else
+ return size_zero_node;
+ }
+ }
+ }
+ }
+ else if (code != ADDR_EXPR)
+ return NULL_TREE;
}
+ /* Unless computing the largest size (for memcpy and other raw memory
+ functions), try to determine the size of the object from its type. */
+ if (!ostype)
+ return NULL_TREE;
+
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow.c b/gcc/testsuite/gcc.dg/Wstringop-overflow.c
new file mode 100644
index 0000000..b5bd40e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow.c
@@ -0,0 +1,132 @@
+/* PR middle-end/77608 - missing protection on trivially detectable runtime
+ buffer overflow
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-overflow -ftrack-macro-expansion=0" } */
+
+#define SIZE_MAX __SIZE_MAX__
+#define DIFF_MAX __PTRDIFF_MAX__
+#define DIFF_MIN (-DIFF_MAX - 1)
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+extern char* strcpy (char*, const char*);
+extern char* strncpy (char*, const char*, size_t);
+
+void sink (void*);
+
+extern size_t unsigned_value (void)
+{
+ extern volatile size_t unsigned_value_source;
+ return unsigned_value_source;
+}
+
+size_t unsigned_range (size_t min, size_t max)
+{
+ size_t val = unsigned_value ();
+ return val < min || max < val ? min : val;
+}
+
+#define UR(min, max) unsigned_range (min, max)
+
+
+char a7[7];
+
+struct MemArray { char a9[9]; char a1[1]; };
+
+void test_memcpy_array (const void *s)
+{
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ T (a7 + UR (0, 1), s, 7);
+ T (a7 + UR (0, 7), s, 7);
+ T (a7 + UR (0, 8), s, 7);
+ T (a7 + UR (0, DIFF_MAX), s, 7);
+ T (a7 + UR (0, SIZE_MAX), s, 7);
+
+ T (a7 + UR (1, 2), s, 7); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), s, 7); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ /* This is valid. */
+ char *d = a7 + 7;
+ T (d + UR (-8, -7), s, 7);
+}
+
+/* Verify the absence of warnings for memcpy writing beyond object
+ boundaries. */
+
+void test_memcpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ /* The following are valid. */
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ /* The following are invalid. Unfortunately, there is apparently enough
+ code out there that abuses memcpy to write past the end of one member
+ and into the members that follow so the following are not diagnosed
+ by design. It sure would be nice not to have to cater to hacks like
+ these... */
+ T (p->a9 + UR (1, 2), s, 9);
+ T (p->a9 + UR (1, 2), s, 123);
+}
+
+
+void test_strcpy_array (void)
+{
+#undef T
+#define T(d, s) (strcpy ((d), (s)), sink (d))
+
+ T (a7 + UR (0, 1), "012345");
+ T (a7 + UR (0, 7), "012345");
+ T (a7 + UR (0, 8), "012345");
+ T (a7 + UR (0, DIFF_MAX), "012345");
+ T (a7 + UR (0, SIZE_MAX), "012345");
+
+ T (a7 + UR (1, 2), "012345"); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), "012345"); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ char *d = a7 + 7;
+
+ T (d + UR (-8, -7), "012345");
+}
+
+void test_strncpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (strncpy ((d), (s), (n)), sink (d))
+
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ T (p->a9 + UR (1, 2), s, 9); /* { dg-warning "writing 9 bytes into a region of size 8" } */
+ T (p->a9 + UR (2, 3), s, 9); /* { dg-warning "writing 9 bytes into a region of size 7" } */
+ T (p->a9 + UR (6, 9), s, 9); /* { dg-warning "writing 9 bytes into a region of size 3" } */
+ T (p->a9 + UR (9, 10), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+ T (p->a9 + UR (10, 11), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+
+ T (p->a9 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 1); /* { dg-warning "writing 1 byte into a region of size 0" } */
+ T (p->a9 + UR (DIFF_MAX, SIZE_MAX), s, 3); /* { dg-warning "writing 3 bytes into a region of size 0" } */
+}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-11-30 21:47 ` Martin Sebor
@ 2017-12-01 8:27 ` Jeff Law
2017-12-01 18:06 ` Martin Sebor
0 siblings, 1 reply; 12+ messages in thread
From: Jeff Law @ 2017-12-01 8:27 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 11/30/2017 01:30 PM, Martin Sebor wrote:
> On 11/22/2017 05:03 PM, Jeff Law wrote:
>> On 11/21/2017 12:07 PM, Martin Sebor wrote:
>>> On 11/21/2017 09:55 AM, Jeff Law wrote:
>>>> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>>>>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>>>>>> The attached patch enhances -Wstringop-overflow to detect more
>>>>>>> instances of buffer overflow at compile time by handling non-
>>>>>>> constant offsets into the destination object that are known to
>>>>>>> be in some range. The solution could be improved by handling
>>>>>>> even more cases (e.g., anti-ranges or offsets relative to
>>>>>>> pointers beyond the beginning of an object) but it's a start.
>>>>>>>
>>>>>>> In addition to bootsrapping/regtesting GCC, also tested with
>>>>>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>>>>>> regressions.
>>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>> The top of GDB fails to compile at the moment so the validation
>>>>>>> there was incomplete.
>>>>>>>
>>>>>>> gcc-77608.diff
>>>>>>>
>>>>>>>
>>>>>>> PR middle-end/77608 - missing protection on trivially detectable
>>>>>>> runtime buffer overflow
>>>>>>>
>>>>>>> gcc/ChangeLog:
>>>>>>>
>>>>>>> Â Â Â PR middle-end/77608
>>>>>>> Â Â Â * builtins.c (compute_objsize): Handle non-constant offsets.
>>>>>>>
>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>
>>>>>>> Â Â Â PR middle-end/77608
>>>>>>> Â Â Â * gcc.dg/Wstringop-overflow.c: New test.
>>>>>> The recursive call into compute_objsize passing in the ostype avoids
>>>>>> having to think about the whole object vs nearest containing object
>>>>>> issues. Right?
>>>>>>
>>>>>> What's left to worry about is maximum or minimum remaining bytes
>>>>>> in the
>>>>>> object. At least that's my understanding of how ostype works here.
>>>>>>
>>>>>> So we get the amount remaining, ignoring the variable offset, from
>>>>>> the
>>>>>> recursive call (SIZE). The space left after we account for the
>>>>>> variable
>>>>>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>>>>>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>>>>>> SIZE-MAX which I think you get wrong (and you have to account for the
>>>>>> possibility that MAX or MIN is greater than SIZE and thus there's
>>>>>> nothing left).
>>>>>
>>>>> Subtracting the upper bound of the offset from the size instead
>>>>> of the lower bound when the caller is asking for the minimum
>>>>> object size would make the result essentially meaningless in
>>>>> common cases where the offset is smaller than size_t, as in:
>>>>>
>>>>> Â char a[7];
>>>>>
>>>>> Â void f (const char *s, unsigned i)
>>>>> Â {
>>>>> Â Â Â __builtin_strcpy (a + i, s);
>>>>> Â }
>>>>>
>>>>> Here, i's range is [0, UINT_MAX].
>>>>>
>>>>> IMO, it's only useful to use the lower bound here, otherwise
>>>>> the result would only rarely be non-zero.
>>>> But when we're asking for the minimum left, aren't we essentially
>>>> asking
>>>> for "how much space am I absolutely sure I can write"? And if that is
>>>> the question, then the only conservatively correct answer is to
>>>> subtract
>>>> the high bound.
>>>
>>> I suppose you could look at it that way but IME with this work
>>> (now, and also last year when I submitted a patch actually
>>> changing the built-in), using the upper bound is just not that
>>> useful because it's too often way too big. There's no way to
>>> distinguish an out-of-range upper bound that's the result of
>>> an inadequate attempt to constrain a value from an out-of-range
>>> upper bound that is sufficiently constrained but in a way GCC
>>> doesn't see.
>> Understood.
>>
>> So while it's reasonable to not warn in those cases where we just have
>> crap range information (that's always going to be the case for some code
>> regardless of how good my work or Andrew/Aldy's work is), we have to be
>> very careful and make sure that nobody acts on this information for
>> optimization purposes because what we're returning is not conservatively
>> correct.
>>
>>
>>>
>>> There are no clients of this API that would be affected by
>>> the decision one way or the other (unless the user specifies
>>> a -Wstringop-overflow= argument greater than the default 2)
>>> so I don't think what we do now matters much, if at all.
>> Right, but what's to stop someone without knowledge of the
>> implementation and its quirk of not returning the conservatively safe
>> result from using the results in other ways.
>
> Presumably they would find out by testing their code. But this
> is a hypothetical scenario. I added the function for warnings.
> I wasn't expecting it to be used for optimization, no such uses
> have emerged, and I don't have the impression that anyone is
> contemplating adding them (certainly not in stage 3). If you
> think the function could be useful for optimization then we
> should certainly consider changing it as we gain experience
> with it under those conditions.
Merely passing tests does not mean the code is correct, we both have the
war stories and scars to prove it. :-) Hell, I prove it to myself
nearly daily :(
Furthermore, just because nobody is using the function today in an
optimization context does not mean it will always be the case. Worse
yet, someone could potentially use a caller of compute_objsize without
knowing about the limitations.
In fact, if I look at how we handle expand_builtin_mempcpy we have:
/* Avoid expanding mempcpy into memcpy when the call is determined
to overflow the buffer. This also prevents the same overflow
from being diagnosed again when expanding memcpy. */
if (!check_memop_sizes (exp, dest, src, len))
return NULL_RTX;
While that's not strictly meant to be an optimization, it is in effect
changing the code we generate based on the return value of
check_memop_sizes, which comes from compute_objsize. Thankfully, the
worst that happens here is we'll fail to turn the mempcpy into a memcpy
if compute_objsize/check_memop_sizes returns a value that is not
strictly correct. But I think it highlights how easy it is to end up
having code generation changing based on the results of compute_objsize.
>> What would the impact be of simply not supporting those queries,
>> essentially returning "I don't know" rather than returning something
>> that isn't conservatively correct?
>
> Except for false negatives with -Wstringop-overflow=3 and =4
> (i.e., Object Size type 2 and 3) I don't think there would be
> any impact. As I said, the function isn't used for optimization
> and I don't think the option is used in these modes either, so
> in my mind it matters very little. I don't even think there
> are any tests for it in these modes, either.
SO based on my research around the mempcpy stuff above, my thinking is
refined a bit.
Inherently the result of compute_objsize is inexact when presented with
a non-constant offset (and it may be in other contexts as well). That
introduces a level of risk in that callers may well end up using the
result of compute_objsize in ways that are unsafe.
THe best way I know of to deal with that is to make the result a
multi-state so that we can distinguish between the exact results and
inexact results. ie, the fact that the function returns an inexact
result gets baked into its API. It's a lot harder to mis-use.
I could possibly live with a pair of comments though. In
compute_objsize we'd want a comment clarifying that the result is
inexact and discourage using the result to make code generation
decisions of any kind (ie, more general than just discouraging using the
return value for optimization purposes).
In the mempcpy code we'd want a comment indicating why it's safe to use
the result the way we do. Essentially if compute_objsize returns true,
then we transform the result into a memcpy + return value adjustment,
otherwise we just call mempcpy. In both cases we copy the same data and
the final return value is the same.
>
> For now, I've added comments to clarify the return value and
> the purpose of the function. Let me know if that's good enough
> or if you want me to make any other changes.
SO would you rather go with baking the inexact nature of the return
value into the API or the pair of comments noted above?
jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-12-01 8:27 ` Jeff Law
@ 2017-12-01 18:06 ` Martin Sebor
2017-12-07 21:06 ` Jeff Law
0 siblings, 1 reply; 12+ messages in thread
From: Martin Sebor @ 2017-12-01 18:06 UTC (permalink / raw)
To: Jeff Law, Gcc Patch List; +Cc: Jakub Jelinek
[-- Attachment #1: Type: text/plain, Size: 11878 bytes --]
On 12/01/2017 01:26 AM, Jeff Law wrote:
> On 11/30/2017 01:30 PM, Martin Sebor wrote:
>> On 11/22/2017 05:03 PM, Jeff Law wrote:
>>> On 11/21/2017 12:07 PM, Martin Sebor wrote:
>>>> On 11/21/2017 09:55 AM, Jeff Law wrote:
>>>>> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>>>>>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>>>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
>>>>>>>> The attached patch enhances -Wstringop-overflow to detect more
>>>>>>>> instances of buffer overflow at compile time by handling non-
>>>>>>>> constant offsets into the destination object that are known to
>>>>>>>> be in some range. The solution could be improved by handling
>>>>>>>> even more cases (e.g., anti-ranges or offsets relative to
>>>>>>>> pointers beyond the beginning of an object) but it's a start.
>>>>>>>>
>>>>>>>> In addition to bootsrapping/regtesting GCC, also tested with
>>>>>>>> Binutils/GDB, Glibc, and the Linux kernel on x86_64 with no
>>>>>>>> regressions.
>>>>>>>>
>>>>>>>> Martin
>>>>>>>>
>>>>>>>> The top of GDB fails to compile at the moment so the validation
>>>>>>>> there was incomplete.
>>>>>>>>
>>>>>>>> gcc-77608.diff
>>>>>>>>
>>>>>>>>
>>>>>>>> PR middle-end/77608 - missing protection on trivially detectable
>>>>>>>> runtime buffer overflow
>>>>>>>>
>>>>>>>> gcc/ChangeLog:
>>>>>>>>
>>>>>>>> PR middle-end/77608
>>>>>>>> * builtins.c (compute_objsize): Handle non-constant offsets.
>>>>>>>>
>>>>>>>> gcc/testsuite/ChangeLog:
>>>>>>>>
>>>>>>>> PR middle-end/77608
>>>>>>>> * gcc.dg/Wstringop-overflow.c: New test.
>>>>>>> The recursive call into compute_objsize passing in the ostype avoids
>>>>>>> having to think about the whole object vs nearest containing object
>>>>>>> issues. Right?
>>>>>>>
>>>>>>> What's left to worry about is maximum or minimum remaining bytes
>>>>>>> in the
>>>>>>> object. At least that's my understanding of how ostype works here.
>>>>>>>
>>>>>>> So we get the amount remaining, ignoring the variable offset, from
>>>>>>> the
>>>>>>> recursive call (SIZE). The space left after we account for the
>>>>>>> variable
>>>>>>> offset is [SIZE - MAX, SIZE - MIN]. So ISTM for type 0/1 you have to
>>>>>>> return SIZE-MIN (which you do) and for type 2/3 you have to return
>>>>>>> SIZE-MAX which I think you get wrong (and you have to account for the
>>>>>>> possibility that MAX or MIN is greater than SIZE and thus there's
>>>>>>> nothing left).
>>>>>>
>>>>>> Subtracting the upper bound of the offset from the size instead
>>>>>> of the lower bound when the caller is asking for the minimum
>>>>>> object size would make the result essentially meaningless in
>>>>>> common cases where the offset is smaller than size_t, as in:
>>>>>>
>>>>>> char a[7];
>>>>>>
>>>>>> void f (const char *s, unsigned i)
>>>>>> {
>>>>>> __builtin_strcpy (a + i, s);
>>>>>> }
>>>>>>
>>>>>> Here, i's range is [0, UINT_MAX].
>>>>>>
>>>>>> IMO, it's only useful to use the lower bound here, otherwise
>>>>>> the result would only rarely be non-zero.
>>>>> But when we're asking for the minimum left, aren't we essentially
>>>>> asking
>>>>> for "how much space am I absolutely sure I can write"? And if that is
>>>>> the question, then the only conservatively correct answer is to
>>>>> subtract
>>>>> the high bound.
>>>>
>>>> I suppose you could look at it that way but IME with this work
>>>> (now, and also last year when I submitted a patch actually
>>>> changing the built-in), using the upper bound is just not that
>>>> useful because it's too often way too big. There's no way to
>>>> distinguish an out-of-range upper bound that's the result of
>>>> an inadequate attempt to constrain a value from an out-of-range
>>>> upper bound that is sufficiently constrained but in a way GCC
>>>> doesn't see.
>>> Understood.
>>>
>>> So while it's reasonable to not warn in those cases where we just have
>>> crap range information (that's always going to be the case for some code
>>> regardless of how good my work or Andrew/Aldy's work is), we have to be
>>> very careful and make sure that nobody acts on this information for
>>> optimization purposes because what we're returning is not conservatively
>>> correct.
>>>
>>>
>>>>
>>>> There are no clients of this API that would be affected by
>>>> the decision one way or the other (unless the user specifies
>>>> a -Wstringop-overflow= argument greater than the default 2)
>>>> so I don't think what we do now matters much, if at all.
>>> Right, but what's to stop someone without knowledge of the
>>> implementation and its quirk of not returning the conservatively safe
>>> result from using the results in other ways.
>>
>> Presumably they would find out by testing their code. But this
>> is a hypothetical scenario. I added the function for warnings.
>> I wasn't expecting it to be used for optimization, no such uses
>> have emerged, and I don't have the impression that anyone is
>> contemplating adding them (certainly not in stage 3). If you
>> think the function could be useful for optimization then we
>> should certainly consider changing it as we gain experience
>> with it under those conditions.
> Merely passing tests does not mean the code is correct, we both have the
> war stories and scars to prove it. :-) Hell, I prove it to myself
> nearly daily :(
>
> Furthermore, just because nobody is using the function today in an
> optimization context does not mean it will always be the case. Worse
> yet, someone could potentially use a caller of compute_objsize without
> knowing about the limitations.
>
> In fact, if I look at how we handle expand_builtin_mempcpy we have:
>
> /* Avoid expanding mempcpy into memcpy when the call is determined
> to overflow the buffer. This also prevents the same overflow
> from being diagnosed again when expanding memcpy. */
> if (!check_memop_sizes (exp, dest, src, len))
> return NULL_RTX;
>
> While that's not strictly meant to be an optimization, it is in effect
> changing the code we generate based on the return value of
> check_memop_sizes, which comes from compute_objsize. Thankfully, the
> worst that happens here is we'll fail to turn the mempcpy into a memcpy
> if compute_objsize/check_memop_sizes returns a value that is not
> strictly correct. But I think it highlights how easy it is to end up
> having code generation changing based on the results of compute_objsize.
I think you have misread the code (which is easy to do), so I'm
not sure it does highlight it.
The mempcpy code unconditionally uses Object Size type 0 (see
check_memop_sizes) so it cannot be used the way you are concerned
about. All raw memory built-ins that write into memory do.
The string functions do use other Object Size modes, depending
on the -Wstringop-overflow=N argument (by default mode 1), but
they don't use it to disable the expansion.
Even if any of the existing functions did use the function to
disable the expansion, because compute_objsize uses the lower
bound of the offset, the result is the upper of the size and
so only considered invalid if it definitely is out-of-bounds.
In this scenario, using the upper bound would have the effect
of disabling the expansion even in cases they are not provably
in bounds (though only in Object Size modes 2 and 3, which
I suspect are never used). I.e., it would have an adverse
impact not only in terms of false positives but also on
the expansion.
If we were talking about changing __builtin_object_size (we're
not), using the upper bound would cause string function calls
with an in-bounds lower bound and an out-of-bounds upper bound
to abort with _FORTIFY_SOURCE. Conversely, giving up by
returning a "don't know" result in this case would fail to
prevent provable buffer overflows.
I believe that where ranges are involved, it only makes sense
to use the conservative bound (which may be the lower or the
upper bound, depending on the circumstances). But it just
isn't viable to use the other one because it leads to wrong
(suboptimal) results.
>>> What would the impact be of simply not supporting those queries,
>>> essentially returning "I don't know" rather than returning something
>>> that isn't conservatively correct?
>>
>> Except for false negatives with -Wstringop-overflow=3 and =4
>> (i.e., Object Size type 2 and 3) I don't think there would be
>> any impact. As I said, the function isn't used for optimization
>> and I don't think the option is used in these modes either, so
>> in my mind it matters very little. I don't even think there
>> are any tests for it in these modes, either.
> SO based on my research around the mempcpy stuff above, my thinking is
> refined a bit.
>
> Inherently the result of compute_objsize is inexact when presented with
> a non-constant offset (and it may be in other contexts as well). That
> introduces a level of risk in that callers may well end up using the
> result of compute_objsize in ways that are unsafe.
>
> THe best way I know of to deal with that is to make the result a
> multi-state so that we can distinguish between the exact results and
> inexact results. ie, the fact that the function returns an inexact
> result gets baked into its API. It's a lot harder to mis-use.
The result already is multi-state: NULL means "don't know," which
is interpreted as valid.
> I could possibly live with a pair of comments though. In
> compute_objsize we'd want a comment clarifying that the result is
> inexact and discourage using the result to make code generation
> decisions of any kind (ie, more general than just discouraging using the
> return value for optimization purposes).
>
> In the mempcpy code we'd want a comment indicating why it's safe to use
> the result the way we do. Essentially if compute_objsize returns true,
> then we transform the result into a memcpy + return value adjustment,
> otherwise we just call mempcpy. In both cases we copy the same data and
> the final return value is the same.
>
>>
>> For now, I've added comments to clarify the return value and
>> the purpose of the function. Let me know if that's good enough
>> or if you want me to make any other changes.
> SO would you rather go with baking the inexact nature of the return
> value into the API or the pair of comments noted above?
I added the comment to compute_objsize in the last iteration
of the patch.
As I explained above, expand_builtin_mempcpy isn't affected by
this patch. The comments above and within in the function
already explain what what happens when a buffer overflow is
detected and why so I'm not sure what else to say there. If
I misunderstood your suggestion please clarify.
The other change you are suggesting means restoring the false
negatives in Object Size modes 2 and 3 where my patch detected
true positives. Here's an example to demonstrate the effect.
With my original patch, the overflow in both functions below
is diagnosed with all -Wstringop-overflow=N arguments. With
the change you asked for, only the memcpy overflow is diagnosed
with all arguments. The strncpy overflow is only diagnosed
with N=1 and 2, and suppressed with N=3 and N=4. That's clearly
a worse outcome, but in the interest of moving forward I've made
the change. I think the overall improvement is worthwhile despite
this flaw.
$ cat a.c && gcc -O2 -S -Wstringop-overflow=3 a.c
char d[7];
void f (const void *s, unsigned i)
{
if (i < 3 || 9 < i)
i = 3;
__builtin_memcpy (d + i, s, 5); // diagnosed
}
void g (const char *s, unsigned i)
{
if (i < 3 || 9 < i)
i = 3;
__builtin_strncpy (d + i, s, 5); // false negative
}
a.c: In function âfâ:
a.c:8:3: warning: â__builtin_memcpyâ writing 5 bytes into a region of
size 4 overflows the destination [-Wstringop-overflow=]
__builtin_memcpy (d + i, s, 5); // diagnosed
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Martin
[-- Attachment #2: gcc-77608.diff --]
[-- Type: text/x-patch, Size: 8872 bytes --]
PR middle-end/77608 - missing protection on trivially detectable runtime buffer overflow
gcc/ChangeLog:
PR middle-end/77608
* builtins.c (compute_objsize): Handle non-constant offsets.
gcc/testsuite/ChangeLog:
PR middle-end/77608
* gcc.dg/Wstringop-overflow.c: New test.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index 650de0d..0565eaf 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3262,8 +3262,12 @@ check_sizes (int opt, tree exp, tree size, tree maxlen, tree src, tree objsize)
/* Helper to compute the size of the object referenced by the DEST
expression which must have pointer type, using Object Size type
OSTYPE (only the least significant 2 bits are used). Return
- the size of the object if successful or NULL when the size cannot
- be determined. */
+ an estimate of the size of the object if successful or NULL when
+ the size cannot be determined. When the referenced object involves
+ a non-constant offset in some range the returned value represents
+ the largest size given the smallest non-negative offset in the
+ range. The function is intended for diagnostics and is not
+ suitable for optimization. */
tree
compute_objsize (tree dest, int ostype)
@@ -3276,24 +3280,68 @@ compute_objsize (tree dest, int ostype)
if (compute_builtin_object_size (dest, ostype, &size))
return build_int_cst (sizetype, size);
- /* Unless computing the largest size (for memcpy and other raw memory
- functions), try to determine the size of the object from its type. */
- if (!ostype)
- return NULL_TREE;
-
if (TREE_CODE (dest) == SSA_NAME)
{
gimple *stmt = SSA_NAME_DEF_STMT (dest);
if (!is_gimple_assign (stmt))
return NULL_TREE;
+ dest = gimple_assign_rhs1 (stmt);
+
tree_code code = gimple_assign_rhs_code (stmt);
- if (code != ADDR_EXPR && code != POINTER_PLUS_EXPR)
- return NULL_TREE;
+ if (code == POINTER_PLUS_EXPR)
+ {
+ /* compute_builtin_object_size fails for addresses with
+ non-constant offsets. Try to determine the range of
+ such an offset here and use it to adjus the constant
+ size. */
+ tree off = gimple_assign_rhs2 (stmt);
+ if (TREE_CODE (off) == SSA_NAME
+ && INTEGRAL_TYPE_P (TREE_TYPE (off)))
+ {
+ wide_int min, max;
+ enum value_range_type rng = get_range_info (off, &min, &max);
- dest = gimple_assign_rhs1 (stmt);
+ if (rng == VR_RANGE)
+ {
+ if (tree size = compute_objsize (dest, ostype))
+ {
+ wide_int wisiz = wi::to_wide (size);
+
+ /* Ignore negative offsets for now. */
+ if (wi::sign_mask (min))
+ ;
+ else if (ostype > 1)
+ {
+ /* In Object Size modes 2 and 3, return zero
+ if the remaining size is definitely zero,
+ otherwise return a "don't know" result. */
+ if (wi::leu_p (wisiz, min))
+ return size_zero_node;
+
+ return NULL_TREE;
+ }
+ else if (wi::ltu_p (min, wisiz))
+ /* In Object Size modes 0 and 1, use the lower
+ bound of the offset to compute as the most
+ optimistic estimate of the (remaining) size. */
+ return wide_int_to_tree (TREE_TYPE (size),
+ wi::sub (wisiz, min));
+ else
+ return size_zero_node;
+ }
+ }
+ }
+ }
+ else if (code != ADDR_EXPR)
+ return NULL_TREE;
}
+ /* Unless computing the largest size (for memcpy and other raw memory
+ functions), try to determine the size of the object from its type. */
+ if (!ostype)
+ return NULL_TREE;
+
if (TREE_CODE (dest) != ADDR_EXPR)
return NULL_TREE;
diff --git a/gcc/testsuite/gcc.dg/Wstringop-overflow.c b/gcc/testsuite/gcc.dg/Wstringop-overflow.c
new file mode 100644
index 0000000..b5bd40e
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wstringop-overflow.c
@@ -0,0 +1,132 @@
+/* PR middle-end/77608 - missing protection on trivially detectable runtime
+ buffer overflow
+ { dg-do compile }
+ { dg-options "-O2 -Wstringop-overflow -ftrack-macro-expansion=0" } */
+
+#define SIZE_MAX __SIZE_MAX__
+#define DIFF_MAX __PTRDIFF_MAX__
+#define DIFF_MIN (-DIFF_MAX - 1)
+
+typedef __SIZE_TYPE__ size_t;
+
+extern void* memcpy (void*, const void*, size_t);
+extern char* strcpy (char*, const char*);
+extern char* strncpy (char*, const char*, size_t);
+
+void sink (void*);
+
+extern size_t unsigned_value (void)
+{
+ extern volatile size_t unsigned_value_source;
+ return unsigned_value_source;
+}
+
+size_t unsigned_range (size_t min, size_t max)
+{
+ size_t val = unsigned_value ();
+ return val < min || max < val ? min : val;
+}
+
+#define UR(min, max) unsigned_range (min, max)
+
+
+char a7[7];
+
+struct MemArray { char a9[9]; char a1[1]; };
+
+void test_memcpy_array (const void *s)
+{
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ T (a7 + UR (0, 1), s, 7);
+ T (a7 + UR (0, 7), s, 7);
+ T (a7 + UR (0, 8), s, 7);
+ T (a7 + UR (0, DIFF_MAX), s, 7);
+ T (a7 + UR (0, SIZE_MAX), s, 7);
+
+ T (a7 + UR (1, 2), s, 7); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), s, 7); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), s, 7); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ /* This is valid. */
+ char *d = a7 + 7;
+ T (d + UR (-8, -7), s, 7);
+}
+
+/* Verify the absence of warnings for memcpy writing beyond object
+ boundaries. */
+
+void test_memcpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (memcpy ((d), (s), (n)), sink (d))
+
+ /* The following are valid. */
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ /* The following are invalid. Unfortunately, there is apparently enough
+ code out there that abuses memcpy to write past the end of one member
+ and into the members that follow so the following are not diagnosed
+ by design. It sure would be nice not to have to cater to hacks like
+ these... */
+ T (p->a9 + UR (1, 2), s, 9);
+ T (p->a9 + UR (1, 2), s, 123);
+}
+
+
+void test_strcpy_array (void)
+{
+#undef T
+#define T(d, s) (strcpy ((d), (s)), sink (d))
+
+ T (a7 + UR (0, 1), "012345");
+ T (a7 + UR (0, 7), "012345");
+ T (a7 + UR (0, 8), "012345");
+ T (a7 + UR (0, DIFF_MAX), "012345");
+ T (a7 + UR (0, SIZE_MAX), "012345");
+
+ T (a7 + UR (1, 2), "012345"); /* { dg-warning "writing 7 bytes into a region of size 6" } */
+ T (a7 + UR (2, 3), "012345"); /* { dg-warning "writing 7 bytes into a region of size 5" } */
+ T (a7 + UR (6, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 1" } */
+ T (a7 + UR (7, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (8, 9), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ T (a7 + UR (9, 10), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+ T (a7 + UR (DIFF_MAX, SIZE_MAX), "012345"); /* { dg-warning "writing 7 bytes into a region of size 0" } */
+
+ char *d = a7 + 7;
+
+ T (d + UR (-8, -7), "012345");
+}
+
+void test_strncpy_memarray (struct MemArray *p, const void *s)
+{
+#undef T
+#define T(d, s, n) (strncpy ((d), (s), (n)), sink (d))
+
+ T (p->a9 + UR (0, 1), s, 9);
+ T (p->a9 + UR (0, 7), s, 9);
+ T (p->a9 + UR (0, 8), s, 9);
+ T (p->a9 + UR (0, DIFF_MAX), s, 9);
+ T (p->a9 + UR (0, SIZE_MAX), s, 9);
+
+ T (p->a9 + UR (1, 2), s, 9); /* { dg-warning "writing 9 bytes into a region of size 8" } */
+ T (p->a9 + UR (2, 3), s, 9); /* { dg-warning "writing 9 bytes into a region of size 7" } */
+ T (p->a9 + UR (6, 9), s, 9); /* { dg-warning "writing 9 bytes into a region of size 3" } */
+ T (p->a9 + UR (9, 10), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+ T (p->a9 + UR (10, 11), s, 9); /* { dg-warning "writing 9 bytes into a region of size 0" } */
+
+ T (p->a9 + UR (DIFF_MAX, DIFF_MAX + (size_t)1), s, 1); /* { dg-warning "writing 1 byte into a region of size 0" } */
+ T (p->a9 + UR (DIFF_MAX, SIZE_MAX), s, 3); /* { dg-warning "writing 3 bytes into a region of size 0" } */
+}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608)
2017-12-01 18:06 ` Martin Sebor
@ 2017-12-07 21:06 ` Jeff Law
0 siblings, 0 replies; 12+ messages in thread
From: Jeff Law @ 2017-12-07 21:06 UTC (permalink / raw)
To: Martin Sebor, Gcc Patch List; +Cc: Jakub Jelinek
On 12/01/2017 11:06 AM, Martin Sebor wrote:
> On 12/01/2017 01:26 AM, Jeff Law wrote:
>> On 11/30/2017 01:30 PM, Martin Sebor wrote:
>>> On 11/22/2017 05:03 PM, Jeff Law wrote:
>>>> On 11/21/2017 12:07 PM, Martin Sebor wrote:
>>>>> On 11/21/2017 09:55 AM, Jeff Law wrote:
>>>>>> On 11/19/2017 04:28 PM, Martin Sebor wrote:
>>>>>>> On 11/18/2017 12:53 AM, Jeff Law wrote:
>>>>>>>> On 11/17/2017 12:36 PM, Martin Sebor wrote:
[ Lots of snipping ]
>>
>> In fact, if I look at how we handle expand_builtin_mempcpy we have:
>>
>> Â /* Avoid expanding mempcpy into memcpy when the call is determined
>>     to overflow the buffer. This also prevents the same overflow
>>     from being diagnosed again when expanding memcpy. */
>> Â if (!check_memop_sizes (exp, dest, src, len))
>> Â Â Â return NULL_RTX;
>>
>> While that's not strictly meant to be an optimization, it is in effect
>> changing the code we generate based on the return value of
>> check_memop_sizes, which comes from compute_objsize. Thankfully, the
>> worst that happens here is we'll fail to turn the mempcpy into a memcpy
>> if compute_objsize/check_memop_sizes returns a value that is not
>> strictly correct. But I think it highlights how easy it is to end up
>> having code generation changing based on the results of compute_objsize.
>
> I think you have misread the code (which is easy to do), so I'm
> not sure it does highlight it.
Just to be clear, I'm convinced the code should DTRT as-written and that
it's behavior is not changed by your patch.
My point is that these routines should not generally be used to
influence code generation/optimization decisions. In an ideal world we
could express and enforce that rule. But we don't live in that ideal
world and as a result it's relatively easy to use those routines to
influence code generation decisions without even knowing it.
So again, anytime we have something like what we see above where we call
check_memop_sizes/compute_objsize and the result is used to select
between paths that change code generation/optimization we need a
comment. We also need a comment in compute_objsize to discourage its
use in any contexts where the result affects code generation/optimization.
Again, the code is safe as-is and not affected by your patch. I'm just
asking for a comment for those functions and at any site where those
functions are used to influence code generation/optimization decisions.
[ More snipping.. Hopefully not losing too much context.. ]
>> SO would you rather go with baking the inexact nature of the return
>> value into the API or the pair of comments noted above?
>
> I added the comment to compute_objsize in the last iteration
> of the patch.
>
> As I explained above, expand_builtin_mempcpy isn't affected by
> this patch. The comments above and within in the function
> already explain what what happens when a buffer overflow is
> detected and why so I'm not sure what else to say there. If
> I misunderstood your suggestion please clarify.
The comment I'm looking for in expand_builtin_mempcpy would be something
like this:
/* Policy does not generally allow using compute_objsize (which
is used internally by check_memop_size) to change code generation
or drive optimization decisions.
In this instance it is safe because the code we generate has
the same semantics regardless of the return value of
check_memop_sizes. Exactly the same amount of data is copied
and the return value is exactly the same in both cases.
Furthermore, check_memop_size always uses mode 0 for the call to
compute_objsize, so the imprecise nature of compute_objsize is
avoided. */
>
> The other change you are suggesting means restoring the false
> negatives in Object Size modes 2 and 3 where my patch detected
> true positives. Here's an example to demonstrate the effect.
> With my original patch, the overflow in both functions below
> is diagnosed with all -Wstringop-overflow=N arguments. With
> the change you asked for, only the memcpy overflow is diagnosed
> with all arguments. The strncpy overflow is only diagnosed
> with N=1 and 2, and suppressed with N=3 and N=4. That's clearly
> a worse outcome, but in the interest of moving forward I've made
> the change. I think the overall improvement is worthwhile despite
> this flaw.
So given the comments about restrictions in how compute_objsize is used,
I don't think we need to make the change I originally asked for. Go
with whatever you think is best WRT handling of that bound computation.
Either computation of that bound has a degree of imprecision due to
range involvement and makes the result unsuitable for influencing code
generation or optimization.
In the compute_objsize comment I'd use
"The function is intended for diagnostics and should not be used to
influence code generation or optimization."
I think that's slightly better.
OK with either computation the updated comment for compute_objsize.
Please add a comment to the mempcpy bits as well.
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-12-07 21:06 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-17 19:48 [PATCH] handle non-constant offsets in -Wstringop-overflow (PR 77608) Martin Sebor
2017-11-18 8:06 ` Jeff Law
2017-11-18 16:59 ` Martin Sebor
2017-11-19 22:36 ` Jeff Law
2017-11-19 23:47 ` Martin Sebor
2017-11-21 17:01 ` Jeff Law
2017-11-21 19:53 ` Martin Sebor
2017-11-23 0:24 ` Jeff Law
2017-11-30 21:47 ` Martin Sebor
2017-12-01 8:27 ` Jeff Law
2017-12-01 18:06 ` Martin Sebor
2017-12-07 21:06 ` Jeff Law
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).