This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC][Draft patch] Introduce IntegerSanitizer in GCC.


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




Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]