Bug 109731 - g++ evaluates delete expressions more than once
Summary: g++ evaluates delete expressions more than once
Status: RESOLVED DUPLICATE of bug 52339
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 11.3.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2023-05-04 10:51 UTC by Georg-Johann Lay
Modified: 2023-05-04 11:34 UTC (History)
1 user (show)

See Also:
Host:
Target: i386, x86_64
Build:
Known to work:
Known to fail: 13.1.0, 14.0, 4.1.0
Last reconfirmed: 2023-05-04 00:00:00


Attachments
C++ test case (243 bytes, text/plain)
2023-05-04 10:51 UTC, Georg-Johann Lay
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Georg-Johann Lay 2023-05-04 10:51:55 UTC
Created attachment 54992 [details]
C++ test case

The attached test case evaluates the last delete expression more than once, which leads to a segmentation fault when the code is executed:

> g++ main-3.cpp -O2 && ./a.out

but it also occurs with -O0. Seen on:

g++ (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0

g++ (GCC) 14.0.0 20230420 (experimental)

https://eel.is/c++draft/expr.delete#4
Comment 1 Jonathan Wakely 2023-05-04 11:06:45 UTC
Confirmed. GCC does not follow [expr.delete] p4:

 "The cast-expression in a delete-expression shall be evaluated exactly once."
Comment 2 Jonathan Wakely 2023-05-04 11:10:28 UTC
ASan shows the use-after-free:

=================================================================
==46643==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000000010 at pc 0x0000004010c4 bp 0x7ffff0f09830 sp 0x7ffff0f09828
READ of size 8 at 0x602000000010 thread T0
    #0 0x4010c3 in main /tmp/del.cc:33
    #1 0x7fc85844a50f in __libc_start_call_main (/lib64/libc.so.6+0x2750f) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #2 0x7fc85844a5c8 in __libc_start_main@GLIBC_2.2.5 (/lib64/libc.so.6+0x275c8) (BuildId: 81daba31ee66dbd63efdc4252a872949d874d136)
    #3 0x401114 in _start (/tmp/a.out+0x401114)

0x602000000010 is located 0 bytes inside of 8-byte region [0x602000000010,0x602000000018)
freed by thread T0 here:
    #0 0x7fc858addd48 in operator delete(void*, unsigned long) /home/jwakely/src/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:164
    #1 0x40109a in Lexer::~Lexer() /tmp/del.cc:20
    #2 0x40109a in main /tmp/del.cc:33

previously allocated by thread T0 here:
    #0 0x7fc858adce48 in operator new(unsigned long) /home/jwakely/src/gcc/gcc/libsanitizer/asan/asan_new_delete.cpp:95
    #1 0x40108a in main /tmp/del.cc:31

SUMMARY: AddressSanitizer: heap-use-after-free /tmp/del.cc:33 in main


valgrind agrees:

==46706== Invalid read of size 8
==46706==    at 0x40106B: main (del.cc:33)
==46706==  Address 0x4dca080 is 0 bytes inside a block of size 8 free'd
==46706==    at 0x48468DD: operator delete(void*, unsigned long) (vg_replace_malloc.c:947)
==46706==    by 0x40106A: ~Lexer (del.cc:20)
==46706==    by 0x40106A: main (del.cc:33)
==46706==  Block was alloc'd at
==46706==    at 0x4843FF5: operator new(unsigned long) (vg_replace_malloc.c:434)
==46706==    by 0x40105A: main (del.cc:31)



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.
"""
Comment 3 Jonathan Wakely 2023-05-04 11:12:15 UTC
It seems that it's considered safe because Token::lexer_ is const, so GCC thinks the value of token->lexer_ can't be changed by the ~Lexer() destructor. That's true, but 'token' can become an invalid pointer.
Comment 4 Andrew Pinski 2023-05-04 11:21:08 UTC
I think this is a dup of bug 52339 .
Comment 5 Jonathan Wakely 2023-05-04 11:34:40 UTC
Ah yes, it's exactly the same, even noting that it works without a const variable. Let's close this one.

*** This bug has been marked as a duplicate of bug 52339 ***