Fix bugs in thread termination causing occasional pthread
authorCarl Hetherington <cth@carlh.net>
Wed, 29 Jul 2020 18:22:54 +0000 (20:22 +0200)
committerCarl Hetherington <cth@carlh.net>
Wed, 29 Jul 2020 18:22:54 +0000 (20:22 +0200)
assertion failures.

Before this, it was possible for J2KEncoder::terminate_threads()
to finish without terminating all threads if the thread _running_
terminate_threads() was itself interrupt()ed.

This is because the thread_group::join_all() in terminate_threads()
is an interruption point, so it was possible it not to complete
but instead to throw interrupted_exception.  Then the owning
J2KEncoder would be torn down but the threads would still be running,
causing use-after-frees.

This commit adds some boost::this_thread::disable_interruption
objects to ensure that the owning thread is not interrupted while
it is being destroyed.

Also tidy up code that does this stuff, assuming that it's safe
to not call thread::joinable but instead do

thread.interrupt();
try {
  thread.join();
} catch (...) {}

12 files changed:
src/lib/butler.cc
src/lib/checker.cc
src/lib/encode_server.cc
src/lib/encode_server_finder.cc
src/lib/hints.cc
src/lib/j2k_encoder.cc
src/lib/job.cc
src/lib/job_manager.cc
src/lib/update_checker.cc
src/lib/writer.cc
src/wx/gl_video_view.cc
test/socket_test.cc

index 595d988..df23580 100644 (file)
@@ -101,6 +101,8 @@ Butler::Butler (
 
 Butler::~Butler ()
 {
+       boost::this_thread::disable_interruption dis;
+
        {
                boost::mutex::scoped_lock lm (_mutex);
                _stop_thread = true;
@@ -113,9 +115,7 @@ Butler::~Butler ()
        _thread.interrupt ();
        try {
                _thread.join ();
-       } catch (...) {
-
-       }
+       } catch (...) {}
 }
 
 /** Caller must hold a lock on _mutex */
index c32462b..b96b965 100644 (file)
@@ -43,19 +43,17 @@ Checker::run ()
 
 Checker::~Checker ()
 {
+       boost::this_thread::disable_interruption dis;
+
        {
                boost::mutex::scoped_lock lm (_mutex);
                _terminate = true;
        }
 
-       if (_thread.joinable()) {
-               _thread.interrupt ();
-               try {
-                       _thread.join ();
-               } catch (...) {
-
-               }
-       }
+       _thread.interrupt ();
+       try {
+               _thread.join ();
+       } catch (...) {}
 }
 
 void
index c30fc8f..f422479 100644 (file)
@@ -83,6 +83,8 @@ EncodeServer::EncodeServer (bool verbose, int num_threads)
 
 EncodeServer::~EncodeServer ()
 {
+       boost::this_thread::disable_interruption dis;
+
        {
                boost::mutex::scoped_lock lm (_mutex);
                _terminate = true;
@@ -104,13 +106,9 @@ EncodeServer::~EncodeServer ()
        }
 
        _broadcast.io_service.stop ();
-       if (_broadcast.thread.joinable()) {
-               try {
-                       _broadcast.thread.join ();
-               } catch (...) {
-
-               }
-       }
+       try {
+               _broadcast.thread.join ();
+       } catch (...) {}
 }
 
 /** @param after_read Filled in with gettimeofday() after reading the input from the network.
index 52c8c89..7297734 100644 (file)
@@ -70,25 +70,19 @@ EncodeServerFinder::~EncodeServerFinder ()
 void
 EncodeServerFinder::stop ()
 {
+       boost::this_thread::disable_interruption dis;
+
        _stop = true;
 
        _search_condition.notify_all ();
-       if (_search_thread.joinable()) {
-               try {
-                       _search_thread.join();
-               } catch (...) {
-
-               }
-       }
+       try {
+               _search_thread.join();
+       } catch (...) {}
 
        _listen_io_service.stop ();
-       if (_listen_thread.joinable()) {
-               try {
-                       _listen_thread.join ();
-               } catch (...) {
-
-               }
-       }
+       try {
+               _listen_thread.join ();
+       } catch (...) {}
 
        boost::mutex::scoped_lock lm (_servers_mutex);
        _servers.clear ();
index 9cc10d7..e936332 100644 (file)
@@ -69,17 +69,13 @@ Hints::start ()
 
 Hints::~Hints ()
 {
-       if (!_thread.joinable()) {
-               return;
-       }
+       boost::this_thread::disable_interruption dis;
 
        try {
                _stop = true;
                _thread.interrupt ();
                _thread.join ();
-       } catch (...) {
-
-       }
+       } catch (...) {}
 }
 
 void
index d8d8209..4e2a30e 100644 (file)
@@ -64,14 +64,7 @@ J2KEncoder::J2KEncoder (shared_ptr<const Film> film, shared_ptr<Writer> writer)
 
 J2KEncoder::~J2KEncoder ()
 {
-       try {
-               terminate_threads ();
-       } catch (...) {
-               /* Destructors must not throw exceptions; anything bad
-                  happening now is too late to worry about anyway,
-                  I think.
-               */
-       }
+       terminate_threads ();
 }
 
 void
