Bug 104213 - [12 Regression] bogus use-after-free in virtual dtor with -ffat-lto-objects on ARM
Summary: [12 Regression] bogus use-after-free in virtual dtor with -ffat-lto-objects o...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 12.0
: P1 normal
Target Milestone: 12.0
Assignee: Marek Polacek
URL:
Keywords: diagnostic
: 104190 (view as bug list)
Depends on:
Blocks: Wuse-after-free
  Show dependency treegraph
 
Reported: 2022-01-24 19:13 UTC by David Tardon
Modified: 2022-02-11 20:37 UTC (History)
6 users (show)

See Also:
Host:
Target: arm*-linux-gnu
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-01-24 00:00:00


Attachments
reproducer.cpp (108 bytes, text/x-csrc)
2022-01-24 19:13 UTC, David Tardon
Details
reproducer.s (5.86 KB, text/plain)
2022-01-24 19:14 UTC, David Tardon
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Tardon 2022-01-24 19:13:36 UTC
Created attachment 52281 [details]
reproducer.cpp

g++ 12 emits a bogus use-after-free warning on any virtual dtor on ARM if -ffat-lto-objects is used. See the attached minimal reproducer.
Comment 1 David Tardon 2022-01-24 19:14:07 UTC
Created attachment 52282 [details]
reproducer.s
Comment 2 Marek Polacek 2022-01-24 20:35:08 UTC
Confirmed with an arm-linux-gnueabihf cross:

$ ./cc1plus -quiet -Iinclude reproducer.cpp -flto=auto -ffat-lto-objects -Wuse-after-free
reproducer.cpp: In member function ‘C::~C()’:
reproducer.cpp:8:10: warning: pointer used after ‘operator delete(void*, unsigned int)’ [-Wuse-after-free]
    8 | C::~C() {}
      |          ^
reproducer.cpp:8:10: note: call to ‘operator delete(void*, unsigned int)’ here
Comment 3 Marek Polacek 2022-01-24 20:39:33 UTC
-flto=auto -ffat-lto-objects is not needed.
Comment 4 Martin Sebor 2022-01-24 20:48:55 UTC
The warning points out the return statement in the dtor below, so it's doing what it's been taught to do.  I'm not sure why a dtor would have a return type other than void or why it would need to return the this pointer (it's invalid after the object has been deleted).  If that's necessary then the code that emits the return statement could suppress the warning via suppress_warning(ret_stmt, OPT_Wuse_after_free).

;; Function C::~C (_ZN1CD0Ev, funcdef_no=3, decl_uid=4619, cgraph_uid=4, symbol_order=3)

void * C::~C (struct C * const this)
{
  void * D.4660;
  void * _5;

  <bb 2> :
  C::~C (this_2(D));
  operator delete (this_2(D), 4);   <<< delete (this)
  _5 = this_2(D);

  <bb 3> :
<L0>:
  return _5;                        <<< return this

}


pr104213.C: In member function 'C::~C()':
pr104213.C:8:10: warning: pointer used after 'operator delete(void*, unsigned int)' [-Wuse-after-free]
    8 | C::~C() {}
      |          ^
pr104213.C:8:10: note: call to 'operator delete(void*, unsigned int)' here
tmp$
Comment 5 Jakub Jelinek 2022-01-24 20:51:52 UTC
Return type from ctors/dtors is an ARM EABI special thing, the warning should ignore such returns from dtors if targetm.cxx.cdtor_returns_this ().
Comment 6 Andrew Pinski 2022-01-24 20:54:49 UTC
> I'm not sure why a dtor would have a return type other than void or why it would need to return the this pointer

Because that is what is required by the ARM C++ EABI:

From the PDF on:
https://developer.arm.com/documentation/ihi0041/latest

