[Bug sanitizer/59258] New: usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[Bug sanitizer/59258] New: usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

            Bug ID: 59258
           Summary: usan: ICE(segfault): stack-buffer-overflow with
                    -fsanitize=undefined
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Keywords: ice-on-valid-code
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                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,
                    mpolacek at gcc dot gnu.org

I run into this with a bigger code and GCC 4.9 r205287 on x86-64-gnu-linux.
Using an about one-week old GCC, it compiled successfully - but now it
segfaults.

I have now created a -fsanitize=address GCC build, which allowed me to reduce
the code (before, I couldn't go smaller than about 64k).

In any case, GCC now fails for:
  cc1plus -fsanitize=undefined
for this 5-line program:

class Parent { };
class RefNameDict : public Parent {
  explicit RefNameDict (bool uniqueNames);
};
RefNameDict::RefNameDict (bool uniqueNames) { }


Address sanitizer prints:

==8820==ERROR: AddressSanitizer: stack-buffer-overflow on address
0x7fffcc257f58 at pc 0x1768edf bp 0x7fffcc257e60 sp 0x7fffcc257e58
WRITE of size 8 at 0x7fffcc257f58 thread T0
    #0 ubsan_create_data(char const*, unsigned int, ubsan_mismatch_data const*,
...) + 2493 in section .text
    #1 ubsan_expand_null_ifn(gimple_stmt_iterator_d) + 2283 in section .text
    #2 execute_sanopt() + 838 in section .text
    #3 (anonymous namespace)::pass_sanopt::execute() + 8 in section .text
...
Address 0x7fffcc257f58 is located in stack of thread T0 at offset 184 in frame
    #0 ubsan_create_data(char const*, unsigned int, ubsan_mismatch_data const*,
...) + 15 in section .text
  This frame has 5 object(s):
    [32, 40) 'saved_args'
    [96, 104) 'v'
    [160, 184) 'fields' <== Memory access at offset 184 overflows this variable
    [224, 248) 'args'
    [288, 320) 'tmp_name'

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #2 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #1)
> Ugh, of course.  This should fix it.  Thanks for the report.

Seems to work for the reduced test case but not for the big code - also
-fsanitize=addr can only detect a segfault. Hopefully, it can be reduced to
something small. Currently, it looks as if it could be a similar kind of bug,
which makes reducing difficult.


> --- a/gcc/ubsan.c
> +++ b/gcc/ubsan.c

How about also adding:

--- a/gcc/ubsan.c
+++ b/gcc/ubsan.c
@@ -468,4 +468,5 @@ ubsan_create_data (const char *name, location_t loc,
   for (i = 0; i < nelts; i++)
     {
+      gcc_checking_assert (i < 5);
       t = (*saved_args)[i];

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Why?  We are already checking that nelts is < 3.  See the gcc_checking_assert
above.

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #2)
> Seems to work for the reduced test case but not for the big code - also
> -fsanitize=addr can only detect a segfault. Hopefully, it can be reduced to
> something small. Currently, it looks as if it could be a similar kind of
> bug, which makes reducing difficult.

Still not much success with reducing (~90 kB), but it seems to be due to
-fsanitize=null.


Except for staring at the -fsanitize=null code, I don't have any good idea how
to debug this - valgrind and an -fsanitized=address instrumented GCC don't
help.

Do you have any suggestion?

BTW: The 90kB code fails due to with a segfault for "cc1plus -fsanitize=null
-Og -fsanitize=address" with

0x0000000000e7a877 in resolve_addr_in_expr (loc=0xb60f7fff8000c181) at
../../gcc/dwarf2out.c:22974
22974       switch (loc->dw_loc_opc)
#1  0x0000000000e7c209 in resolve_addr (die=0x7ffff2704c80) at
../../gcc/dwarf2out.c:23203
23203                   if (!resolve_addr_in_expr ((*curr)->expr))

(gdb) p *loc
Cannot access memory at address 0xb60f7fff8000c181

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #5 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #3)
> Why?  We are already checking that nelts is < 3.  See the
> gcc_checking_assert above.

Well, at least to me it is not obvious whether it can get larger.

size_t i = 0;
  if (loc != UNKNOWN_LOCATION)
    i++
  for (t = va_arg (args, tree); t != NULL_TREE;
       i++, t = va_arg (args, tree))
      gcc_checking_assert (i < 3);
      vec_safe_push (saved_args, t);

Thus, saved_args probably has 0, 1 or 2 entries but with loc ==
UNKNOWN_LOCATION it could have 3.

  if (mismatch != NULL)
      i++;
  for (i = 0; i < nelts; i++)
      t = (*saved_args)[i];

If mismatch != NULL, (number of saved_args) == 2 and loc != UNKNOWN_LOCATION,
one has: 1+1+2*2 = 6 array elements, but only space for 5; with loc ==
UNKNOWN_LOCATION  one could even have 0+1+2*(3) = 7.

I don't find it obvious that 5 is sufficient, even if closer inspection will
shows that this is always the case.

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #7 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #5)
> I don't find it obvious that 5 is sufficient, even if closer inspection will
> shows that this is always the case.

At worst, the fields array can have 5 elements: the location, two type
descriptors, alignment and the kind.

[Bug sanitizer/59258] usan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #8 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Author: mpolacek
Date: Mon Nov 25 10:46:20 2013
New Revision: 205347

URL: http://gcc.gnu.org/viewcvs?rev=205347&root=gcc&view=rev
Log:
2013-11-25  Marek Polacek  <polacek@redhat.com>

    * ubsan.c (ubsan_create_data): Increase the size of the fields array.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ubsan.c

[Bug sanitizer/59258] ubsan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[mpolacek at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #9 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #6)
> Seems like we're passing bogus location.  Can you e-mail me the unreduced
> testcase?  I'll look into it.

The unreduced testcase doesn't fail with -gstrict-dwarf.

[Bug sanitizer/59258] ubsan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 31289
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31289&action=edit
gcc49-pr59258.patch

Untested fix for the dwarf2out.c ICE.  The problem is that -fsanitize=null
will preserve in location_t of ADDR_EXPR a reference to a block, but the BLOCK
isn't referenced from anywhere else and thus during ggc_collect removed as
unused, then dwarf2out.c allocates the same GC memory for something else and
finally when processing the ADDR_EXPR the old BLOCK is modified again.

Either of the hunks in the patch should fix this, IMHO we want both, it doesn't
make sense to provide any locus for the ADDR_EXPR of the string in the static
ctor, and as ubsan_create_data compares loc to UNKNOWN_LOCATION in a bunch of
places, not doing LOCATION_LOCUS there means it might not compare to
UNKNOWN_LOCATION even when it actually is UNKNOWN_LOCATION just with some
non-NULL associated block.  Perhaps my
        * ubsan.c (ubsan_source_location): Don't crash on unknown locations.
change will be unneeded now?

[Bug sanitizer/59258] ubsan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Author: jakub
Date: Tue Nov 26 21:27:19 2013
New Revision: 205415

URL: http://gcc.gnu.org/viewcvs?rev=205415&root=gcc&view=rev
Log:
    PR sanitizer/59258
    * ubsan.c (ubsan_source_location): Don't add any location
    to ADDR_EXPR in the ctor.  Revert 2013-11-22 change.
    (ubsan_create_data): Strip block info from LOC.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ubsan.c

[Bug sanitizer/59258] ubsan: ICE(segfault): stack-buffer-overflow with -fsanitize=undefined

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59258

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

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

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.