From 76a1cae5d8c9cf0279a5b7a5e5caeadfa0c76fd9 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Wed, 8 Feb 2017 17:04:46 +0100 Subject: [PATCH 1/8] {input,mixer}/alsa: fix off-by-one bug in count check Doesn't make a practical difference - but it's more correct this way. --- src/input/plugins/AlsaInputPlugin.cxx | 2 +- src/mixer/plugins/AlsaMixerPlugin.cxx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx index 4b3fd38de..7c5e55e56 100644 --- a/src/input/plugins/AlsaInputPlugin.cxx +++ b/src/input/plugins/AlsaInputPlugin.cxx @@ -181,7 +181,7 @@ AlsaInputStream::PrepareSockets() } int count = snd_pcm_poll_descriptors_count(capture_handle); - if (count < 0) { + if (count <= 0) { ClearSocketList(); return std::chrono::steady_clock::duration(-1); } diff --git a/src/mixer/plugins/AlsaMixerPlugin.cxx b/src/mixer/plugins/AlsaMixerPlugin.cxx index 255439932..4f0e329a8 100644 --- a/src/mixer/plugins/AlsaMixerPlugin.cxx +++ b/src/mixer/plugins/AlsaMixerPlugin.cxx @@ -102,7 +102,7 @@ AlsaMixerMonitor::PrepareSockets() } int count = snd_mixer_poll_descriptors_count(mixer); - if (count < 0) + if (count <= 0) count = 0; struct pollfd *pfds = pfd_buffer.Get(count); From 3c55487a16efbb4ac1b615e2bf5283f59b7408da Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 13:09:03 +0100 Subject: [PATCH 2/8] configure.ac: don't require libsidutils when building with libsidplayfp The libsidplayfp fork has merged libsidutils into the main library. The libsidutils we used to link with was part of the original libsidplay project. --- NEWS | 2 ++ configure.ac | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/NEWS b/NEWS index 665e13353..7ba59b10a 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ ver 0.20.5 (not yet released) * tags - id3: fix memory leak on corrupt ID3 tags +* decoder + - sidplay: don't require libsidutils when building with libsidplayfp ver 0.20.4 (2017/02/01) * input diff --git a/configure.ac b/configure.ac index a53913a2d..06d593383 100644 --- a/configure.ac +++ b/configure.ac @@ -992,7 +992,7 @@ AM_CONDITIONAL(ENABLE_VORBIS_DECODER, test x$enable_vorbis = xyes || test x$enab dnl --------------------------------- sidplay --------------------------------- if test x$enable_sidplay != xno; then dnl Check for libsidplayfp first - PKG_CHECK_MODULES([SIDPLAY], [libsidplayfp libsidutils], + PKG_CHECK_MODULES([SIDPLAY], [libsidplayfp], [found_sidplayfp=yes], [found_sidplayfp=no]) found_sidplay=$found_sidplayfp From 4e5271fcdf74f55959b8e6b88aff787d57600f8d Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 21:10:55 +0100 Subject: [PATCH 3/8] event/Call: allow usage during shutdown Change EventLoop::IsInside() call to EventLoop::IsInsideOrNull(). This means that BlockingCall() may be used during shutdown, after the main EventLoop::Run() has finished. This is important because mixers are currently registered in the main EventLoop. --- src/event/Call.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/event/Call.cxx b/src/event/Call.cxx index f3eb82235..ded252645 100644 --- a/src/event/Call.cxx +++ b/src/event/Call.cxx @@ -79,7 +79,7 @@ private: void BlockingCall(EventLoop &loop, std::function &&f) { - if (loop.IsInside()) { + if (loop.IsInsideOrNull()) { /* we're already inside the loop - we can simply call the function */ f(); From e92e5e8eb8a40ea2383e4117ee7756dc6c118a21 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 20:28:36 +0100 Subject: [PATCH 4/8] event/MultiSocketMonitor: more API documentation Now ClearSocketList() may only be called from PrepareSockets(). Calling it before destroying the object doesn't work properly, because it doesn't unregister the TimeoutMonitor and the IdleMonitor. Some of its callers need to be fixed. --- src/event/MultiSocketMonitor.hxx | 34 +++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/src/event/MultiSocketMonitor.hxx b/src/event/MultiSocketMonitor.hxx index 40ce1b454..f138fa31c 100644 --- a/src/event/MultiSocketMonitor.hxx +++ b/src/event/MultiSocketMonitor.hxx @@ -96,7 +96,19 @@ class MultiSocketMonitor : IdleMonitor, TimeoutMonitor friend class SingleFD; - bool ready, refresh; + /** + * DispatchSockets() should be called. + */ + bool ready; + + /** + * PrepareSockets() should be called. + * + * Note that this doesn't need to be initialized by the + * constructor; this class is activated with the first + * InvalidateSockets() call, which initializes this flag. + */ + bool refresh; std::forward_list fds; @@ -121,12 +133,19 @@ public: IdleMonitor::Schedule(); } + /** + * Add one socket to the list of monitored sockets. + * + * May only be called from PrepareSockets(). + */ void AddSocket(int fd, unsigned events) { fds.emplace_front(*this, fd, events); } /** * Remove all sockets. + * + * May only be called from PrepareSockets(). */ void ClearSocketList(); @@ -135,6 +154,8 @@ public: * each one; its return value is the events bit mask. A * return value of 0 means the socket will be removed from the * list. + * + * May only be called from PrepareSockets(). */ template void UpdateSocketList(E &&e) { @@ -157,15 +178,26 @@ public: /** * Replace the socket list with the given file descriptors. * The given pollfd array will be modified by this method. + * + * May only be called from PrepareSockets(). */ void ReplaceSocketList(pollfd *pfds, unsigned n); #endif protected: /** + * Override this method and update the socket registrations. + * To do that, call AddSocket(), ClearSocketList(), + * UpdateSocketList() and ReplaceSocketList(). + * * @return timeout or a negative value for no timeout */ virtual std::chrono::steady_clock::duration PrepareSockets() = 0; + + /** + * At least one socket is ready or the timeout has expired. + * This method should be used to perform I/O. + */ virtual void DispatchSockets() = 0; private: From 4b30ef1cf216df3d5395033c2ad7cfaff4aeaa51 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 20:45:29 +0100 Subject: [PATCH 5/8] event/MultiSocketMonitor: use C++11 initializer --- src/event/MultiSocketMonitor.cxx | 2 +- src/event/MultiSocketMonitor.hxx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/event/MultiSocketMonitor.cxx b/src/event/MultiSocketMonitor.cxx index aa8450da8..9a28dd71d 100644 --- a/src/event/MultiSocketMonitor.cxx +++ b/src/event/MultiSocketMonitor.cxx @@ -28,7 +28,7 @@ #endif MultiSocketMonitor::MultiSocketMonitor(EventLoop &_loop) - :IdleMonitor(_loop), TimeoutMonitor(_loop), ready(false) { + :IdleMonitor(_loop), TimeoutMonitor(_loop) { } MultiSocketMonitor::~MultiSocketMonitor() diff --git a/src/event/MultiSocketMonitor.hxx b/src/event/MultiSocketMonitor.hxx index f138fa31c..d6c400b9e 100644 --- a/src/event/MultiSocketMonitor.hxx +++ b/src/event/MultiSocketMonitor.hxx @@ -99,7 +99,7 @@ class MultiSocketMonitor : IdleMonitor, TimeoutMonitor /** * DispatchSockets() should be called. */ - bool ready; + bool ready = false; /** * PrepareSockets() should be called. From eda06993f8cba429e247cac5e75b1e5938fb3a3e Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 20:42:33 +0100 Subject: [PATCH 6/8] event/MultiSocketMonitor: add method Reset() --- src/event/MultiSocketMonitor.cxx | 10 ++++++++-- src/event/MultiSocketMonitor.hxx | 19 +++++++++++++++++-- src/input/plugins/AlsaInputPlugin.cxx | 6 +----- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/src/event/MultiSocketMonitor.cxx b/src/event/MultiSocketMonitor.cxx index 9a28dd71d..02a51e96f 100644 --- a/src/event/MultiSocketMonitor.cxx +++ b/src/event/MultiSocketMonitor.cxx @@ -31,9 +31,15 @@ MultiSocketMonitor::MultiSocketMonitor(EventLoop &_loop) :IdleMonitor(_loop), TimeoutMonitor(_loop) { } -MultiSocketMonitor::~MultiSocketMonitor() +void +MultiSocketMonitor::Reset() { - // TODO + assert(GetEventLoop().IsInsideOrNull()); + + fds.clear(); + IdleMonitor::Cancel(); + TimeoutMonitor::Cancel(); + ready = refresh = false; } void diff --git a/src/event/MultiSocketMonitor.hxx b/src/event/MultiSocketMonitor.hxx index d6c400b9e..db6bec3cb 100644 --- a/src/event/MultiSocketMonitor.hxx +++ b/src/event/MultiSocketMonitor.hxx @@ -119,11 +119,26 @@ public: static constexpr unsigned HANGUP = SocketMonitor::HANGUP; MultiSocketMonitor(EventLoop &_loop); - ~MultiSocketMonitor(); using IdleMonitor::GetEventLoop; -public: + /** + * Clear the socket list and disable all #EventLoop + * registrations. Run this in the #EventLoop thread before + * destroying this object. + * + * Later, this object can be reused and reactivated by calling + * InvalidateSockets(). + * + * Note that this class doesn't have a destructor which calls + * this method, because this would be racy and thus pointless: + * at the time ~MultiSocketMonitor() is called, our virtual + * methods have been morphed to be pure again, and in the + * meantime the #EventLoop thread could invoke those pure + * methods. + */ + void Reset(); + /** * Invalidate the socket list. A call to PrepareSockets() is * scheduled which will then update the list. diff --git a/src/input/plugins/AlsaInputPlugin.cxx b/src/input/plugins/AlsaInputPlugin.cxx index 7c5e55e56..40967dd40 100644 --- a/src/input/plugins/AlsaInputPlugin.cxx +++ b/src/input/plugins/AlsaInputPlugin.cxx @@ -98,12 +98,8 @@ public: } ~AlsaInputStream() { - /* ClearSocketList must be called from within the - IOThread; if we don't do it manually here, the - ~MultiSocketMonitor() will do it in the current - thread */ BlockingCall(MultiSocketMonitor::GetEventLoop(), [this](){ - ClearSocketList(); + MultiSocketMonitor::Reset(); }); snd_pcm_close(capture_handle); From 29e1b6e4653b4ab65b9fe5326b1df8fe7cb6a1d2 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 20:57:14 +0100 Subject: [PATCH 7/8] mixer/alsa: reset the MultiSocketMonitor in the destructor Fixes potential crash bug. --- NEWS | 2 ++ src/mixer/plugins/AlsaMixerPlugin.cxx | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/NEWS b/NEWS index 7ba59b10a..1fcae0d6d 100644 --- a/NEWS +++ b/NEWS @@ -3,6 +3,8 @@ ver 0.20.5 (not yet released) - id3: fix memory leak on corrupt ID3 tags * decoder - sidplay: don't require libsidutils when building with libsidplayfp +* mixer + - alsa: fix crash bug ver 0.20.4 (2017/02/01) * input diff --git a/src/mixer/plugins/AlsaMixerPlugin.cxx b/src/mixer/plugins/AlsaMixerPlugin.cxx index 4f0e329a8..852326926 100644 --- a/src/mixer/plugins/AlsaMixerPlugin.cxx +++ b/src/mixer/plugins/AlsaMixerPlugin.cxx @@ -23,6 +23,7 @@ #include "output/OutputAPI.hxx" #include "event/MultiSocketMonitor.hxx" #include "event/DeferredMonitor.hxx" +#include "event/Call.hxx" #include "util/ASCII.hxx" #include "util/ReusableArray.hxx" #include "util/Domain.hxx" @@ -53,6 +54,12 @@ public: DeferredMonitor::Schedule(); } + ~AlsaMixerMonitor() { + BlockingCall(MultiSocketMonitor::GetEventLoop(), [this](){ + MultiSocketMonitor::Reset(); + }); + } + private: virtual void RunDeferred() override { InvalidateSockets(); From 7372c931b3d1cbf4237b37750aa76602fc0382e7 Mon Sep 17 00:00:00 2001 From: Max Kellermann Date: Thu, 9 Feb 2017 21:21:49 +0100 Subject: [PATCH 8/8] event/Loop: make IsInsideOrNull() available in the NDEBUG build Fixes build breakage by commit 4e5271fcdf7; and this method does make sense in non-debug builds. --- src/event/Loop.cxx | 3 ++- src/event/Loop.hxx | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/event/Loop.cxx b/src/event/Loop.cxx index 3420676a1..ada820c2d 100644 --- a/src/event/Loop.cxx +++ b/src/event/Loop.cxx @@ -222,8 +222,9 @@ EventLoop::Run() #ifndef NDEBUG assert(busy); assert(thread.IsInside()); - thread = ThreadId::Null(); #endif + + thread = ThreadId::Null(); } void diff --git a/src/event/Loop.hxx b/src/event/Loop.hxx index 7c0e0b028..f1962b4df 100644 --- a/src/event/Loop.hxx +++ b/src/event/Loop.hxx @@ -212,12 +212,16 @@ public: } #endif -#ifndef NDEBUG + /** + * Like IsInside(), but also returns true if the thread has + * already ended (or was not started yet). This is useful for + * code which may run during startup or shutdown, when events + * are not yet/anymore handled. + */ gcc_pure bool IsInsideOrNull() const { return thread.IsNull() || thread.IsInside(); } -#endif }; #endif /* MAIN_NOTIFY_H */