From 64d1525f50b52c1a2a797ba138d80b25b86a9331 Mon Sep 17 00:00:00 2001 From: Srivats P Date: Sat, 10 Aug 2019 13:16:18 +0530 Subject: [PATCH] Fix infinite loop when stopping capture etc. On some platforms and/or some libpcap verisons, libpcap doesn't support a timeout which makes interactive stop not possible. So we now use a UNIX signal to break out. Obviously this works only on *nix platforms - which includes MacOS. For now the problem is not seen on Windows with WinPCAP, so we should be fine. May need to revisit when we add Npcap support. Fixes #215, #234 --- server/drone.pro | 1 + server/linuxport.cpp | 2 + server/pcapport.cpp | 22 +++++++++-- server/pcapport.h | 7 ++-- server/pcaprxstats.cpp | 13 +++++-- server/pcaprxstats.h | 5 +-- server/pcapsession.cpp | 85 ++++++++++++++++++++++++++++++++++++++++++ server/pcapsession.h | 82 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 204 insertions(+), 13 deletions(-) create mode 100644 server/pcapsession.cpp create mode 100644 server/pcapsession.h diff --git a/server/drone.pro b/server/drone.pro index 48f4064..4aa591d 100644 --- a/server/drone.pro +++ b/server/drone.pro @@ -50,6 +50,7 @@ SOURCES += \ portmanager.cpp \ abstractport.cpp \ pcapport.cpp \ + pcapsession.cpp \ pcaptransmitter.cpp \ pcaprxstats.cpp \ pcaptxstats.cpp \ diff --git a/server/linuxport.cpp b/server/linuxport.cpp index 2498a79..b468dcf 100644 --- a/server/linuxport.cpp +++ b/server/linuxport.cpp @@ -73,6 +73,8 @@ LinuxPort::LinuxPort(int id, const char *device) populateInterfaceInfo(); // We don't need per port Rx/Tx monitors for Linux + // No need to stop them because we start them only in + // PcapPort::init which has not yet been called delete monitorRx_; delete monitorTx_; monitorRx_ = monitorTx_ = NULL; diff --git a/server/pcapport.cpp b/server/pcapport.cpp index 362a750..fbb9c84 100644 --- a/server/pcapport.cpp +++ b/server/pcapport.cpp @@ -404,6 +404,7 @@ _retry: } dumpHandle_ = pcap_dump_open(handle_, qPrintable(capFile_.fileName())); + PcapSession::preRun(); state_ = kRunning; while (1) { @@ -425,16 +426,21 @@ _retry: __PRETTY_FUNCTION__, ret, pcap_geterr(handle_)); break; case -2: + qDebug("Loop/signal break or some other error"); + break; default: - qFatal("%s: Unexpected return value %d", __PRETTY_FUNCTION__, ret); + qWarning("%s: Unexpected return value %d", __PRETTY_FUNCTION__, ret); + stop_ = true; } if (stop_) { - qDebug("user requested capture stop\n"); + qDebug("user requested capture stop"); break; } } + PcapSession::postRun(); + pcap_dump_close(dumpHandle_); pcap_close(handle_); dumpHandle_ = NULL; @@ -464,6 +470,7 @@ void PcapPort::PortCapturer::stop() { if (state_ == kRunning) { stop_ = true; + PcapSession::stop(handle_); while (state_ == kRunning) QThread::msleep(10); } @@ -606,6 +613,7 @@ _retry: } _skip_filter: + PcapSession::preRun(); state_ = kRunning; while (1) { @@ -643,17 +651,22 @@ _skip_filter: __PRETTY_FUNCTION__, ret, pcap_geterr(handle_)); break; case -2: + qDebug("Loop/signal break or some other error"); + break; default: - qFatal("%s: Unexpected return value %d", __PRETTY_FUNCTION__, + qWarning("%s: Unexpected return value %d", __PRETTY_FUNCTION__, ret); + stop_ = true; } if (stop_) { - qDebug("user requested receiver stop\n"); + qDebug("user requested receiver stop"); break; } } + PcapSession::postRun(); + pcap_close(handle_); handle_ = NULL; stop_ = false; @@ -680,6 +693,7 @@ void PcapPort::EmulationTransceiver::stop() { if (state_ == kRunning) { stop_ = true; + PcapSession::stop(handle_); while (state_ == kRunning) QThread::msleep(10); } diff --git a/server/pcapport.h b/server/pcapport.h index cb99f34..cf59119 100644 --- a/server/pcapport.h +++ b/server/pcapport.h @@ -27,6 +27,7 @@ along with this program. If not, see #include "abstractport.h" #include "pcapextra.h" #include "pcaprxstats.h" +#include "pcapsession.h" #include "pcaptransmitter.h" class PcapPort : public AbstractPort @@ -84,7 +85,7 @@ protected: kDirectionTx }; - class PortMonitor: public QThread + class PortMonitor: public QThread // TODO: inherit from PcapSession (only if required) { public: PortMonitor(const char *device, Direction direction, @@ -106,7 +107,7 @@ protected: bool isPromisc_; }; - class PortCapturer: public QThread + class PortCapturer: public PcapSession { public: PortCapturer(const char *device); @@ -133,7 +134,7 @@ protected: volatile State state_; }; - class EmulationTransceiver: public QThread + class EmulationTransceiver: public PcapSession { public: EmulationTransceiver(const char *device, DeviceManager *deviceManager); diff --git a/server/pcaprxstats.cpp b/server/pcaprxstats.cpp index 826035f..e1b0a7c 100644 --- a/server/pcaprxstats.cpp +++ b/server/pcaprxstats.cpp @@ -104,6 +104,7 @@ void PcapRxStats::run() } _skip_filter: + PcapSession::preRun(); state_ = kRunning; while (1) { int ret; @@ -128,16 +129,21 @@ _skip_filter: __PRETTY_FUNCTION__, ret, pcap_geterr(handle_)); break; case -2: + qDebug("Loop/signal break or some other error"); + break; default: - qFatal("%s: Unexpected return value %d", __PRETTY_FUNCTION__, + qWarning("%s: Unexpected return value %d", __PRETTY_FUNCTION__, ret); + stop_ = true; } if (stop_) { - qDebug("user requested receiver stop\n"); + qDebug("user requested rxstats stop"); break; } } + PcapSession::postRun(); + pcap_close(handle_); handle_ = NULL; stop_ = false; @@ -154,7 +160,7 @@ bool PcapRxStats::start() } state_ = kNotStarted; - QThread::start(); + PcapSession::start(); while (state_ == kNotStarted) QThread::msleep(10); @@ -166,6 +172,7 @@ bool PcapRxStats::stop() { if (state_ == kRunning) { stop_ = true; + PcapSession::stop(handle_); while (state_ == kRunning) QThread::msleep(10); } diff --git a/server/pcaprxstats.h b/server/pcaprxstats.h index 9d006f9..d6e4d9e 100644 --- a/server/pcaprxstats.h +++ b/server/pcaprxstats.h @@ -22,10 +22,9 @@ along with this program. If not, see #include "streamstats.h" -#include -#include +#include "pcapsession.h" -class PcapRxStats: public QThread +class PcapRxStats: public PcapSession { public: PcapRxStats(const char *device, StreamStats &portStreamStats); diff --git a/server/pcapsession.cpp b/server/pcapsession.cpp new file mode 100644 index 0000000..94ef00b --- /dev/null +++ b/server/pcapsession.cpp @@ -0,0 +1,85 @@ +/* +Copyright (C) 2019 Srivats P. + +This file is part of "Ostinato" + +This is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see +*/ + +#include "pcapsession.h" + +#ifdef Q_OS_UNIX +#include + +#define MY_BREAK_SIGNAL SIGUSR1 + +QHash PcapSession::signalSeen_; + +void PcapSession::preRun() +{ + // Should be called in the thread's context + thread_ = pthread_self(); + + struct sigaction sa; + memset(&sa, 0, sizeof(sa)); + sigemptyset(&sa.sa_mask); + sa.sa_handler = PcapSession::signalBreakHandler; + if (!sigaction(MY_BREAK_SIGNAL, &sa, NULL)) { + signalSeen_[thread_] = false; + qDebug("Break signal handler installed"); + } + else + qWarning("Failed to install MY_BREAK_SIGNAL handler"); +} + +void PcapSession::postRun() +{ + // Should be called in the thread's context + ThreadId id = pthread_self(); + qDebug("In %s::%s", typeid(*this).name(), __FUNCTION__); + if (!signalSeen_.contains(id)) { + qWarning("Thread not found in signalSeen"); + return; + } + + bool &seen = signalSeen_[id]; + // XXX: don't exit the thread until we see the signal; if we don't + // some platforms will crash + if (!seen) { + qDebug("Wait for signal"); + while (!seen) + QThread::msleep(10); + } + signalSeen_.remove(id); + qDebug("Signal seen and handled"); +} + +void PcapSession::stop(pcap_t *handle) +{ + // Should be called OUTSIDE the thread's context + // XXX: As per the man page for pcap_breakloop, we need both + // pcap_breakloop and a mechanism to interrupt system calls; + // we use a signal for the latter + // TODO: If the signal mechanism doesn't work, we could try + // pthread_cancel(thread_); + pcap_breakloop(handle); + pthread_kill(thread_.nativeId(), MY_BREAK_SIGNAL); +} + +void PcapSession::signalBreakHandler(int /*signum*/) +{ + qDebug("In %s", __FUNCTION__); + signalSeen_[pthread_self()] = true; +} +#endif diff --git a/server/pcapsession.h b/server/pcapsession.h new file mode 100644 index 0000000..e583304 --- /dev/null +++ b/server/pcapsession.h @@ -0,0 +1,82 @@ +/* +Copyright (C) 2019 Srivats P. + +This file is part of "Ostinato" + +This is free software: you can redistribute it and/or modify +it under the terms of the GNU General Public License as published by +the Free Software Foundation, either version 3 of the License, or +(at your option) any later version. + +This program is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +GNU General Public License for more details. + +You should have received a copy of the GNU General Public License +along with this program. If not, see +*/ + +#ifndef _PCAP_SESSION_H +#define _PCAP_SESSION_H + +#include +#include + +#ifdef Q_OS_UNIX +#include +#include +class ThreadId { +public: + ThreadId() { + id_ = pthread_self(); + } + ThreadId(pthread_t id) { + id_ = id; + } + pthread_t nativeId() { + return id_; + } + uint hash() const { + QByteArray t((const char*)(&id_), sizeof(id_)); + return qHash(t); + } + bool operator==(const ThreadId &other) const { + return (pthread_equal(id_, other.id_) != 0); + } +private: + pthread_t id_; +}; + +inline uint qHash(const ThreadId &key) +{ + return key.hash(); +} + +class PcapSession: public QThread +{ +protected: + void preRun(); + void postRun(); + void stop(pcap_t *handle); + +private: + static void signalBreakHandler(int /*signum*/); + + ThreadId thread_; + static QHash signalSeen_; +}; +#else +class PcapSession: public QThread +{ +protected: + void preRun() {}; + void postRun() {}; + void stop(pcap_t *handle) { + qDebug("calling breakloop with handle %p", handle); + pcap_breakloop(handle); + } +}; +#endif + +#endif