GC++ABI §3.1.512 Constructor return values
This ABI requires C1 and C2 constructors to return this (instead of being void functions) so that a C3
constructor can tail call the C1 constructor and the C1 constructor can tail call C2.
Similarly, we require D2 and D1 to return this so that D0 need not save and restore this and D1 can tail
call D2 (if there are no virtual bases). D0 is still a void function.
We do not require thunks to virtual destructors to return this. Such a thunk would have to adjust the
destructor’s result, preventing it from tail calling the destructor, and nullifying any possible saving.
Consequently, only non-virtual calls of D1 and D2 destructors can be relied on to return this.
Comment 7 Jakub Jelinek 2022-01-24 21:02:08 UTC
Now that you cite that, seems this error is because we go beyond what the EABI requires and make even the D0 dtor return this that nothing can really use.
So one way to fix that would be to also make sure that D0 dtors return void always.  We'd need to check what other targets use true for this target hook though and whether they have similar wordings...
Comment 8 Marek Polacek 2022-01-24 21:19:06 UTC
I've tried

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 22d3dd1e2ad..c2a0f0c24e2 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -17319,6 +17319,7 @@ finish_constructor_body (void)
            DECL_RESULT (current_function_decl), val);
       /* Return the address of the object.  */
       exprstmt = build_stmt (input_location, RETURN_EXPR, val);
+      suppress_warning (exprstmt, OPT_Wuse_after_free);
       add_stmt (exprstmt);
     }
 }
@@ -17412,6 +17413,7 @@ finish_destructor_body (void)
            DECL_RESULT (current_function_decl), val);
       /* Return the address of the object.  */
       exprstmt = build_stmt (input_location, RETURN_EXPR, val);
+      suppress_warning (exprstmt, OPT_Wuse_after_free);
       add_stmt (exprstmt);
     }
 }
diff --git a/gcc/cp/optimize.cc b/gcc/cp/optimize.cc
index 4ad3f1dc9aa..51393710526 100644
--- a/gcc/cp/optimize.cc
+++ b/gcc/cp/optimize.cc
@@ -168,6 +168,7 @@ build_delete_destructor_body (tree delete_dtor, tree complete_dtor)
       tree val = DECL_ARGUMENTS (delete_dtor);
       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
                     DECL_RESULT (delete_dtor), val);
+      suppress_warning (val, OPT_Wuse_after_free);
       add_stmt (build_stmt (0, RETURN_EXPR, val));
     }
 }

the last hunk should have done the trick but the suppress_warning bits don't survive gimplification, it seems.
Comment 9 Andrew Pinski 2022-01-24 21:22:00 UTC
Try:

       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
                     DECL_RESULT (delete_dtor), val);
       tree stmt = build_stmt (0, RETURN_EXPR, val);
       suppress_warning (stmt, OPT_Wuse_after_free);
       add_stmt (stmt);


That is do it for the RETURN_EXPR instead of the MODIFY_EXPR. It might be saved. Otherwise the gimplification of RETURN_EXPR to GIMPLE_RETURN needs to save it around.
Comment 10 Marek Polacek 2022-01-24 21:31:13 UTC
I did but it didn't work either.  So I think we need to properly propagate the suppression bits.  Something like this:

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index bf2f60cce9a..0d266241b8c 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -6126,6 +6126,7 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   else
     {
       assign = gimple_build_assign (*to_p, *from_p);
+      copy_warning (assign, *expr_p);
       gimple_set_location (assign, EXPR_LOCATION (*expr_p));
       if (COMPARISON_CLASS_P (*from_p))
    copy_warning (assign, *from_p);

which, along with the patch above, works.
Comment 11 Marek Polacek 2022-01-24 21:37:37 UTC
Obviously the
 6131       if (COMPARISON_CLASS_P (*from_p))
 6132         copy_warning (assign, *from_p);
doesn't work because we are not dealing with a comparison here.
Comment 12 Marek Polacek 2022-01-25 20:12:16 UTC
Removing the COMPARISON_CLASS_P check regresses uninit-pr74762.C.  So how about

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index 22d3dd1e2ad..6534a7fd320 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -17315,6 +17315,7 @@ finish_constructor_body (void)
       add_stmt (build_stmt (input_location, LABEL_EXPR, cdtor_label));
 
       val = DECL_ARGUMENTS (current_function_decl);
+      suppress_warning (val, OPT_Wuse_after_free);
       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
            DECL_RESULT (current_function_decl), val);
       /* Return the address of the object.  */
@@ -17408,6 +17409,7 @@ finish_destructor_body (void)
       tree val;
 
       val = DECL_ARGUMENTS (current_function_decl);
+      suppress_warning (val, OPT_Wuse_after_free);
       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
            DECL_RESULT (current_function_decl), val);
       /* Return the address of the object.  */
