[Bug c++/95221] New: g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[Bug c++/95221] New: g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[mpolacek at gcc dot gnu.org]

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

            Bug ID: 95221
           Summary: g++.dg/ubsan/vptr-12.C fails with
                    -fstrong-eval-order=all
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mpolacek at gcc dot gnu.org
  Target Milestone: ---

It's this code:

struct MyClass
{
  virtual ~MyClass () {}
  virtual void Doit () {}
};

int
main ()
{
  MyClass *c = new MyClass;
  c->~MyClass ();
  c->Doit ();

  return 0;
}

$ g++ vptr-12.C -fsanitize=vptr -fno-sanitize-recover=vptr
-fstrong-eval-order=some; ./a.out
vptr-12.C:16:11: runtime error: member call on address 0x000000a25eb0 which
does not point to an object of type 'MyClass'
0x000000a25eb0: note: object has invalid vptr
 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
00 00 00  41 f1 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr

but

$ g++ vptr-12.C -fsanitize=vptr -fno-sanitize-recover=vptr
-fstrong-eval-order=all; ./a.out
Segmentation fault (core dumped)

This is important because C++17 implies -fstrong-eval-order=all.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[mpolacek at gcc dot gnu.org]

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

--- Comment #1 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
It is caused by this code:

 853       if (flag_strong_eval_order == 2
 854           && CALL_EXPR_FN (*expr_p)
 855           && cp_get_callee_fndecl_nofold (*expr_p) == NULL_TREE)
 856         {
 857           tree fnptrtype = TREE_TYPE (CALL_EXPR_FN (*expr_p));
 858           enum gimplify_status t
 859             = gimplify_to_rvalue (&CALL_EXPR_FN (*expr_p), pre_p, NULL,
 860                                   is_gimple_call_addr);
 861           if (t == GS_ERROR)
 862             ret = GS_ERROR;
 863           /* GIMPLE considers most pointer conversion useless, but for
 864              calls we actually care about the exact function pointer type.
 */
 865           else if (TREE_TYPE (CALL_EXPR_FN (*expr_p)) != fnptrtype)
 866             CALL_EXPR_FN (*expr_p)
 867               = build1 (NOP_EXPR, fnptrtype, CALL_EXPR_FN (*expr_p));
 868         }

and causes this difference:

--- 1g  2020-05-19 11:42:32.123168379 -0400
+++ 2g  2020-05-19 11:42:41.254133594 -0400
@@ -18,21 +18,21 @@
         operator delete (D.2725, 8);
       }
     c = D.2725;
+    _1 = c->_vptr.MyClass;
+    _2 = *_1;
     c.0 = c;
-    _1 = c.0->_vptr.MyClass;
-    _2 = (long unsigned int) _1;
-    .UBSAN_VPTR (c.0, _2, 2968783897514143607, &_ZTI7MyClass, 4B);
-    _3 = c->_vptr.MyClass;
-    _4 = *_3;
-    OBJ_TYPE_REF(_4;(struct MyClass)c->0) (c.0);
+    _3 = c.0->_vptr.MyClass;
+    _4 = (long unsigned int) _3;
+    .UBSAN_VPTR (c.0, _4, 2968783897514143607, &_ZTI7MyClass, 4B);
+    OBJ_TYPE_REF(_2;(struct MyClass)c->0) (c.0);
+    _5 = c->_vptr.MyClass;
+    _6 = _5 + 16;
+    _7 = *_6;
     c.1 = c;
-    _5 = c.1->_vptr.MyClass;
-    _6 = (long unsigned int) _5;
-    .UBSAN_VPTR (c.1, _6, 2968783897514143607, &_ZTI7MyClass, 4B);
-    _7 = c->_vptr.MyClass;
-    _8 = _7 + 16;
-    _9 = *_8;
-    OBJ_TYPE_REF(_9;(struct MyClass)c->2) (c.1);
+    _8 = c.1->_vptr.MyClass;
+    _9 = (long unsigned int) _8;
+    .UBSAN_VPTR (c.1, _9, 2968783897514143607, &_ZTI7MyClass, 4B);
+    OBJ_TYPE_REF(_7;(struct MyClass)c->2) (c.1);
     D.2751 = 0;
     return D.2751;
   }

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[mpolacek at gcc dot gnu.org]

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

--- Comment #2 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
I think the thing is that we have a CALL_EXPR, something like

  OBJ_TYPE_REF (...) (.UBSAN_VPTR ())

and now we first evaluate the OBJ_TYPE_REF.  In this case this is what seems to
happen here:

1) we evaluate OBJ_TYPE_REF before the arguments:

     OBJ_TYPE_REF(_2;(struct MyClass)c->0) (c.0);

   I think this calls the destructor of 'c', and that clears up the vptr:

     this->_vptr.MyClass = 0B;

