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
