Signals created via move constructor/assignment use the moved from object
dummyunit opened this issue · 1 comments
dummyunit commented
Move support for signal_type added in #19 has a major flaw - the signal created by a move constructor/assignment keeps a pointer (_shared_disconnector) to _disconnector stored inside the moved from object.
If the original object is then deleted, any operation that uses the disconnector (another move, connection::disconnect) will try to access a freed memory and will most likely crash.
This can be easily detected by AddressSanitizer using a code like this:
#include "nod.hpp"
#include <memory>
int main()
{
auto sig1 = std::make_unique<nod::signal<void ()>>();
auto conn = sig1->connect([]{}); // initializes `_shared_disconnector`
auto sig2 = std::move(*sig1);
sig1.reset();
// Any of the following lines tries to access memory of the deleted *sig1 object:
conn.disconnect();
auto sig3 = std::move(sig2);
}Compiling (g++ -g -O1 -fsanitize=address nod_move_fail.cpp) and running this code with ASan results in this report:
==12348==ERROR: AddressSanitizer: heap-use-after-free on address 0x60b000000138 at pc 0x557ffecfb967 bp 0x7fff01c98960 sp 0x7fff01c98950
READ of size 8 at 0x60b000000138 thread T0
#0 0x557ffecfb966 in nod::connection::disconnect() /tmp/nod.hpp:657
#1 0x557ffecfa32c in main /tmp/nod_move_fail.cpp:10
#2 0x7f0b36963e9d in __libc_start_main (/lib64/libc.so.6+0x23e9d)
#3 0x557ffecf9269 in _start (/tmp/a.out+0x2269)
0x60b000000138 is located 72 bytes inside of 104-byte region [0x60b0000000f0,0x60b000000158)
freed by thread T0 here:
#0 0x7f0b36f1ba87 in operator delete(void*, unsigned long) (/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/libasan.so.6+0xb3a87)
#1 0x557ffecfa320 in std::default_delete<nod::signal_type<nod::multithread_policy, void ()> >::operator()(nod::signal_type<nod::multithread_policy, void ()>*) const /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/include/g++-v10/bits/unique_ptr.h:85
#2 0x557ffecfa320 in std::__uniq_ptr_impl<nod::signal_type<nod::multithread_policy, void ()>, std::default_delete<nod::signal_type<nod::multithread_policy, void ()> > >::reset(nod::signal_type<nod::multithread_policy, void ()>*) /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/include/g++-v10/bits/unique_ptr.h:182
#3 0x557ffecfa320 in std::unique_ptr<nod::signal_type<nod::multithread_policy, void ()>, std::default_delete<nod::signal_type<nod::multithread_policy, void ()> > >::reset(nod::signal_type<nod::multithread_policy, void ()>*) /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/include/g++-v10/bits/unique_ptr.h:456
#4 0x557ffecfa320 in main /tmp/nod_move_fail.cpp:8
previously allocated by thread T0 here:
#0 0x7f0b36f1aa97 in operator new(unsigned long) (/usr/lib/gcc/x86_64-pc-linux-gnu/11.1.0/libasan.so.6+0xb2a97)
#1 0x557ffecf94b4 in std::_MakeUniq<nod::signal_type<nod::multithread_policy, void ()> >::__single_object std::make_unique<nod::signal_type<nod::multithread_policy, void ()>>() /usr/lib/gcc/x86_64-pc-linux-gnu/10.2.0/include/g++-v10/bits/unique_ptr.h:962
#2 0x557ffecf94b4 in main /tmp/nod_move_fail.cpp:5
SUMMARY: AddressSanitizer: heap-use-after-free /tmp/nod.hpp:657 in nod::connection::disconnect()
Shadow bytes around the buggy address:
0x0c167fff7fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c167fff7fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c167fff7ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0c167fff8000: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x0c167fff8010: fd fd fd fd fd fa fa fa fa fa fa fa fa fa fd fd
=>0x0c167fff8020: fd fd fd fd fd fd fd[fd]fd fd fd fa fa fa fa fa
0x0c167fff8030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c167fff8040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c167fff8050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c167fff8060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c167fff8070: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
I don't know how this can be fixed without creating the disconnector on the heap.
fr00b0 commented
Thank you for this issue report.
I am aware of a few thread safety issues at this time, but I currently have not figured out any elegant solutions. I'll keep the issue open as a reminder to what I do need to fix eventually.