[Bug c++/94325] New: [UBSAN] "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[Bug c++/94325] New: [UBSAN] "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[jaroslaw.melzer.gcc at gmail dot com]

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

            Bug ID: 94325
           Summary: [UBSAN] "invalid vptr" false positive for virtual
                    inheritance with -fno-sanitize-recover=all
           Product: gcc
           Version: 9.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jaroslaw.melzer.gcc at gmail dot com
  Target Milestone: ---

Created attachment 48111
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48111&action=edit
reproducer code

See attached: ubsan-gcc.ii

Program ubsan-gcc compiled with options:
g++ -fsanitize=undefined -fno-sanitize-recover=all ubsan-gcc.ii

exits with following false positive:

ubsan-gcc.cpp:12:7: runtime error: member call on address 0x7ffcdd8c9800 which
does not point to an object of type 'DE'
0x7ffcdd8c9800: note: object has invalid vptr
 17 56 00 00  00 00 00 00 00 00 00 00  00 76 43 93 7b 4b c2 bf  30 71 eb 2f 17
56 00 00  e3 41 18 cc
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr

May be related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87095

This error doesn't manifest without -fno-sanitize-recover=all


gcc -v

Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-linux-gnu/9/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none:hsa
OFFLOAD_TARGET_DEFAULT=1
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 9.2.1-9ubuntu2'
--with-bugurl=file:///usr/share/doc/gcc-9/README.Bugs
--enable-languages=c,ada,c++,go,brig,d,fortran,objc,obj-c++,gm2 --prefix=/usr
--with-gcc-major-version-only --program-suffix=-9
--program-prefix=x86_64-linux-gnu- --enable-shared --enable-linker-build-id
--libexecdir=/usr/lib --without-included-gettext --enable-threads=posix
--libdir=/usr/lib --enable-nls --enable-bootstrap --enable-clocale=gnu
--enable-libstdcxx-debug --enable-libstdcxx-time=yes
--with-default-libstdcxx-abi=new --enable-gnu-unique-object
--disable-vtable-verify --enable-plugin --enable-default-pie --with-system-zlib
--with-target-system-zlib=auto --enable-multiarch --disable-werror
--with-arch-32=i686 --with-abi=m64 --with-multilib-list=m32,m64,mx32
--enable-multilib --with-tune=generic --enable-offload-targets=nvptx-none,hsa
--without-cuda-driver --enable-checking=release --build=x86_64-linux-gnu
--host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2)

[Bug sanitizer/94325] [UBSAN] "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.0, 9.3.0
      Known to work|                            |7.4.0
     Ever confirmed|0                           |1
           Keywords|                            |needs-bisection
   Last reconfirmed|                            |2020-03-25
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Martin Liška <marxin at gcc dot gnu.org> ---
Confirmed, I'm bisecting that.

