[Bug sanitizer/105714] New: ASan in gcc trunk missed a buffer-overflow at -Os

[Bug sanitizer/105714] New: ASan in gcc trunk missed a buffer-overflow at -Os

[shaohua.li at inf dot ethz.ch]

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

            Bug ID: 105714
           Summary: ASan in gcc trunk missed a buffer-overflow at -Os
           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, `gcc-trunk -Os -fsanitize=address` reported nothing,
however other opt levels reported the global buffer-overflow at the end of
function g().

Initially, I thought it might due to optimization at -Os, but I indeed found
the overflowed buffer be loaded in the assembly code and gcc-11 -Os did not
miss this bug: https://godbolt.org/z/r4rhM8bjz 

$cat a.c
struct a {
  int x
};
struct a b[2];
struct a *c = b, *d = b;
int e;

int g() {
  for (e = 0; e < 1; e++) {
    int i[1];
    i;
  }
  for (int h = 0; h < 3; h++)
    *c = *d;
  *c = *(b+3);
  return c->x;
}

void main() { 
    g(); 
}
$
$gcc-trunk -Os -fsanitize=address a.c && ./a.out
$
$gcc-trunk -O3 -fsanitize=address a.c && ./a.out
==12272==ERROR: AddressSanitizer: global-buffer-overflow on address
0x0000004042ac at pc 0x00000040132a bp 0x7ffdbc905820 sp 0x7ffdbc905818
READ of size 4 at 0x0000004042ac thread T0
    #0 0x401329 in g /local/home/shaoli/a.c:15
    #1 0x7fc367b2f082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082) (BuildId:
1878e6b475720c7c51969e69ab2d276fae6d1dee)
    #2 0x40111d in _start (/local/home/shaoli/a.out+0x40111d)

0x0000004042ac is located 4 bytes to the right of global variable 'b' defined
in 'a.c:4:10' (0x4042a0) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow /local/home/shaoli/a.c:15 in
g
Shadow bytes around the buggy address:
  0x000080078800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080078810: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x000080078820: 00 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080078830: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080078840: f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 04 f9 f9 f9
=>0x000080078850: f9 f9 f9 f9 00[f9]f9 f9 f9 f9 f9 f9 00 00 00 00
  0x000080078860: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080078870: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080078880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x000080078890: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0000800788a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==12272==ABORTING

