[Bug sanitizer/106558] New: ASan failed to detect a global-buffer-overflow

[Bug sanitizer/106558] New: ASan failed to detect a global-buffer-overflow

[shaohua.li at inf dot ethz.ch]

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

            Bug ID: 106558
           Summary: ASan failed to detect a global-buffer-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, `gcc-trunk -O1 -fsanitize=address` failed to detect the
global-buffer-overflow, while other opt flags (-O0, -O2, and -O3) can.

$cat a.c
int a;
int *b = &a;
int **c = &b;
int d[1];
int *e = &d[1];

static int f(int *g) {
  *b = e;
  *c = e;
  *b = 2;
  *g = 2;
}
int main() { 
    f(b); 
    return *b;
}
$
$gcc-trunk -O1 -fsanitize=address -w -g && ./a.out
$
$gcc-trunk -O3 -fsanitize=address -w -g && ./a.out
=================================================================
==1==ERROR: AddressSanitizer: global-buffer-overflow on address 0x000000404304
at pc 0x00000040110d bp 0x7ffffc438ae0 sp 0x7ffffc438ad8
WRITE of size 4 at 0x000000404304 thread T0
    #0 0x40110c in f /app/a.c:10
    #1 0x40110c in main /app/a.c:14
    #2 0x7f9bc6e1a0b2 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x240b2) (BuildId:
9fdb74e7b217d06c93172a8243f8547f947ee6d1)
    #3 0x40117d in _start (/app/output.s+0x40117d) (BuildId:
55182140539c37abf57e49748b511b560966f7c4)

0x000000404304 is located 60 bytes to the left of global variable 'a' defined
in '/app/a.c:1:5' (0x404340) of size 4
0x000000404304 is located 0 bytes to the right of global variable 'd' defined
in '/app/a.c:4:5' (0x404300) of size 4
SUMMARY: AddressSanitizer: global-buffer-overflow /app/a.c:10 in f
Shadow bytes around the buggy address:
  0x000080078810: 00 f9 f9 f9 f9 f9 f9 f9 00 f9 f9 f9 f9 f9 f9 f9
  0x000080078820: 00 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x000080078830: 00 00 00 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080078840: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9
  0x000080078850: f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
=>0x000080078860:[04]f9 f9 f9 f9 f9 f9 f9 04 f9 f9 f9 f9 f9 f9 f9
  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
  0x0000800788b0: 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
==1==ABORTING

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
With -fno-toplevel-reorder, it can be detected.
I can't figure out why there is a difference really.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-08-09
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Might be related to PR 82501.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Looks like a bug in the sanopt pass.
For -O2, we have before sanopt in main:
  b.0_1 = b;
  e.2_3 = e;
  c.5_4 = c;
  .ASAN_CHECK (7, c.5_4, 8, 8);
  *c.5_4 = e.2_3;
  b.7_5 = b;
  .ASAN_CHECK (7, b.7_5, 4, 4);
  *b.7_5 = 2;
  .ASAN_CHECK (7, b.0_1, 4, 4);
  *b.0_1 = 2;
  return 2;
and in sanopt:
Leaving: .ASAN_CHECK (7, c.5_4, 8, 8);
Leaving: .ASAN_CHECK (7, b.7_5, 4, 4);
Optimizing out: .ASAN_CHECK (7, b.0_1, 4, 4);
Expanded: .ASAN_CHECK (7, c.5_4, 8, 8);
Expanded: .ASAN_CHECK (7, b.7_5, 4, 4);
Even that is incorrect, we don't generally know what b points to before the *c
store and after it, so neither of the stores is redundant because *c store can
change the value of b.
At -O1 we have before sanopt:
  b.0_1 = b;
  e.2_6 = e;
  e.3_7 = (long int) e.2_6;
  _9 = (int) e.3_7;
  .ASAN_CHECK (7, b.0_1, 4, 4);
  *b.0_1 = _9;
  c.5_10 = c;
  e.6_11 = e;
  .ASAN_CHECK (7, c.5_10, 8, 8);
  *c.5_10 = e.6_11;
  b.7_12 = b;
  .ASAN_CHECK (7, b.7_12, 4, 4);
  *b.7_12 = 2;
  *b.0_1 = 2;
  b.1_2 = b;
  .ASAN_CHECK (6, b.1_2, 4, 4);
  _5 = *b.1_2;
  return _5;
because we optimize less at that optimization level, and sanopt:
Leaving: .ASAN_CHECK (7, b.0_1, 4, 4);
Leaving: .ASAN_CHECK (7, c.5_10, 8, 8);
Optimizing out: .ASAN_CHECK (7, b.7_12, 4, 4);
Optimizing out: .ASAN_CHECK (6, b.1_2, 4, 4);
Expanded: .ASAN_CHECK (7, b.0_1, 4, 4);
Expanded: .ASAN_CHECK (7, c.5_10, 8, 8);
The b.1_2 .ASAN_CHECK is IMHO redundant (b couldn't change between b.7_12 = b
and b.1_2 = b;) but the b.7_12 .ASAN_CHECK is not redundant.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
This is due to the
https://gcc.gnu.org/legacy-ml/gcc-patches/2014-12/msg00242.html
r5-5530-ge28f2090dbbb5072 optimization, which is incorrect.
If we want to track pointers which live in memory, we'd need to ask the alias
oracle on each store or call whether the store or call couldn't change the
value of such a pointer and if yes, invalidate them.
Not really sure it is worth it though.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[tetra2005 at gmail dot com]

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

