[Bug sanitizer/108060] New: UBsan missed an out-of-bound bug at -O0

[Bug sanitizer/108060] New: UBsan missed an out-of-bound bug at -O0

[shaohua.li at inf dot ethz.ch]

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

            Bug ID: 108060
           Summary: UBsan missed an out-of-bound bug at -O0
           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 -O0 missed reporting the out-of-bound access,
while -O1 and above caught it. 

Clang could detect it at all optimization levels.

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

% cat a.c
int a[8];
short b;
char c;
int main() {
  b = -32768;
  a[b] |= c;
}
%
% gcc-tk -O0 -fsanitize=undefined -fno-sanitize-recover=all a.c && ./a.out
Segmentation fault
% gcc-tk -O1 -fsanitize=undefined -fno-sanitize-recover=all a.c && ./a.out
a.c:6:4: runtime error: index -32768 out of bounds for type 'int [8]'
%

Interestingly, if you don't use `-fno-sanitize-recover=all`, none of opt levels
could detect it:
% gcc-tk -O1 -fsanitize=undefined a.c && ./a.out
Segmentation fault
%

[Bug sanitizer/108060] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-12-12
                 CC|                            |jason at gcc dot gnu.org
            Summary|UBsan missed an             |UBsan missed an
                   |out-of-bound bug at -O0     |out-of-bound bug at -O0
                   |                            |since
                   |                            |r7-1900-g8a1b7b7fd75a3847
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Confirmed. So it started with r7-1900-g8a1b7b7fd75a3847 where the array is
accessed before UBSAN check:

gcc pr108060.c -fsanitize=undefined -fdump-tree-gimple=/dev/stdout
int main ()
{
  int D.2422;
  int D.2423;

  {
    b = -32768;
    b.0_1 = b;
    _2 = (int) b.0_1;
    _3 = a[_2]; <--- load happens here
    c.1_4 = c;
    _5 = (int) c.1_4;
    b.2_6 = b;
    D.2422 = (int) b.2_6;
    .UBSAN_BOUNDS (0B, D.2422, 7);
    _7 = _3 | _5;
    a[D.2422] = _7;
  }
  D.2423 = 0;
  return D.2423;
}

thus we crashes before the .UBSAN_BOUNDS happens. Before the revision we
emitted:

   {
    b = -4169;
    b.0_1 = b;
    D.2074 = (int) b.0_1;
    UBSAN_BOUNDS (0B, D.2074, 7);
    b.1_2 = b;
    _3 = (int) b.1_2;
    _4 = a[_3];
    _5 = _4 | 1;
    a[D.2074] = _5;
  }

[Bug sanitizer/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|UBsan missed an             |[10/11/12/13 Regression]
                   |out-of-bound bug at -O0     |UBsan missed an
                   |since                       |out-of-bound bug at -O0
                   |r7-1900-g8a1b7b7fd75a3847   |since
                   |                            |r7-1900-g8a1b7b7fd75a3847
   Target Milestone|---                         |10.5

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
          Component|sanitizer                   |c

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
The frontend emits

{
  b = -32768;
  a[.UBSAN_BOUNDS (0B, SAVE_EXPR <(int) b>, 7);, SAVE_EXPR <(int) b>;] =
a[(int) b] | (int) c;
}

and appearantly expects that the side-effects of the LHS are evaluated before
the side-effects of the RHS.  It also doesn't look at the RHS at all,
likely the instrumentation happens before GENERICizing the |= operator.

I think this is a frontend mistake.

The C++ frontend splits turns a[b] |= c into a[b] = a[b] | c before
instrumentation.

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
@Marek, can you please take a look?

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[marxin at gcc dot gnu.org]

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

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #3)
> @Marek, can you please take a look?

PING please

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[marxin at gcc dot gnu.org]

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

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
*** Bug 109050 has been marked as a duplicate of this bug. ***

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |mpolacek at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #6 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
OK, I didn't see the PR until now.

[Bug c/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[mpolacek at gcc dot gnu.org]

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

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Candidate fix:

--- a/gcc/c-family/c-gimplify.cc
+++ b/gcc/c-family/c-gimplify.cc
@@ -106,6 +106,18 @@ ubsan_walk_array_refs_r (tree *tp, int *walk_subtrees,
void *data)
     }
   else if (TREE_CODE (*tp) == ARRAY_REF)
     ubsan_maybe_instrument_array_ref (tp, false);
+  else if (TREE_CODE (*tp) == MODIFY_EXPR)
+    {
+      /* Since r7-1900, we gimplify RHS before LHS.  Consider
+      a[b] |= c;
+    wherein we can have a single shared tree a[b] in both LHS and RHS.
+    If we only instrument the LHS and the access is invalid, the program
+    could crash before emitting a UBSan error.  So instrument the RHS
+    first.  */
+      *walk_subtrees = 0;
+      walk_tree (&TREE_OPERAND (*tp, 1), ubsan_walk_array_refs_r, pset, pset);
+      walk_tree (&TREE_OPERAND (*tp, 0), ubsan_walk_array_refs_r, pset, pset);
+    }
   return NULL_TREE;
 }

