Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signals created via move constructor/assignment use the moved from object #24

Open
dummyunit opened this issue May 26, 2021 · 1 comment · May be fixed by #32
Open

Signals created via move constructor/assignment use the moved from object #24

dummyunit opened this issue May 26, 2021 · 1 comment · May be fixed by #32

Comments

@dummyunit
Copy link

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
Copy link
Owner

fr00b0 commented Jul 12, 2021

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.

W4RH4WK added a commit to W4RH4WK/nod that referenced this issue Sep 16, 2022
The disconnector object is no longer stored directly in the signal object.
It is now stored on the heap and managed by the shared pointer that was already present to enable weak pointers, but did not actually manage the disconnector object.

fixes fr00b0#24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants