Use of the singleton pattern leads to "use after free"

[karulont]

The cpp_redis::network::io_service is implemented as a singleton.
The problem with that is, that in some cases the destructor for io_service is called before the destructor of redis_client and this leads to "use after free".

Example log from valgrind:

==5190== Invalid read of size 8
==5190==    at 0x9FE3258: _M_find_before_node (hashtable.h:1413)
==5190==    by 0x9FE3258: _M_erase (hashtable.h:1779)
==5190==    by 0x9FE3258: erase (hashtable.h:751)
==5190==    by 0x9FE3258: erase (unordered_map.h:776)
==5190==    by 0x9FE3258: cpp_redis::network::io_service::untrack(int) (io_service.cpp:176)
==5190==    by 0x9FE66D5: cpp_redis::network::tcp_client::disconnect() (tcp_client.cpp:76)
==5190==    by 0x9FCE8E7: cpp_redis::redis_client::~redis_client() (redis_client.cpp:8)
==5190==    by 0x9DB1A71: ~ModuleData (ModuleData.h:19)
<snip....>
==5190==    by 0x67CD98F: __run_exit_handlers (in /usr/lib/libc-2.24.so)
==5190==    by 0x67CD9E9: exit (in /usr/lib/libc-2.24.so)
==5190==    by 0x67B8297: (below main) (in /usr/lib/libc-2.24.so)
==5190==  Address 0xb444c20 is 0 bytes inside a block of size 16 free'd
==5190==    at 0x4C2C20A: operator delete(void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==5190==    by 0x9FE2757: deallocate (new_allocator.h:110)
==5190==    by 0x9FE2757: deallocate (alloc_traits.h:442)
==5190==    by 0x9FE2757: _M_deallocate_buckets (hashtable_policy.h:2008)
==5190==    by 0x9FE2757: _M_deallocate_buckets (hashtable.h:355)
==5190==    by 0x9FE2757: _M_deallocate_buckets (hashtable.h:360)
==5190==    by 0x9FE2757: ~_Hashtable (hashtable.h:1228)
==5190==    by 0x9FE2757: ~unordered_map (unordered_map.h:98)
==5190==    by 0x9FE2757: cpp_redis::network::io_service::~io_service() (io_service.cpp:29)
==5190==    by 0x67CD98F: __run_exit_handlers (in /usr/lib/libc-2.24.so)
==5190==    by 0x67CD9E9: exit (in /usr/lib/libc-2.24.so)
==5190==    by 0x67B8297: (below main) (in /usr/lib/libc-2.24.so)

[Cylix]

I pushed a fix

The io_service is still a singleton.
However, instead of having the get_instance() returning a reference to the instance, it returns a shared_pointer that is then stored in the tcp_client instances.

That way, we can ensure that the io_service instance is destroyed only when every resource using it have released the shared_ptr they hold.

Can you confirm that it solves the issue?

Thanks for taking the time to report that btw!

[karulont]

This fixes the problem.
I confirmed, that the static shared_ptr is destroyed before the destructor of tcp_client.
But thanks to it being a shared_ptr, the underlying io_service is still valid, until all tcp_clients are destructed.