From 1a6474976c9bb606ecc15ea52841156b44c8b1e9 Mon Sep 17 00:00:00 2001 From: Bill Somerville Date: Thu, 24 Sep 2020 17:53:21 +0100 Subject: [PATCH] Wrap QProcess to avoid inherited handles causing issues on Windows --- CMakeLists.txt | 1 + NonInheritingProcess.cpp | 114 +++++++++++++++++++++++++++++++++++++++ NonInheritingProcess.hpp | 35 ++++++++++++ 3 files changed, 150 insertions(+) create mode 100644 NonInheritingProcess.cpp create mode 100644 NonInheritingProcess.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 9418ee60d..fcd5a8a2e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -236,6 +236,7 @@ set (wsjt_qt_CXXSRCS WFPalette.cpp Radio.cpp RadioMetaType.cpp + NonInheritingProcess.cpp models/IARURegions.cpp models/Bands.cpp models/Modes.cpp diff --git a/NonInheritingProcess.cpp b/NonInheritingProcess.cpp new file mode 100644 index 000000000..595d9038f --- /dev/null +++ b/NonInheritingProcess.cpp @@ -0,0 +1,114 @@ +#include "NonInheritingProcess.hpp" + +#ifdef Q_OS_WIN +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#ifndef NOMINMAX +#define NOMINMAX +#endif +#ifndef _UNICODE +#define _UNICODE +#endif +#ifdef _WIN32_WINNT +#undef _WIN32_WINNT +#endif +#define _WIN32_WINNT 0x0601 +#include +#endif + +#include +#include + +#include "pimpl_impl.hpp" + +namespace +{ + struct start_info_deleter + { + void operator () (STARTUPINFOEXW * si) + { + if (si->lpAttributeList) + { + ::DeleteProcThreadAttributeList (si->lpAttributeList); + } + delete si; + } + }; +} + +class NonInheritingProcess::impl +{ +public: +#ifdef Q_OS_WIN + void extend_CreateProcessArguments (QProcess::CreateProcessArguments * args) + { + // + // Here we modify the CreateProcessArguments structure to use a + // STARTUPINFOEX extended argument to CreateProcess. In that we + // set up a list of handles for the new process to inherit. By + // doing this we stop all inherited handles from being + // inherited. Unfortunately UpdateProcThreadAttribute does not let + // us set up an empty handle list, so we populate the list with + // the three standard stream handles that QProcess::start has set + // up as Pipes to do IPC. Even though these Pipe handles are + // created with inheritance disabled, UpdateProcThreadAtribute and + // CreateProcess don't seem to mind, which suits us fine. + // + // Note: that we cannot just clear the inheritHandles flag as that + // stops the standard stream handles being inherited which breaks + // our IPC using std(in|out|err). Only be using a + // PROC_THREAD_ATTRIBUTE_HANDLE_LIST attribute in a STARTUPINFOEX + // structure can we avoid the all or nothing behaviour of + // CreateProcess /w respect to handle inheritance. + // + BOOL fSuccess; + SIZE_T size {0}; + LPPROC_THREAD_ATTRIBUTE_LIST lpAttributeList = nullptr; + ::InitializeProcThreadAttributeList (nullptr, 1, 0, &size); + lpAttributeList = reinterpret_cast (::HeapAlloc (::GetProcessHeap (), 0, size)); + fSuccess = !!lpAttributeList; + if (fSuccess) + { + fSuccess = ::InitializeProcThreadAttributeList (lpAttributeList, 1, 0, &size); + } + if (fSuccess) + { + // empty list of handles + fSuccess = ::UpdateProcThreadAttribute (lpAttributeList, 0, + PROC_THREAD_ATTRIBUTE_HANDLE_LIST, + &args->startupInfo->hStdInput, 3 * sizeof (HANDLE), + nullptr, 0); + } + if (fSuccess) + { + start_info_.reset (new STARTUPINFOEXW); + start_info_->StartupInfo = *args->startupInfo; + start_info_->StartupInfo.cb = sizeof (STARTUPINFOEXW); + start_info_->lpAttributeList = lpAttributeList; + args->startupInfo = reinterpret_cast (start_info_.get ()); + args->flags |= EXTENDED_STARTUPINFO_PRESENT; + } + } + + using start_info_type = std::unique_ptr; + start_info_type start_info_; +#endif +}; + +NonInheritingProcess::NonInheritingProcess (QObject * parent) + : QProcess {parent} +{ +#ifdef Q_OS_WIN + using namespace std::placeholders; + + // enable cleanup after process starts or fails to start + connect (this, &QProcess::started, [this] {m_->start_info_.reset ();}); + connect (this, &QProcess::errorOccurred, [this] (QProcess::ProcessError) {m_->start_info_.reset ();}); + setCreateProcessArgumentsModifier (std::bind (&NonInheritingProcess::impl::extend_CreateProcessArguments, &*m_, _1)); +#endif +} + +NonInheritingProcess::~NonInheritingProcess () +{ +} diff --git a/NonInheritingProcess.hpp b/NonInheritingProcess.hpp new file mode 100644 index 000000000..c38d41ed3 --- /dev/null +++ b/NonInheritingProcess.hpp @@ -0,0 +1,35 @@ +#ifndef NON_INHERITING_PROCESS_HPP__ +#define NON_INHERITING_PROCESS_HPP__ + +#include +#include "pimpl_h.hpp" + +class QObject; + +// +// class NonInheritingProcess - Manage a process without it inheriting +// all inheritable handles +// +// On MS Windows QProcess creates sub-processes which inherit all +// inheritable handles, and handles on Windows are inheritable by +// default. This can cause the lifetime of objects to be unexpectedly +// extended, which in turn can cause unexpected errors. The motivation +// for this class was implementing log file rotation using the Boost +// log library. The current log file's handle gets inherited by any +// long running sub-process started by QProcess and that causes a +// sharing violation when attempting to rename the log file on +// rotation, even though the log library closes the current log file +// before trying to rename it. +// +class NonInheritingProcess + : public QProcess +{ +public: + NonInheritingProcess (QObject * parent = nullptr); + ~NonInheritingProcess (); + +private: + class impl; + pimpl m_; +}; +#endif