It handles
  b = 0;
  a[b] = (a[b], b = -32768, a[b] | c);
correctly (the first a[b] is OK but not the 2nd or 3rd).

[Bug sanitizer/108060] [10/11/12/13 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:4d0baeae315ebe7d0ec7682ea3e7c0516027c2b8

commit r13-6593-g4d0baeae315ebe7d0ec7682ea3e7c0516027c2b8
Author: Marek Polacek <polacek@redhat.com>
Date:   Wed Mar 8 09:15:07 2023 -0500

    ubsan: missed -fsanitize=bounds for compound ops [PR108060]

    In this PR we are dealing with a missing .UBSAN_BOUNDS, so the
    out-of-bounds access in the test makes the program crash before
    a UBSan diagnostic was emitted.  In C and C++, c_genericize gets

      a[b] = a[b] | c;

    but in C, both a[b] are one identical shared tree (not in C++ because
    cp_fold/ARRAY_REF created two same but not identical trees).  Since
    ubsan_walk_array_refs_r keeps a pset, in C we produce

      a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] = a[b] | c;

    because the LHS is walked before the RHS.

    Since r7-1900, we gimplify the RHS before the LHS.  So the statement above
    gets gimplified into

        _1 = a[b];
        c.0_2 = c;
        b.1 = b;
        .UBSAN_BOUNDS (0B, b.1, 8);

    With this patch we produce:

      a[b] = a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] | c;

    which gets gimplified into:

        b.0 = b;
        .UBSAN_BOUNDS (0B, b.0, 8);
        _1 = a[b.0];

    therefore we emit a runtime error before making the bad array access.

    I think it's OK that only the RHS gets a .UBSAN_BOUNDS, as in few lines
    above: the instrumented array access dominates the array access on the
    LHS, and I've verified that

      b = 0;
      a[b] = (a[b], b = -32768, a[0] | c);

    works as expected: the inner a[b] is OK but we do emit an error for the
    a[b] on the LHS.

    For GCC 14, we could apply
    <https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613687.html>
    since the copy_node doesn't seem to be needed.

            PR sanitizer/108060
            PR sanitizer/109050

    gcc/c-family/ChangeLog:

            * c-gimplify.cc (ubsan_walk_array_refs_r): For a MODIFY_EXPR,
instrument
            the RHS before the LHS.

    gcc/testsuite/ChangeLog:

            * c-c++-common/ubsan/bounds-17.c: New test.
            * c-c++-common/ubsan/bounds-18.c: New test.
            * c-c++-common/ubsan/bounds-19.c: New test.
            * c-c++-common/ubsan/bounds-20.c: New test.
            * c-c++-common/ubsan/bounds-21.c: New test.

