CThread: fix memory leak

[repojohnray]

Description

m_thread was allocated with a "new", this commit ensures that the thread is properly freed.

[a1rwulf]

nope, lthread is assigned from m_thread and m_thread is deleted in CThreads desctructor.

[garbear]

Right, this introduces the possibility of a double-free.

[repojohnray]

The same block sets m_thread to nullptr which prevents the CThread destructor to clean it. We should keep in mind that some classes, which are child of CThread, are overriding the destructor and rely on StopThread to clean the thread. This issue triggers the gcc sanitizer.

[a1rwulf]

True, I'll let someone with more knowledge about CThread comment on this.
Thinking of RAII, I'd rather let the destructor do it's job and remove the m_thread = nullptr; but that will break IsRunning so probably your change is correct.
@lrusak or @jimfcarroll ?

[garbear]

The design allows CThread to be constructed with an "autodelete" feature, which allows for "new and forget me" type construction. In this mode, new is called but the return value never recorded. Then, when the thread stops, delete is called inside the thread context.

And yes, this has caused sanitizers to go nuts a lot.

Adding a delete here changes the design, which I would not recommend. What I would recommend is to remove the "autodelete" feature altogether. An indication of a good design is where new and delete are called in the same thread.

If anything, it'd be nice to shut the sanitizers up.

[repojohnray]

This issue is specific to StopThread(), I don't think that autodelete is affected by this change. StopThread() is explicitly called and we must ensure that m_thread is deleted in the process.

[garbear]

This issue is specific to StopThread(), I don't think that autodelete is affected by this change.

The issue is that StopThread() behavior now depends on autodelete (because the destiny of the pointer depends on autodelete). If autodelete was true on creation, then stopping the thread manually will cause a double free.

[garbear]

My advice: remove autodelete. Nothing short of this will clean the santizer errors. Use refcounting or make a manager that tracks autodeleting threads.

[repojohnray]

This change fixes the sanitizer errors triggered by StopThread(); I let you try on your configuration.

[garbear]

The change quiets the sanitizers, but they wouldn't see the double free for the same reason they didn't see the single free.

Have you runtime tested this change? Can you launch and stop an autodelete thread, and verify that a double-free doesn't occur, as it appears to me it would?

[repojohnray]

OK, I will check.

[repojohnray]

Here is a simplified version of CThread for test purposes: https://gist.github.com/repojohnray/8642d27635534e23ab85630dca56f7e3

./cthread_test autodelete
Thread t1 start, auto delete: false
test...
Thread t1 140556188849728 terminating
Thread t2 start, auto delete: true
Thread t2 140556180457024 terminating (autodelete)
free(): invalid pointer
Aborted

"free(): invalid pointer" is happening at the autodelete "delete" execution (line 344). In this specific case StopThread() is not even called, I don't think autodelete is working.

Here is the output with the sanitizer:

c++ -g -O2 -Wall -fsanitize=bounds -fsanitize=address -fsanitize=undefined -fsanitize=unreachable -fsanitize=bool -fsanitize=enum -fsanitize=leak -fsanitize-recover=address "cthread_test.cpp" -o "cthread_test" -lstdc++
./cthread_test autodelete
Thread Thread t1 start, auto delete: falset2 start, auto delete: true
Thread 
test...
Thread t1 139819318507072 terminating
t2 139819310114368 terminating (autodelete)
/usr/include/c++/11.3.0/chrono:240:38: runtime error: signed integer overflow: 9223372036854775807 * 1000000 cannot be represented in type 'long int'
=================================================================
/usr/include/c++/11.3.0/chrono:240:38: runtime error: signed integer overflow: 9223372036854775807 * 1000000 cannot be represented in type 'long int'
AddressSanitizer:DEADLYSIGNAL
==22473==ERROR: AddressSanitizer: attempting free on address which was not malloc()-ed: 0x7ffcb7c55810 in thread T2
    #0 0x7f2a3d035db7 in operator delete(void*, unsigned long) (/usr/lib64/libasan.so.6+0xb3db7)
    #1 0x409d04 in operator() cthread_test.cpp:344
    #2 0x40c500 in __invoke_impl<void, CThread::Create(bool)::<lambda(CThread*, std::promise<bool>)>, CThread*, std::promise<bool> > /usr/include/c++/11.3.0/bits/invoke.h:61
    #3 0x40c500 in __invoke<CThread::Create(bool)::<lambda(CThread*, std::promise<bool>)>, CThread*, std::promise<bool> > /usr/include/c++/11.3.0/bits/invoke.h:96
    #4 0x40c500 in _M_invoke<0, 1, 2> /usr/include/c++/11.3.0/bits/std_thread.h:253
    #5 0x40c500 in operator() /usr/include/c++/11.3.0/bits/std_thread.h:260
    #6 0x40c500 in _M_run /usr/include/c++/11.3.0/bits/std_thread.h:211
    #7 0x7f2a3ce452e3  (/usr/lib64/libstdc++.so.6+0xdb2e3)
    #8 0x7f2a3c17f535  (/lib64/libc.so.6+0x85535)
    #9 0x7f2a3c2010ef in __clone (/lib64/libc.so.6+0x1070ef)

Address 0x7ffcb7c55810 is located in stack of thread T0 at offset 144 in frame
    #0 0x4055bf in main cthread_test.cpp:506

  This frame has 2 object(s):
    [32, 104) 't1' (line 507)
    [144, 216) 't2' (line 512) <== Memory access at offset 144 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism, swapcontext or vfork
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: bad-free (/usr/lib64/libasan.so.6+0xb3db7) in operator delete(void*, unsigned long)
Thread T2 created by T0 here:
    #0 0x7f2a3cfdb716 in pthread_create (/usr/lib64/libasan.so.6+0x59716)
    #1 0x7f2a3ce453b8 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/usr/lib64/libstdc++.so.6+0xdb3b8)
    #2 0x7ffcb7c5577f  ([stack]+0x1e77f)
    #3 0x4057ab in main cthread_test.cpp:513
    #4 0x42282f  (cthread_test+0x42282f)

==22473==ABORTING

[jimfcarroll]

@a1rwulf I believe you're correct. Ideally this should be fixed by removing CThread. Inheriting from CThread is an anti-patternn

@garbear on autodelete; if thread management were independent of business functionality then this wouldn't be needed. IOW, if there was a thread pool based executor that supported daemon threads then ALL thread management would be delegated to the executor and no one would need to inherit from CThread (and CThread could be deleted). I had started work on this once a while back. It proved to be quite invasive so I dropped it. Maybe I'll pick it up again.

[jimfcarroll]

FWIW: master...jimfcarroll:xbmc:executor