[Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[rombust at hotmail dot co.uk]

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

             Bug #: 52339
           Summary: using delete ptr1->ptr2 where ptr2 destructor deletes
                    a const ptr1 fails
    Classification: Unclassified
           Product: gcc
           Version: 4.6.2
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: c++
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: rombust@hotmail.co.uk


The following code crashes and triggers the valgrind error with gcc 4.4 and
4.6. With clang it works just fine. And it also works fine if you remove
"const" on line 15 

*******
struct B;
struct A {
  B* b;
};

struct B {
  A* a;
  B() : a(new A{this}) {}
  ~B() { delete a; }
};

int main() {
  B* b = new B;
  // A* a = b->a;
  const A* a = b->a;
  delete a->b;
}
******

I have no idea if is it undefined behaviour to delete a->b if b deletes a.
Expressions should be evaluated only once, but for some reason this line:
  delete a->b;
is basically acting as if was written like:
  a->b->~B();
  operator delete(a->b); // a is already deleted

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-02-22
     Ever Confirmed|0                           |1

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-22 11:09:53 UTC ---
Confirmed, [expr.delete] p4 says "The cast-expression in a delete-expression
shall be evaluated exactly once."

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[redi at gcc dot gnu.org]

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

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> 2012-02-22 12:48:04 UTC ---
    b = b.0;
    a = b->a;
    D.2361 = a->b;
    if (D.2361 != 0B) goto <D.2362>; else goto <D.2363>;
    <D.2362>:
    D.2364 = a->b;
    B::~B (D.2364);
    D.2365 = a->b;
    operator delete (D.2365);

which is indeed as if written like:

  a->b->~B();
  operator delete(a->b); // a is already deleted


if 'a' is not a pointer-to-const then instead you get

    b = b.0;
    a = b->a;
    D.2361 = a->b;
    if (D.2361 != 0B) goto <D.2362>; else goto <D.2363>;
    <D.2362>:
    B::~B (D.2361);
    operator delete (D.2361);

calling the destructor and operator delete directly on the value of a->b

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
*** Bug 109731 has been marked as a duplicate of this bug. ***

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[redi at gcc dot gnu.org]

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

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |13.1.0, 14.0, 4.1.0, 4.6.2
   Last reconfirmed|2021-08-11 00:00:00         |2023-5-4

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Marc Glisse pointed out that we're not using a SAVE_EXPR for the operand of the
delete-expression: https://gcc.gnu.org/pipermail/gcc-help/2023-May/142512.html

"""
It normally uses a SAVE_EXPR, but seems to consider token->lexer_ as "safe".

         try
           {
             Lexer::~Lexer ((struct Lexer *) token->lexer_);
           }
         finally
           {
             operator delete ((void *) token->lexer_, 8);
           }

whereas if I write delete (token->lexer_ + i); where i is 0, I get

         try
           {
             Lexer::~Lexer (SAVE_EXPR <(struct Lexer *) token->lexer_ + 
(sizetype) (i * 8)>);
           }
         finally
           {
             operator delete ((void *) (SAVE_EXPR <(struct Lexer *) 
token->lexer_ + (sizetype) (i * 8)>), 8);
           }

which works.
"""

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[jakub at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org,
                   |                            |jason at gcc dot gnu.org,
                   |                            |jsm28 at gcc dot gnu.org

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
struct L;
struct T
{
  L *const l;
  T (L *x) : l(x) {}
  ~T () = default;
  T () = delete;
  T (const T &) = delete;
  T (T &&) = delete;
  void operator= (const T &) = delete;
  void operator= (T &&) = delete;
};
struct L
{
  T *t;
  L () = default;
  ~L () { delete t; }
  L (const L&) = delete;
  L (L&&) = delete;
  void operator= (const L &) = delete;
  void operator= (L &&) = delete;
};

int
main ()
{
  L *l = new L ();
  T *t = new T (l);
  l->t = t;
  delete t->l;
}
build_delete uses
5174      if (deleting)
5175        /* We will use ADDR multiple times so we must save it.  */
5176        addr = save_expr (addr);
and for
((struct L *) this)->t
in ~L destructor that wraps it in a SAVE_EXPR, for
(struct L *) VIEW_CONVERT_EXPR<struct T *>(t)->l
in main it doesn't, as the nop is stripped and the COMPONENT_REF is
TREE_READONLY and !TREE_SIDE_EFFECTS.
tree.cc (save_expr) documents that:
   Constants, and certain read-only nodes, are returned with no
   SAVE_EXPR because that is safe.  Expressions containing placeholders
   are not touched; see tree.def for an explanation of what these
   are used for.
So, the big question is whether it is just this use of save_expr in
build_delete that needs stronger treatment (e.g. use addr = get_target_expr
(addr, complain); instead of
addr = save_expr (addr)) or if we should reconsider the save_expr
implementation and wrap with SAVE_EXPR if the expression contains any
INDIRECT_REFs/MEM_REFs (the latter perhaps with exception of ADDR_EXPR decl as
address) despite it currently just returning the passed in expression, for fear
that what the pointer points to could be deleted/reallocated in between.

Is the following testcase valid C99+?  If yes, I think we want to change
SAVE_EXPR rather than the C++ FE.  clang compiles it into something that
passes, gcc evaluates p->a multiple times (to find out the size of the VLA and
then again in the sizeof evaluation) and so it is a heap use after free
(diagnosed e.g. by -fsanitize=address).

#include <stdlib.h>

struct S { int a; };

void
bar (int *p, struct S *q)
{
  free (q);
}

size_t
foo (const struct S *p, struct S *q)
{
  int b[p->a];
  bar (b, q);
  return sizeof (b);
}

int
main ()
{
  struct S *p = malloc (sizeof (struct S));
  if (!p)
    return 0;
  p->a = 42;
  if (foo (p, p) != 42 * sizeof (int))
    abort ();
  return 0;
}

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Though, walking the whole tree to find them in tree_invariant_p_1 would result
in bad compile time complexity, because e.g. skip_simple_arithmetic can call
that on both operands of binary expression etc.
So, perhaps check it in save_expr?
-  if (tree_invariant_p_1 (inner))
+  if (tree_invariant_p_1 (inner)
+      && !walk_tree_without_duplicates (&expr, contains_indirect_refs, NULL))
     return expr;
?

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54994
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54994&action=edit
gcc14-pr52339.patch

Untested fix.

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[joseph at codesourcery dot com]

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

--- Comment #8 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I think it's valid C99, yes: the VLA size should be evaluated exactly 
once, when the declaration is passed in the order of execution.

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[jason at gcc dot gnu.org]

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

--- Comment #9 from Jason Merrill <jason at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> in main it doesn't, as the nop is stripped and the COMPONENT_REF is
> TREE_READONLY and !TREE_SIDE_EFFECTS.
> tree.cc (save_expr) documents that:
>    Constants, and certain read-only nodes, are returned with no
>    SAVE_EXPR because that is safe.

Yeah, assuming that TREE_READONLY (t) && !TREE_SIDE_EFFECTS (t) is enough to
assume that T won't change seems bogus to me.

We could remove that line from tree_invariant_p_1, or restrict it to only apply
if DECL_P (t) is also true?

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[pinskia at gcc dot gnu.org]

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

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> Created attachment 54994 [details]
> gcc14-pr52339.patch
> 
> Untested fix.

I think this might fix PR 108789 too ...

[Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails

[pinskia at gcc dot gnu.org]

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

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #10)
> (In reply to Jakub Jelinek from comment #7)
> > Created attachment 54994 [details]
> > gcc14-pr52339.patch
> > 
> > Untested fix.
> 
> I think this might fix PR 108789 too ...

And PR 53424 .