[Bug sanitizer/105714] [12/13 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-05-24
            Summary|ASan in gcc trunk missed a  |[12/13 Regression] ASan in
                   |buffer-overflow at -Os      |gcc trunk missed a
                   |                            |buffer-overflow at -Os
                   |                            |since
                   |                            |r12-5138-ge82c382971664d6f
                 CC|                            |aldyh at gcc dot gnu.org,
                   |                            |amacleod at redhat dot com
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Confirmed, started with r12-5138-ge82c382971664d6f.

[Bug sanitizer/105714] [12/13 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I'll have a look...

[Bug sanitizer/105714] [12/13 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The problem is we have:
  <bb 6> [local count: 354334800]:
  # h_21 = PHI <h_15(6), 0(5)>
  *c.3_5 = *d.2_4;
  h_15 = h_21 + 1;
  if (h_15 != 3)
    goto <bb 6>; [75.00%]
  else
    goto <bb 7>; [25.00%]

  <bb 7> [local count: 118111600]:
  *c.3_5 = MEM[(struct a *)&b + 12B];
  _13 = c.3_5->x;
  return _13;
We instrument in the *c.3_5 = *d.2_4 both the load and store, but then
when considering instrumentation of *c.3_5 = MEM[(struct a *)&b + 12B];
has_stmt_been_instrumented_p returns true because *c.3_5 has been indeed
instrumented in a dominating bb.  But the rhs hasn't been...

[Bug sanitizer/105714] [12/13 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 53028
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53028&action=edit
gcc13-pr105714.patch

Untested fix.

[Bug sanitizer/105714] [12/13 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:af02daff557a0abbf5521c143f1cdda406848a9b

commit r13-756-gaf02daff557a0abbf5521c143f1cdda406848a9b
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed May 25 12:05:08 2022 +0200

    asan: Fix up instrumentation of assignments which are both loads and stores
[PR105714]

    On the following testcase with -Os asan pass sees:
      <bb 6> [local count: 354334800]:
      # h_21 = PHI <h_15(6), 0(5)>
      *c.3_5 = *d.2_4;
      h_15 = h_21 + 1;
      if (h_15 != 3)
        goto <bb 6>; [75.00%]
      else
        goto <bb 7>; [25.00%]

      <bb 7> [local count: 118111600]:
      *c.3_5 = MEM[(struct a *)&b + 12B];
      _13 = c.3_5->x;
      return _13;
    It instruments the
      *c.3_5 = *d.2_4;
    assignment by adding
      .ASAN_CHECK (7, c.3_5, 4, 4);
      .ASAN_CHECK (6, d.2_4, 4, 4);
    before it (which later lowers to checking the corresponding shadow
    memory).  But when considering instrumentation of
      *c.3_5 = MEM[(struct a *)&b + 12B];
    it doesn't instrument anything, because it sees that *c.3_5 store is
    already instrumented in a dominating block and so there is no need
    to instrument *c.3_5 store again (i.e. add another
      .ASAN_CHECK (7, c.3_5, 4, 4);
    ).  That is true, but misses the fact that we still want to
    instrument the MEM[(struct a *)&b + 12B] load.

    The following patch fixes that by changing has_stmt_been_instrumented_p
    to consider both store and load in the assignment if it does both
    (returning true iff both have been instrumented).
    That matches how we handle e.g. builtin calls, where we also perform AND
    of all the memory locs involved in the call.

    I've verified that we still don't add the redundant
      .ASAN_CHECK (7, c.3_5, 4, 4);
    call but just add
      _18 = &MEM[(struct a *)&b + 12B];
      .ASAN_CHECK (6, _18, 4, 4);
    to instrument the load.

    2022-05-25  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/105714
            * asan.cc (has_stmt_been_instrumented_p): For assignments which
            are both stores and loads, return true only if both destination
            and source have been instrumented.

            * gcc.dg/asan/pr105714.c: New test.

[Bug sanitizer/105714] [12 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[12/13 Regression] ASan in  |[12 Regression] ASan in gcc
                   |gcc trunk missed a          |trunk missed a
                   |buffer-overflow at -Os      |buffer-overflow at -Os
                   |since                       |since
                   |r12-5138-ge82c382971664d6f  |r12-5138-ge82c382971664d6f

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.
The bug really exists in older releases too, just this testcase doesn't trigger
it in gcc 11 or older.

[Bug sanitizer/105714] [12 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:120d99a3ec3f13766e13b40e48b0614242447fea

commit r12-8433-g120d99a3ec3f13766e13b40e48b0614242447fea
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed May 25 12:05:08 2022 +0200

    asan: Fix up instrumentation of assignments which are both loads and stores
[PR105714]

    On the following testcase with -Os asan pass sees:
      <bb 6> [local count: 354334800]:
      # h_21 = PHI <h_15(6), 0(5)>
      *c.3_5 = *d.2_4;
      h_15 = h_21 + 1;
      if (h_15 != 3)
        goto <bb 6>; [75.00%]
      else
        goto <bb 7>; [25.00%]

      <bb 7> [local count: 118111600]:
      *c.3_5 = MEM[(struct a *)&b + 12B];
      _13 = c.3_5->x;
      return _13;
    It instruments the
      *c.3_5 = *d.2_4;
    assignment by adding
      .ASAN_CHECK (7, c.3_5, 4, 4);
      .ASAN_CHECK (6, d.2_4, 4, 4);
    before it (which later lowers to checking the corresponding shadow
    memory).  But when considering instrumentation of
      *c.3_5 = MEM[(struct a *)&b + 12B];
    it doesn't instrument anything, because it sees that *c.3_5 store is
    already instrumented in a dominating block and so there is no need
    to instrument *c.3_5 store again (i.e. add another
      .ASAN_CHECK (7, c.3_5, 4, 4);
    ).  That is true, but misses the fact that we still want to
    instrument the MEM[(struct a *)&b + 12B] load.

    The following patch fixes that by changing has_stmt_been_instrumented_p
    to consider both store and load in the assignment if it does both
    (returning true iff both have been instrumented).
    That matches how we handle e.g. builtin calls, where we also perform AND
    of all the memory locs involved in the call.

    I've verified that we still don't add the redundant
      .ASAN_CHECK (7, c.3_5, 4, 4);
    call but just add
      _18 = &MEM[(struct a *)&b + 12B];
      .ASAN_CHECK (6, _18, 4, 4);
    to instrument the load.

    2022-05-25  Jakub Jelinek  <jakub@redhat.com>

            PR sanitizer/105714
            * asan.cc (has_stmt_been_instrumented_p): For assignments which
            are both stores and loads, return true only if both destination
            and source have been instrumented.

            * gcc.dg/asan/pr105714.c: New test.

    (cherry picked from commit af02daff557a0abbf5521c143f1cdda406848a9b)

[Bug sanitizer/105714] [12 Regression] ASan in gcc trunk missed a buffer-overflow at -Os since r12-5138-ge82c382971664d6f

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 12.2 too.