diff --git a/gcc/cp/optimize.cc b/gcc/cp/optimize.cc
index 4ad3f1dc9aa..13ab8b7361e 100644
--- a/gcc/cp/optimize.cc
+++ b/gcc/cp/optimize.cc
@@ -166,6 +166,7 @@ build_delete_destructor_body (tree delete_dtor, tree complete_dtor)
   if (targetm.cxx.cdtor_returns_this ())
     {
       tree val = DECL_ARGUMENTS (delete_dtor);
+      suppress_warning (val, OPT_Wuse_after_free);
       val = build2 (MODIFY_EXPR, TREE_TYPE (val),
                     DECL_RESULT (delete_dtor), val);
       add_stmt (build_stmt (0, RETURN_EXPR, val));
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc
index 8bc33eeb6fa..f39092ec416 100644
--- a/gcc/gimple-ssa-warn-access.cc
+++ b/gcc/gimple-ssa-warn-access.cc
@@ -3880,9 +3880,19 @@ pass_waccess::warn_invalid_pointer (tree ref, gimple *use_stmt,
                    bool maybe, bool equality /* = false */)
 {
   /* Avoid printing the unhelpful "<unknown>" in the diagnostics.  */
-  if (ref && TREE_CODE (ref) == SSA_NAME
-      && (!SSA_NAME_VAR (ref) || DECL_ARTIFICIAL (SSA_NAME_VAR (ref))))
-    ref = NULL_TREE;
+  if (ref && TREE_CODE (ref) == SSA_NAME)
+    {
+      tree var = SSA_NAME_VAR (ref);
+      if (!var)
+   ref = NULL_TREE;
+      else if (DECL_ARTIFICIAL (var))
+   {
+     /* Don't warn for cases like when a cdtor returns 'this' on ARM.  */
+     if (warning_suppressed_p (var, OPT_Wuse_after_free))
+       return;
+     ref = NULL_TREE;
+   }
+    }
 
   location_t use_loc = gimple_location (use_stmt);
   if (use_loc == UNKNOWN_LOCATION)


I guess I can test & post it.
Comment 13 GCC Commits 2022-01-26 17:22:27 UTC
The trunk branch has been updated by Marek Polacek <mpolacek@gcc.gnu.org>:

https://gcc.gnu.org/g:7bd1e1296cc36b558a27bbe09352c5c2aca4c5d5

commit r12-6880-g7bd1e1296cc36b558a27bbe09352c5c2aca4c5d5
Author: Marek Polacek <polacek@redhat.com>
Date:   Tue Jan 25 15:12:51 2022 -0500

    warn-access: Prevent -Wuse-after-free on ARM [PR104213]
    
    Here, -Wuse-after-free warns about using 'this' which, on ARM, cdtors
    return, as mandated by the EABI.  To be entirely correct, it only
    requires it for C1 and C2 ctors and D2 and D1 dtors, but I don't feel
    like changing that now and possibly running into issues later on.
    
    This patch uses suppress_warning on 'this' for certain cdtor_returns_this
    cases in the C++ FE, and then warn_invalid_pointer makes use of this
    information and doesn't warn.
    
    In my first attempt I tried suppress_warning the MODIFY_EXPR or RETURN_EXPR
    we build in build_delete_destructor_body, but the complication is that
    the suppress_warning bits don't always survive gimplification; see e.g.
    gimplify_modify_expr where we do
    
     6130       if (COMPARISON_CLASS_P (*from_p))
     6131         copy_warning (assign, *from_p);
    
    but here we're not dealing with a comparison.  Removing that check
    regresses uninit-pr74762.C.  Adding copy_warning (assign, *expr_p)
    regresses c-c++-common/uninit-17.c.
    
            PR target/104213
    
    gcc/cp/ChangeLog:
    
            * decl.cc (finish_constructor_body): Suppress -Wuse-after-free.
            (finish_destructor_body): Likewise.
            * optimize.cc (build_delete_destructor_body): Likewise.
    
    gcc/ChangeLog:
    
            * gimple-ssa-warn-access.cc (pass_waccess::warn_invalid_pointer): Don't
            warn when the SSA_NAME_VAR of REF has supressed -Wuse-after-free.
    
    gcc/testsuite/ChangeLog:
    
            * g++.dg/warn/Wuse-after-free2.C: New test.
            * g++.dg/warn/Wuse-after-free3.C: New test.
Comment 14 Marek Polacek 2022-01-26 17:23:20 UTC
Fixed.
Comment 15 Andrew Pinski 2022-01-29 05:40:04 UTC
*** Bug 104190 has been marked as a duplicate of this bug. ***