2) then we fetch the vptr:

      _5 = c->_vptr.MyClass; // _5 == null

3) we add the offset to the vtable for Doit:

      _6 = _5 + 16;  // _6 == 16

4) we deref the pointer and crash

      _7 = *_6;


With -fstrong-eval-order=some we instead do this

    _3 = c->_vptr.MyClass;
    _4 = *_3;
    OBJ_TYPE_REF(_4;(struct MyClass)c->0) (c.0);

so there's no crash, and the subsequent call to the ubsan routine:

    _5 = c.1->_vptr.MyClass;
    _6 = (long unsigned int) _5; 
    .UBSAN_VPTR (c.1, _6, 2968783897514143607, &_ZTI7MyClass, 4B);

reports the error, because it sees that _6 is null.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[mpolacek at gcc dot gnu.org]

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

--- Comment #3 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
And for completeness, the asm for the -fstrong-eval-order=all case:

        movq    %rbx, %rdi
        call    *%r12
        movq    -24(%rbp), %rax
        movq    (%rax), %rax
        addq    $16, %rax
        movq    (%rax), %r12     # crash here

Any ideas what as to we want to do here?

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Is this code even defined?
We call a method after calling the deconstructor on the object?
If we do:
 c->~MyClass ();
 new(c) MyClass();
 c->Doit ();

Then it is defined.

Or am I wrong about that?

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[mpolacek at gcc dot gnu.org]

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

--- Comment #5 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
You're not wrong, but here we're dealing with the undefined behavior sanitizer
whose point is to detect broken code like the above.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-05-20
             Status|UNCONFIRMED                 |NEW

--- Comment #6 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Marek Polacek from comment #2)
> I think the thing is that we have a CALL_EXPR, something like
> 
>   OBJ_TYPE_REF (...) (.UBSAN_VPTR ())
> 
> and now we first evaluate the OBJ_TYPE_REF.

Yep, that's the problem.

We're currently sanitizing the vtable when determining the 'this' argument to
the function, separately from looking at the vtable in order to determine which
function to call.  In C++14 we evaluate 'this' first, in C++17 we evaluate the
function first.  build_over_call tries to share any side-effects between the
'this' and the OBJ_TYPE_REF with a save_expr, but then
cp_ubsan_maybe_instrument_member_call adds the sanitization only to the 'this'
argument, breaking that sharing and introducing a fragile order dependency.

A simple fix might be to not preevaluate when CALL_EXPR_FN is an OBJ_TYPE_REF,
as the sharing above should mean that side-effects happen when the object
argument is evaluated, which should be what we want.  It occurs to me that the
current preevaluation is wrong for a virtual assignment when
CALL_EXPR_REVERSE_ARGS is set.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

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

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[jason at gcc dot gnu.org]

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

Jason Merrill <jason at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jason Merrill <jason at gcc dot gnu.org> ---
Fixed.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:8e915901deb3518d4bef73ea52eab2ece7a2bbf6

commit r11-728-g8e915901deb3518d4bef73ea52eab2ece7a2bbf6
Author: Jason Merrill <jason@redhat.com>
Date:   Fri May 29 11:59:33 2020 -0400

    c++: vptr ubsan and derived class [PR95311].

    We weren't able to find OBJ_TYPE_REF_OBJECT walking through
    OBJ_TYPE_REF_EXPR because we had folded away the ADDR_EXPR.

    gcc/cp/ChangeLog:

            PR c++/95311
            PR c++/95221
            * class.c (build_vfn_ref): Don't fold the INDIRECT_REF.

    gcc/testsuite/ChangeLog:

            PR c++/95311
            * g++.dg/ubsan/vptr-16.C: New test.

[Bug c++/95221] g++.dg/ubsan/vptr-12.C fails with -fstrong-eval-order=all

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jason Merrill <jason@gcc.gnu.org>:

https://gcc.gnu.org/g:172f2c42a10fd470c93f1e84575de9766c157591

commit r11-775-g172f2c42a10fd470c93f1e84575de9766c157591
Author: Jason Merrill <jason@redhat.com>
Date:   Mon Jun 1 16:20:38 2020 -0400

    c++: vptr ubsan and object of known type [PR95466]

    Another case where we can't find the OBJ_TYPE_REF_OBJECT in the
    OBJ_TYPE_REF_EXPR.  So let's just evaluate the sanitize call first.

    gcc/cp/ChangeLog:

            PR c++/95466
            PR c++/95311
            PR c++/95221
            * class.c (build_vfn_ref): Revert 95311 change.
            * cp-ubsan.c (cp_ubsan_maybe_instrument_member_call): Build a
            COMPOUND_EXPR.

    gcc/testsuite/ChangeLog:

            PR c++/95466
            * g++.dg/ubsan/vptr-17.C: New test.