[Bug sanitizer/108481] New: UBsan missed a signed integer overflow

[Bug sanitizer/108481] New: UBsan missed a signed integer overflow

[shaohua.li at inf dot ethz.ch]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

            Bug ID: 108481
           Summary: UBsan missed a signed integer overflow
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: shaohua.li at inf dot ethz.ch
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

For the following code, UBsan at -O1 and above missed the signed integer
overflow, while -O0 found it. I checked the assembly and the buggy code was not
optimized away.

Clang detected it at all optimization levels.

Compiler explorer: https://godbolt.org/z/rK6sad1WG

%cat a.c
int a, d;
short b;
short *c = &b;
int *const e = &d;
static char f(int *g, short h, long i) {
  d = 2;
  for (;; d--) {
    int j = *e;
    *g = (0 || h) / h;
    *c = 3;
    if ((i * (unsigned long)7 <= 1) << j)
      ;
    else {
      i = i - 6822162149299574294;
      if (j) {
        if (*g)
          break;
        continue;
      }
      return 8;
    }
  }
  return h;
}
int main() {
  int *k = &a;
  f(k, 4, 8289379813243698614);
}
%
%gcc-tk -fsanitize=undefined -O1 && ./a.out
%
%gcc-tk -fsanitize=undefined -O0 && ./a.out
/a.c:14:9: runtime error: signed integer overflow: -5354944485355449974 -
6822162149299574294 cannot be represented in type 'long int'
%

[Bug sanitizer/108481] UBsan missed a signed integer overflow

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-01-23
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Confirmed.

[Bug sanitizer/108481] [13 Regression] UBsan missed a signed integer overflow

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |needs-bisection, wrong-code
            Summary|UBsan missed a signed       |[13 Regression] UBsan
                   |integer overflow            |missed a signed integer
                   |                            |overflow
      Known to fail|                            |13.0
   Target Milestone|---                         |13.0
      Known to work|                            |12.2.0

[Bug sanitizer/108481] [13 Regression] UBsan missed a signed integer overflow

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
      i = i - 6822162149299574294;

Is not being invoked on the executable code.

If we look at look at the original code:


    if ((i * (unsigned long)7 <= 1) << j)
      ;
    else {
      i = i - 6822162149299574294;
      if (j) {
        if (*g)
          break;
        continue;
      }
      return 8;
    }
...
  return h;

The only path where the undefined behavior even matters is inside the path that
had continue in it. Which is where the subtraction is pushed to now.

I don't know if we should declear this as a valid thing to do or not.

[Bug sanitizer/108481] [13 Regression] UBsan missed a signed integer overflow

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
>       i = i - 6822162149299574294;
> 
> Is not being invoked on the executable code.
> 
> If we look at look at the original code:
> 
> 
>     if ((i * (unsigned long)7 <= 1) << j)
>       ;
>     else {
>       i = i - 6822162149299574294;
>       if (j) {
>         if (*g)
>           break;
>         continue;
>       }
>       return 8;
>     }
> ...
>   return h;
> 
> The only path where the undefined behavior even matters is inside the path
> that had continue in it. Which is where the subtraction is pushed to now.
> 
> I don't know if we should declear this as a valid thing to do or not.

This is what the IR looks in the end:
  _27 = _17 << _20;
  if (_27 != 0)
    goto <bb 12>; [50.00%]
  else
    goto <bb 9>; [50.00%]

  <bb 9> [local count: 1560331071]:
  if (_20 != 0)
    goto <bb 10>; [94.50%]
  else
    goto <bb 13>; [5.50%]

  <bb 10> [local count: 1474512859]:
  _30 = a;
  if (_30 != 0)
    goto <bb 13>; [67.00%]
  else
    goto <bb 11>; [33.00%]

  <bb 11> [local count: 486589241]:
  i_28 = .UBSAN_CHECK_SUB (i_13, 6822162149299574294);

  <bb 12> [local count: 2046920311]:
  # i_34 = PHI <i_13(8), i_28(11)>
  (Loop back)
...

  <bb 13> [local count: 1073741830]:
  return 0;

As you can see the only path where the ubsan_check_sub is executed is right
before the continue which is seems like a very valid thing to do.

[Bug sanitizer/108481] [13 Regression] UBsan missed a signed integer overflow

[shaohua.li at inf dot ethz.ch]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

--- Comment #4 from Li Shaohua <shaohua.li at inf dot ethz.ch> ---
(In reply to Andrew Pinski from comment #2)
>       i = i - 6822162149299574294;
> 
> Is not being invoked on the executable code.
> 
> If we look at look at the original code:
> 
> 
>     if ((i * (unsigned long)7 <= 1) << j)
>       ;
>     else {
>       i = i - 6822162149299574294;
>       if (j) {
>         if (*g)
>           break;
>         continue;
>       }
>       return 8;
>     }
> ...
>   return h;
> 
> The only path where the undefined behavior even matters is inside the path
> that had continue in it. Which is where the subtraction is pushed to now.
> 
> I don't know if we should declear this as a valid thing to do or not.

When I compiled the source code with `-O1 -fsanitize=undefined` and then used
gdb to check the execution trace of the binary, I indeed observed that `i = i -
6822162149299574294` was executed.

[Bug sanitizer/108481] [10/11/12/13 Regression] UBsan missed a signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|12.2.0                      |
            Summary|[13 Regression] UBsan       |[10/11/12/13 Regression]
                   |missed a signed integer     |UBsan missed a signed
                   |overflow                    |integer overflow
           Keywords|needs-bisection             |

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Why is this marked 13 Regression?  I can certainly reproduce up to ~ r260000,
with just
-O1 -fsanitize=signed-integer-overflow so that I can use newer libubsan for
even older code back to r8-93-g2700fbd655f608e8e23.
The difference with r8-93 from earlier commits is that we decide to sink the
  i_15 = UBSAN_CHECK_SUB (i_9, 6822162149299574294);
statement, because the result of i isn't really used in the path when
j == 0.
We intentionally mark UBSAN_CHECK_SUB etc. internal functions as ECF_CONST, so
that they can be DCEd etc., we don't want to kill performance with
-fsanitize=undefined, if you don't care about performance of the code, just use
-O0 with -fsanitize=undefined.
So, similarly to how we don't diagnose
long
foo (long i)
{
  i = i - 6822162149299574294;
  return 0;
}

int
main ()
{
  return foo (-5354944485355449974);
}
at -O2 because the result of the subtraction isn't used, we allow the i = i -
6822162149299574294; subtraction to be sunk, so effectively:
@@ -11,8 +11,8 @@ static char f(int *g, short h, long i) {
     if ((i * (unsigned long)7 <= 1) << j)
       ;
     else {
-      i = i - 6822162149299574294;
       if (j) {
+        i = i - 6822162149299574294;
         if (*g)
           break;
         continue;
change in the source because for the return 8; path nothing cares about the
result.

I'd say this behaves as designed.

[Bug sanitizer/108481] [10/11/12/13 Regression] UBsan missed a signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'd say the implementation intends to warn about UB that still remains in the
code after optimizations, if a result of some UB invoking operation isn't used
(the DCE case) or as in this case the UB happens only when the result of the UB
isn't used but it is used in some other path (sinking) etc., then the UB isn't
really invoked in the generated code.
While in -O0 code it will be encountered and so it is diagnosed.

[Bug sanitizer/108481] [10/11/12/13 Regression] UBsan missed a signed integer overflow

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108481

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |INVALID

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This works as designed.