[Bug sanitizer/108060] [10/11/12 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11/12/13 Regression]    |[10/11/12 Regression] UBsan
                   |UBsan missed an             |missed an out-of-bound bug
                   |out-of-bound bug at -O0     |at -O0 since
                   |since                       |r7-1900-g8a1b7b7fd75a3847
                   |r7-1900-g8a1b7b7fd75a3847   |

--- Comment #9 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed on trunk so far.

[Bug sanitizer/108060] [10/11/12 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Marek Polacek
<mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:94af33aa4da07269cb4a6645da9f7ddf8d1bad69

commit r12-9264-g94af33aa4da07269cb4a6645da9f7ddf8d1bad69
Author: Marek Polacek <polacek@redhat.com>
Date:   Wed Mar 8 09:15:07 2023 -0500

    ubsan: missed -fsanitize=bounds for compound ops [PR108060]

    In this PR we are dealing with a missing .UBSAN_BOUNDS, so the
    out-of-bounds access in the test makes the program crash before
    a UBSan diagnostic was emitted.  In C and C++, c_genericize gets

      a[b] = a[b] | c;

    but in C, both a[b] are one identical shared tree (not in C++ because
    cp_fold/ARRAY_REF created two same but not identical trees).  Since
    ubsan_walk_array_refs_r keeps a pset, in C we produce

      a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] = a[b] | c;

    because the LHS is walked before the RHS.

    Since r7-1900, we gimplify the RHS before the LHS.  So the statement above
    gets gimplified into

        _1 = a[b];
        c.0_2 = c;
        b.1 = b;
        .UBSAN_BOUNDS (0B, b.1, 8);

    With this patch we produce:

      a[b] = a[.UBSAN_BOUNDS (0B, SAVE_EXPR <b>, 8);, SAVE_EXPR <b>;] | c;

    which gets gimplified into:

        b.0 = b;
        .UBSAN_BOUNDS (0B, b.0, 8);
        _1 = a[b.0];

    therefore we emit a runtime error before making the bad array access.

    I think it's OK that only the RHS gets a .UBSAN_BOUNDS, as in few lines
    above: the instrumented array access dominates the array access on the
    LHS, and I've verified that

      b = 0;
      a[b] = (a[b], b = -32768, a[0] | c);

    works as expected: the inner a[b] is OK but we do emit an error for the
    a[b] on the LHS.

    For GCC 14, we could apply
    <https://gcc.gnu.org/pipermail/gcc-patches/2023-March/613687.html>
    since the copy_node doesn't seem to be needed.

            PR sanitizer/108060
            PR sanitizer/109050

    gcc/c-family/ChangeLog:

            * c-gimplify.cc (ubsan_walk_array_refs_r): For a MODIFY_EXPR,
instrument
            the RHS before the LHS.

    gcc/testsuite/ChangeLog:

            * c-c++-common/ubsan/bounds-17.c: New test.
            * c-c++-common/ubsan/bounds-18.c: New test.
            * c-c++-common/ubsan/bounds-19.c: New test.
            * c-c++-common/ubsan/bounds-20.c: New test.
            * c-c++-common/ubsan/bounds-21.c: New test.

    (cherry picked from commit 4d0baeae315ebe7d0ec7682ea3e7c0516027c2b8)

[Bug sanitizer/108060] [10/11 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
            Summary|[10/11/12 Regression] UBsan |[10/11 Regression] UBsan
                   |missed an out-of-bound bug  |missed an out-of-bound bug
                   |at -O0 since                |at -O0 since
                   |r7-1900-g8a1b7b7fd75a3847   |r7-1900-g8a1b7b7fd75a3847
             Status|ASSIGNED                    |RESOLVED

--- Comment #11 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Fixed.

[Bug sanitizer/108060] [10/11 Regression] UBsan missed an out-of-bound bug at -O0 since r7-1900-g8a1b7b7fd75a3847

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.5                        |12.3