public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
@ 2012-02-22 10:37 rombust at hotmail dot co.uk
  2012-02-22 11:22 ` [Bug c++/52339] " redi at gcc dot gnu.org
                   ` (10 more replies)
  0 siblings, 11 replies; 12+ messages in thread
From: rombust at hotmail dot co.uk @ 2012-02-22 10:37 UTC (permalink / raw)
  To: gcc-bugs

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
@ 2012-02-22 11:22 ` redi at gcc dot gnu.org
  2012-02-22 13:07 ` redi at gcc dot gnu.org
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2012-02-22 11:22 UTC (permalink / raw)
  To: gcc-bugs

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."


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
  2012-02-22 11:22 ` [Bug c++/52339] " redi at gcc dot gnu.org
@ 2012-02-22 13:07 ` redi at gcc dot gnu.org
  2023-05-04 11:34 ` redi at gcc dot gnu.org
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2012-02-22 13:07 UTC (permalink / raw)
  To: gcc-bugs

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
  2012-02-22 11:22 ` [Bug c++/52339] " redi at gcc dot gnu.org
  2012-02-22 13:07 ` redi at gcc dot gnu.org
@ 2023-05-04 11:34 ` redi at gcc dot gnu.org
  2023-05-04 11:35 ` redi at gcc dot gnu.org
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2023-05-04 11:34 UTC (permalink / raw)
  To: gcc-bugs

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. ***

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (2 preceding siblings ...)
  2023-05-04 11:34 ` redi at gcc dot gnu.org
@ 2023-05-04 11:35 ` redi at gcc dot gnu.org
  2023-05-04 12:23 ` jakub at gcc dot gnu.org
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: redi at gcc dot gnu.org @ 2023-05-04 11:35 UTC (permalink / raw)
  To: gcc-bugs

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.
"""

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (3 preceding siblings ...)
  2023-05-04 11:35 ` redi at gcc dot gnu.org
@ 2023-05-04 12:23 ` jakub at gcc dot gnu.org
  2023-05-04 12:45 ` jakub at gcc dot gnu.org
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-05-04 12:23 UTC (permalink / raw)
  To: gcc-bugs

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;
}

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (4 preceding siblings ...)
  2023-05-04 12:23 ` jakub at gcc dot gnu.org
@ 2023-05-04 12:45 ` jakub at gcc dot gnu.org
  2023-05-04 13:45 ` jakub at gcc dot gnu.org
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-05-04 12:45 UTC (permalink / raw)
  To: gcc-bugs

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;
?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (5 preceding siblings ...)
  2023-05-04 12:45 ` jakub at gcc dot gnu.org
@ 2023-05-04 13:45 ` jakub at gcc dot gnu.org
  2023-05-04 16:58 ` joseph at codesourcery dot com
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: jakub at gcc dot gnu.org @ 2023-05-04 13:45 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (6 preceding siblings ...)
  2023-05-04 13:45 ` jakub at gcc dot gnu.org
@ 2023-05-04 16:58 ` joseph at codesourcery dot com
  2023-05-04 22:12 ` jason at gcc dot gnu.org
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 12+ messages in thread
From: joseph at codesourcery dot com @ 2023-05-04 16:58 UTC (permalink / raw)
  To: gcc-bugs

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (7 preceding siblings ...)
  2023-05-04 16:58 ` joseph at codesourcery dot com
@ 2023-05-04 22:12 ` jason at gcc dot gnu.org
  2023-11-09  6:35 ` pinskia at gcc dot gnu.org
  2023-12-21  3:18 ` pinskia at gcc dot gnu.org
  10 siblings, 0 replies; 12+ messages in thread
From: jason at gcc dot gnu.org @ 2023-05-04 22:12 UTC (permalink / raw)
  To: gcc-bugs

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?

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (8 preceding siblings ...)
  2023-05-04 22:12 ` jason at gcc dot gnu.org
@ 2023-11-09  6:35 ` pinskia at gcc dot gnu.org
  2023-12-21  3:18 ` pinskia at gcc dot gnu.org
  10 siblings, 0 replies; 12+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-11-09  6:35 UTC (permalink / raw)
  To: gcc-bugs

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 ...

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Bug c++/52339] using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails
  2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
                   ` (9 preceding siblings ...)
  2023-11-09  6:35 ` pinskia at gcc dot gnu.org
@ 2023-12-21  3:18 ` pinskia at gcc dot gnu.org
  10 siblings, 0 replies; 12+ messages in thread
From: pinskia at gcc dot gnu.org @ 2023-12-21  3:18 UTC (permalink / raw)
  To: gcc-bugs

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 .

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2023-12-21  3:18 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-02-22 10:37 [Bug c++/52339] New: using delete ptr1->ptr2 where ptr2 destructor deletes a const ptr1 fails rombust at hotmail dot co.uk
2012-02-22 11:22 ` [Bug c++/52339] " redi at gcc dot gnu.org
2012-02-22 13:07 ` redi at gcc dot gnu.org
2023-05-04 11:34 ` redi at gcc dot gnu.org
2023-05-04 11:35 ` redi at gcc dot gnu.org
2023-05-04 12:23 ` jakub at gcc dot gnu.org
2023-05-04 12:45 ` jakub at gcc dot gnu.org
2023-05-04 13:45 ` jakub at gcc dot gnu.org
2023-05-04 16:58 ` joseph at codesourcery dot com
2023-05-04 22:12 ` jason at gcc dot gnu.org
2023-11-09  6:35 ` pinskia at gcc dot gnu.org
2023-12-21  3:18 ` pinskia at gcc dot gnu.org

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).