Yuri Gribov <tetra2005 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tetra2005 at gmail dot com

--- Comment #5 from Yuri Gribov <tetra2005 at gmail dot com> ---
Ok, seems my 2014 patch will need to be reverted then. Do you want me to submit
a PR?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Or perhaps could we ask the alias oracle in can_remove_asan_check
for the *base_checks case if base_addr lives in memory whether base_addr could
change in between the stmt in the vector and current stmt, with some low limit
on  how much we walk to find that out?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Perhaps either a quick check that for base ptrs that live in memory gimple_vuse
is the same for both statements or if not, do walk_aliased_vdefs with low
constant limit?
We'd want to stop if we reach the vdef of the stmt in base_checks vector (then
we didn't find anything that could clobber it and can therefore use the cached
check) or when we see a stmt that may clobber it (then we can't use the cached
check).

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[tetra2005 at gmail dot com]

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

--- Comment #8 from Yuri Gribov <tetra2005 at gmail dot com> ---
(In reply to Jakub Jelinek from comment #7)

I've started work on this but I'll probly only have enough time to cook a patch
on weekend.

> Perhaps either a quick check that for base ptrs that live in memory

A silly question, such cases (base_addrs living in memory) can be identified by
  gimple *g = SSA_NAME_DEF_STMT (t);
in maybe_get_single_definition having vuses?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
If maybe_get_single_definition returns a SSA_NAME or is_gimple_min_invariant,
then it is ok as is and doesn't need anything new.
Otherwise I think we need to ask the alias oracle.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[ygribov at gcc dot gnu.org]

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

--- Comment #10 from Yury Gribov <ygribov at gcc dot gnu.org> ---
Created attachment 53458
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53458&action=edit
Very draft patch

(In reply to Jakub Jelinek from comment #7)
> Perhaps either a quick check that for base ptrs that live in memory
> gimple_vuse is the same for both statements or if not, do walk_aliased_vdefs
> with low constant limit?
> We'd want to stop if we reach the vdef of the stmt in base_checks vector
> (then we didn't find anything that could clobber it and can therefore use
> the cached check) or when we see a stmt that may clobber it (then we can't
> use the cached check).

Something like this? It does not help with b.1_2 in attached reprocase though,
because alias oracle considers
  *b.0_1 = 2;
to clobber it.

I'm trying to collect statistics how many checks this optimization removes
during bootstrap-asan but I'm getting crashes when asan-bootstrapping on
unchanged trunk. Is this possible?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[ygribov at gcc dot gnu.org]

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

Yury Gribov <ygribov at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #53458|0                           |1
        is obsolete|                            |
  Attachment #53493|0                           |1
        is obsolete|                            |

--- Comment #12 from Yury Gribov <ygribov at gcc dot gnu.org> ---
Created attachment 53530
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53530&action=edit
Final patch

Attached patch passes bootstrap (regular and asan) and regtesting and, as
explained above, results in very small <1% reduction of optimizations. If there
are no objections, I'll post it to gcc-patches.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[ygribov at gcc dot gnu.org]

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

Yury Gribov <ygribov at gcc dot gnu.org> changed:

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

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[tetra2005 at gmail dot com]

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

--- Comment #13 from Yuri Gribov <tetra2005 at gmail dot com> ---
Posted to gcc-patches:
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/601041.html

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[shaohua.li at inf dot ethz.ch]

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

--- Comment #14 from Li Shaohua <shaohua.li at inf dot ethz.ch> ---
Hello, is this patch going to be pushed to the trunk?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

--- Comment #15 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Li Shaohua from comment #14)
> Hello, is this patch going to be pushed to the trunk?

Not yet. The patch is under review process.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

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

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[tetra2005 at gmail dot com]

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

--- Comment #17 from Yuri Gribov <tetra2005 at gmail dot com> ---
Fix has been approved
(https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606858.html), I hope
to merge it soon.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

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

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[cvs-commit at gcc dot gnu.org]

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

--- Comment #19 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Liska <marxin@gcc.gnu.org>:

https://gcc.gnu.org/g:5dd4d2e93e3de60d4ef1068b6dfd06b6b9fff16e

commit r13-4354-g5dd4d2e93e3de60d4ef1068b6dfd06b6b9fff16e
Author: Yuri Gribov <y.gribov@samsung.com>
Date:   Sun Aug 14 08:42:44 2022 +0300

    asan: fix unsafe optimization of Asan checks.

            PR sanitizer/106558

    gcc/
            * sanopt.cc: Do not optimize out checks for non-SSA addresses.

    gcc/testsuite/
            * c-c++-common/asan/pr106558.c: New test.

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

--- Comment #20 from Martin Liška <marxin at gcc dot gnu.org> ---
Fixed on master. Do we want to do a backport?

[Bug sanitizer/106558] ASan failed to detect a global-buffer-overflow

[marxin at gcc dot gnu.org]

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

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