@@ -246,6 +239,8 @@ J2KEncoder::encode (shared_ptr<PlayerVideo> pv, DCPTime time)
 void
 J2KEncoder::terminate_threads ()
 {
+       boost::this_thread::disable_interruption dis;
+
        if (!_threads) {
                return;
        }
index 0feda64..d960dfa 100644 (file)
@@ -71,16 +71,12 @@ Job::~Job ()
 void
 Job::stop_thread ()
 {
-       if (!_thread.joinable()) {
-               return;
-       }
+       boost::this_thread::disable_interruption dis;
 
        _thread.interrupt ();
        try {
                _thread.join ();
-       } catch (...) {
-               /* Too late to do anything about this */
-       }
+       } catch (...) {}
 }
 
 /** Start the job in a separate thread, returning immediately */
index c40178f..9eb51ae 100644 (file)
@@ -62,6 +62,8 @@ JobManager::start ()
 
 JobManager::~JobManager ()
 {
+       boost::this_thread::disable_interruption dis;
+
        BOOST_FOREACH (boost::signals2::connection& i, _connections) {
                i.disconnect ();
        }
@@ -72,13 +74,9 @@ JobManager::~JobManager ()
                _empty_condition.notify_all ();
        }
 
-       if (_scheduler.joinable()) {
-               try {
-                       _scheduler.join();
-               } catch (...) {
-
-               }
-       }
+       try {
+               _scheduler.join();
+       } catch (...) {}
 }
 
 shared_ptr<Job>
index 82337f9..0fad330 100644 (file)
@@ -81,19 +81,17 @@ UpdateChecker::start ()
 
 UpdateChecker::~UpdateChecker ()
 {
+       boost::this_thread::disable_interruption dis;
+
        {
                boost::mutex::scoped_lock lm (_process_mutex);
                _terminate = true;
        }
 
        _condition.notify_all ();
-       if (_thread.joinable()) {
-               try {
-                       _thread.join ();
-               } catch (...) {
-
-               }
-       }
+       try {
+               _thread.join ();
+       } catch (...) {}
 
        curl_easy_cleanup (_curl);
        delete[] _buffer;
index 7d10882..dc552e6 100644 (file)
@@ -489,17 +489,18 @@ catch (...)
 void
 Writer::terminate_thread (bool can_throw)
 {
+       boost::this_thread::disable_interruption dis;
+
        boost::mutex::scoped_lock lock (_state_mutex);
-       if (!_thread.joinable()) {
-               return;
-       }
 
        _finish = true;
        _empty_condition.notify_all ();
        _full_condition.notify_all ();
        lock.unlock ();
 
-       _thread.join ();
+       try {
+               _thread.join ();
+       } catch (...) {}
 
        if (can_throw) {
                rethrow ();
index a87249f..fe49a5b 100644 (file)
@@ -104,12 +104,12 @@ GLVideoView::GLVideoView (FilmViewer* viewer, wxWindow *parent)
 
 GLVideoView::~GLVideoView ()
 {
+       boost::this_thread::disable_interruption dis;
+
        try {
                _thread.interrupt ();
                _thread.join ();
-       } catch (...) {
-
-       }
+       } catch (...) {}
 
        glDeleteTextures (1, &_id);
 }
index 562d106..5024272 100644 (file)
@@ -48,8 +48,11 @@ public:
 
        ~TestServer ()
        {
+               boost::this_thread::disable_interruption dis;
                stop ();
-               _thread.join ();
+               try {
+                       _thread.join ();
+               } catch (...) {}
                delete[] _buffer;
        }