On 11/07/16 18:05, Jakub Jelinek wrote:
On Tue, Jul 05, 2016 at 10:31:31AM +0300, Maxim Ostapenko wrote:
CC'ing Jakub, Marek and Kostya, sanitizer maintainers in GCC.
Jakub, thanks for your summary.
I'm not convinced it is a good idea, that is why we've intentionally left it
out when adding UBSan support, IMHO such an option defines substantially
different languages.
The reason why I thought about -fsanitize=unsigned-integer-overflow
would be useful is that people still hit on undesired integer overflows
in their code (that may even lead to security vulnerabilities), despite
the fact some people intentionally rely on them. Of course those people
can just ignore -fsanitize=unsigned-integer-overflow. But others might
find this option to be useful in their projects (e.g. Android people),
because it finds real bugs in their code (even this very draft version
managed to find several bugs in OSS projects).
The wrapping behavior of unsigned integer arithmetics is
integral part of the language, lots of code obviously relies on it.
E.g. in unsigned arithmetics, x - y and t = -y; x + t is the same thing,
while the patch would treat that differently. Or would it even allow
unary negation of non-zero unsigned values? On IRC I've mentioned e.g.
loops with unsigned iterator and bounds that can iterate forward or backward
at runtime, like:
void foo (unsigned start, unsigned end, unsigned step)
{
for (unsigned i = start; i != end; i += step)
...
}
would one have to rewrite this to do something like: "step_is_negative" ? i -=
-step : i += step
instead? How would code portably say that for a given arithmetics it really
does assume wrapping behavior? Replacing x = y + z; with
(void) __builtin_add_overflow (y, z, &x);
is not sufficiently portable, trying x = (y + z) & ~(unsigned) 0;
I assume would still trigger, because the unsigned arithmetics still
"overflows".
True, one should rewrite this like "step_is_negative" ? i -= -step : i
+= step to avoid false positive report here. Another option would be
just to ignore a FP report here or use some post processing to filter it
out. Anyway, there is a bunch of situations where we can't simply
determine whether overflow is desired or not and people would need to
decide by themselves.
I'm really surprised you didn't run into many more "issues" during your
tests.
Well, I used some heuristics (e.g. guessing the real type of LHS of
binary expression) to reduce number of these "issues" during my tests.
Actually, I didn't see many such cases:
void foo (unsigned start, unsigned end, unsigned step)
{
for (unsigned i = start; i != end; i += step)
...
}
instead, much more common were:
void bar(char *start, unsigned offset)
{
...
char *end = start + offset;
}
with negative offset. And here we need a special treatment (I treated
'offset' as a signed value when expanding a ISAN_CHECK_ADD builtin in
expand_addsub_overflow, this is not portable, but works most of the time).
-Maxim
Jakub