[Bug sanitizer/94325] [8/9/10 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jason at gcc dot gnu.org,
                   |                            |nathan at gcc dot gnu.org
           Keywords|needs-bisection             |
            Summary|[UBSAN] "invalid vptr"      |[8/9/10 Regression] UBSAN:
                   |false positive for virtual  |"invalid vptr" false
                   |inheritance with            |positive for virtual
                   |-fno-sanitize-recover=all   |inheritance with
                   |                            |-fno-sanitize-recover=all
      Known to fail|                            |8.4.0

--- Comment #2 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with my r8-4602-g896f6b3dfa6fb337109f97bed8d74c1e030c965e.
Anyway, a help from C++ maintainer is more than welcomed.

[Bug sanitizer/94325] [8/9/10 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |8.5

[Bug sanitizer/94325] [8/9/10 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[jakub at gcc dot gnu.org]

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

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
sizeof (MD) == sizeof (void *), so the clearing of the vptr in DD::~DD() when
DE::~DE() is invoked later on looks wrong.
But, without -fsanitize=vptr this isn't done.
I've added that guard in PR87095, see
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87095#c3
for more details.
So, either we can just do what build_clobber_this does, which would be just:
--- gcc/cp/decl.c       2020-03-27 09:59:26.407083563 +0100
+++ gcc/cp/decl.c       2020-04-06 13:14:15.814200485 +0200
@@ -16662,10 +16662,7 @@ begin_destructor_body (void)
            /* If the vptr is shared with some virtual nearly empty base,
               don't clear it if not in charge, the dtor of the virtual
               nearly empty base will do that later.  */
-           if (CLASSTYPE_VBASECLASSES (current_class_type)
-               && CLASSTYPE_PRIMARY_BINFO (current_class_type)
-               && BINFO_VIRTUAL_P
-                         (CLASSTYPE_PRIMARY_BINFO (current_class_type)))
+           if (CLASSTYPE_VBASECLASSES (current_class_type))
              {
                stmt = convert_to_void (stmt, ICV_STATEMENT,
                                        tf_warning_or_error);
or try to narrow down the condition to cover this problematic case too, though
I'm afraid I have no idea what that would be.
Nathan/Jason, can I ask for help here?

A wild shot in the dark could be:
--- gcc/cp/decl.c.jj    2020-03-27 09:59:26.407083563 +0100
+++ gcc/cp/decl.c       2020-04-06 13:25:03.321511554 +0200
@@ -16662,14 +16662,20 @@ begin_destructor_body (void)
            /* If the vptr is shared with some virtual nearly empty base,
               don't clear it if not in charge, the dtor of the virtual
               nearly empty base will do that later.  */
-           if (CLASSTYPE_VBASECLASSES (current_class_type)
-               && CLASSTYPE_PRIMARY_BINFO (current_class_type)
-               && BINFO_VIRTUAL_P
-                         (CLASSTYPE_PRIMARY_BINFO (current_class_type)))
+           if (CLASSTYPE_VBASECLASSES (current_class_type))
              {
-               stmt = convert_to_void (stmt, ICV_STATEMENT,
-                                       tf_warning_or_error);
-               stmt = build_if_in_charge (stmt);
+               tree c = current_class_type;
+               while (CLASSTYPE_PRIMARY_BINFO (c))
+                 {
+                   if (BINFO_VIRTUAL_P (CLASSTYPE_PRIMARY_BINFO (c)))
+                     {
+                       stmt = convert_to_void (stmt, ICV_STATEMENT,
+                                               tf_warning_or_error);
+                       stmt = build_if_in_charge (stmt);
+                       break;
+                     }
+                   c = BINFO_TYPE (CLASSTYPE_PRIMARY_BINFO (c));
+                 }
              }
            finish_decl_cleanup (NULL_TREE, stmt);
          }
just from the fact that the existing code seemed to DTRT for D::~D() where
CLASSTYPE_VBASECLASSES is non-NULL, CLASSTYPE_PRIMARY_BINFO too and
BINFO_VIRTUAL_P on that is set as well.
But in DD::~DD() the last condition doesn't hold, but BINFO_TYPE of the
CLASSTYPE_PRIMARY_BINFO is D which has this property.

[Bug sanitizer/94325] [8/9/10 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[jakub at gcc dot gnu.org]

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

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

The shot in the dark in whole patch form.

[Bug sanitizer/94325] [8/9/10 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[cvs-commit at gcc dot gnu.org]

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

--- 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:4cf6b06cb5b02c053738e2975e3b7a4b3c577401

commit r10-7620-g4cf6b06cb5b02c053738e2975e3b7a4b3c577401
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 8 15:30:16 2020 +0200

    c++: Further fix for -fsanitize=vptr [PR94325]

    For -fsanitize=vptr, we insert a NULL store into the vptr instead of just
    adding a CLOBBER of this.  build_clobber_this makes the CLOBBER conditional
    on in_charge (implicit) parameter whenever CLASSTYPE_VBASECLASSES, but when
    adding this conditionalization to the -fsanitize=vptr code in PR87095,
    I wanted it to catch some more cases when the class has
CLASSTYPE_VBASECLASSES,
    but the vptr is still not shared with something else, otherwise the
    sanitization would be less effective.
    The following testcase shows that the chosen test that
CLASSTYPE_PRIMARY_BINFO
    is non-NULL and has BINFO_VIRTUAL_P set wasn't sufficient,
    the D class has still sizeof(D) == sizeof(void*) and thus contains just
    a single vptr, but while in B::~B() this results in the vptr not being
    cleared, in C::~C() this condition isn't true, as CLASSTYPE_PRIMARY_BINFO
    in that case is B and is not BINFO_VIRTUAL_P, so it clears the vptr, but
the
    D::~D() dtor after invoking C::~C() invokes A::~A() with an already cleared
    vptr, which is then reported.
    The following patch is just a shot in the dark, keep looking through
    CLASSTYPE_PRIMARY_BINFO until we find BINFO_VIRTUAL_P, but it works on the
    existing testcase as well as this new one.

    2020-04-08  Jakub Jelinek  <jakub@redhat.com>

            PR c++/94325
            * decl.c (begin_destructor_body): For CLASSTYPE_VBASECLASSES class
            dtors, if CLASSTYPE_PRIMARY_BINFO is non-NULL, but not
BINFO_VIRTUAL_P,
            look at CLASSTYPE_PRIMARY_BINFO of its BINFO_TYPE if it is not
            BINFO_VIRTUAL_P, and so on.

            * g++.dg/ubsan/vptr-15.C: New test.

[Bug sanitizer/94325] [8/9 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
            Summary|[8/9/10 Regression] UBSAN:  |[8/9 Regression] UBSAN:
                   |"invalid vptr" false        |"invalid vptr" false
                   |positive for virtual        |positive for virtual
                   |inheritance with            |inheritance with
                   |-fno-sanitize-recover=all   |-fno-sanitize-recover=all
             Status|NEW                         |ASSIGNED

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug sanitizer/94325] [8/9 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[cvs-commit at gcc dot gnu.org]

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

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

https://gcc.gnu.org/g:175f052446556d32e887e1658a5a92c3c2f3a6f5

commit r9-8875-g175f052446556d32e887e1658a5a92c3c2f3a6f5
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Apr 8 15:30:16 2020 +0200

    c++: Further fix for -fsanitize=vptr [PR94325]

    For -fsanitize=vptr, we insert a NULL store into the vptr instead of just
    adding a CLOBBER of this.  build_clobber_this makes the CLOBBER conditional
    on in_charge (implicit) parameter whenever CLASSTYPE_VBASECLASSES, but when
    adding this conditionalization to the -fsanitize=vptr code in PR87095,
    I wanted it to catch some more cases when the class has
CLASSTYPE_VBASECLASSES,
    but the vptr is still not shared with something else, otherwise the
    sanitization would be less effective.
    The following testcase shows that the chosen test that
CLASSTYPE_PRIMARY_BINFO
    is non-NULL and has BINFO_VIRTUAL_P set wasn't sufficient,
    the D class has still sizeof(D) == sizeof(void*) and thus contains just
    a single vptr, but while in B::~B() this results in the vptr not being
    cleared, in C::~C() this condition isn't true, as CLASSTYPE_PRIMARY_BINFO
    in that case is B and is not BINFO_VIRTUAL_P, so it clears the vptr, but
the
    D::~D() dtor after invoking C::~C() invokes A::~A() with an already cleared
    vptr, which is then reported.
    The following patch is just a shot in the dark, keep looking through
    CLASSTYPE_PRIMARY_BINFO until we find BINFO_VIRTUAL_P, but it works on the
    existing testcase as well as this new one.

    2020-04-08  Jakub Jelinek  <jakub@redhat.com>

            PR c++/94325
            * decl.c (begin_destructor_body): For CLASSTYPE_VBASECLASSES class
            dtors, if CLASSTYPE_PRIMARY_BINFO is non-NULL, but not
BINFO_VIRTUAL_P,
            look at CLASSTYPE_PRIMARY_BINFO of its BINFO_TYPE if it is not
            BINFO_VIRTUAL_P, and so on.

            * g++.dg/ubsan/vptr-15.C: New test.

    (cherry picked from commit 4cf6b06cb5b02c053738e2975e3b7a4b3c577401)

[Bug sanitizer/94325] [8/9 Regression] UBSAN: "invalid vptr" false positive for virtual inheritance with -fno-sanitize-recover=all

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #8 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 8.5 in r8-10484-g1298b488c37c44abf33cca6932e760ef69dd7815 and by the
above commit for 9.4+ too.