From 39611cbd609fbfc7126a8620f4b1946abd773b7d Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Tue, 25 Aug 2020 16:10:21 +0200 Subject: [PATCH 1/5] Wait for waitingThread to exit before destroying the subprocess memory. WaitingThread read some shared memory with the SubProcess (`mutex`, `m_running`). When we destroy the SubProcess, we must be sure that WaitingThread has correctly finished else we may have invalid read/write on freed memory. --- src/subprocess_unix.cpp | 1 + src/subprocess_windows.cpp | 14 ++++++++------ src/subprocess_windows.h | 3 ++- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/subprocess_unix.cpp b/src/subprocess_unix.cpp index bc4791736..68819255b 100644 --- a/src/subprocess_unix.cpp +++ b/src/subprocess_unix.cpp @@ -26,6 +26,7 @@ UnixImpl::~UnixImpl() #else pthread_cancel(m_waitingThread); #endif + pthread_join(m_waitingThread, nullptr); } #ifdef __ANDROID__ diff --git a/src/subprocess_windows.cpp b/src/subprocess_windows.cpp index 19145811c..98ef02954 100644 --- a/src/subprocess_windows.cpp +++ b/src/subprocess_windows.cpp @@ -11,7 +11,8 @@ WinImpl::WinImpl(): m_pid(0), m_running(false), - m_handle(INVALID_HANDLE_VALUE) + m_subprocessHandle(INVALID_HANDLE_VALUE), + m_waitingThreadHandle(INVALID_HANDLE_VALUE) { InitializeCriticalSection(&m_criticalSection); } @@ -19,14 +20,15 @@ WinImpl::WinImpl(): WinImpl::~WinImpl() { kill(); - CloseHandle(m_handle); + WaitForSingleObject(m_waitingThreadHandle, INFINITE); + CloseHandle(m_subprocessHandle); DeleteCriticalSection(&m_criticalSection); } DWORD WINAPI WinImpl::waitForPID(void* _self) { WinImpl* self = static_cast(_self); - WaitForSingleObject(self->m_handle, INFINITE); + WaitForSingleObject(self->m_subprocessHandle, INFINITE); EnterCriticalSection(&self->m_criticalSection); self->m_running = false; @@ -79,16 +81,16 @@ void WinImpl::run(commandLine_t& commandLine) &procInfo)) { m_pid = procInfo.dwProcessId; - m_handle = procInfo.hProcess; + m_subprocessHandle = procInfo.hProcess; CloseHandle(procInfo.hThread); m_running = true; - CreateThread(NULL, 0, &waitForPID, this, 0, NULL ); + m_waitingThreadHandle = CreateThread(NULL, 0, &waitForPID, this, 0, NULL); } } bool WinImpl::kill() { - return TerminateProcess(m_handle, 0); + return TerminateProcess(m_subprocessHandle, 0); } bool WinImpl::isRunning() diff --git a/src/subprocess_windows.h b/src/subprocess_windows.h index 36e1b0127..d5d3bf1e0 100644 --- a/src/subprocess_windows.h +++ b/src/subprocess_windows.h @@ -11,7 +11,8 @@ class WinImpl : public SubprocessImpl private: int m_pid; bool m_running; - HANDLE m_handle; + HANDLE m_subprocessHandle; + HANDLE m_waitingThreadHandle; CRITICAL_SECTION m_criticalSection; public: From af9e03904c052513c946af1fb64e709bcdfbf6ba Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 26 Aug 2020 12:12:14 +0200 Subject: [PATCH 2/5] Use std::mutex and std::unique_lock instead of pthread mutex/lock. It simplify a bit the code and ensure that mutex is correctly unlock even in case of exception. --- src/aria2.cpp | 45 +++++++++++++++++++++++---------------------- src/aria2.h | 4 ++-- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/aria2.cpp b/src/aria2.cpp index efa9e3f3b..5aea07f45 100644 --- a/src/aria2.cpp +++ b/src/aria2.cpp @@ -26,8 +26,7 @@ Aria2::Aria2(): mp_aria(nullptr), m_port(42042), m_secret("kiwixariarpc"), - mp_curl(nullptr), - m_lock(PTHREAD_MUTEX_INITIALIZER) + mp_curl(nullptr) { m_downloadDir = getDataDirectory(); makeDirectory(m_downloadDir); @@ -115,6 +114,7 @@ Aria2::Aria2(): Aria2::~Aria2() { + std::unique_lock lock(m_lock); curl_easy_cleanup(mp_curl); } @@ -133,31 +133,32 @@ size_t write_callback_to_iss(char* ptr, size_t size, size_t nmemb, void* userdat std::string Aria2::doRequest(const MethodCall& methodCall) { - pthread_mutex_lock(&m_lock); auto requestContent = methodCall.toString(); std::stringstream stringstream; CURLcode res; - curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDSIZE, requestContent.size()); - curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDS, requestContent.c_str()); - curl_easy_setopt(mp_curl, CURLOPT_WRITEFUNCTION, &write_callback_to_iss); - curl_easy_setopt(mp_curl, CURLOPT_WRITEDATA, &stringstream); - res = curl_easy_perform(mp_curl); - if (res == CURLE_OK) { - long response_code; + long response_code; + { + std::unique_lock lock(m_lock); + curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDSIZE, requestContent.size()); + curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDS, requestContent.c_str()); + curl_easy_setopt(mp_curl, CURLOPT_WRITEFUNCTION, &write_callback_to_iss); + curl_easy_setopt(mp_curl, CURLOPT_WRITEDATA, &stringstream); + res = curl_easy_perform(mp_curl); + if (res != CURLE_OK) { + throw std::runtime_error("Cannot perform request"); + } curl_easy_getinfo(mp_curl, CURLINFO_RESPONSE_CODE, &response_code); - pthread_mutex_unlock(&m_lock); - if (response_code != 200) { - throw std::runtime_error("Invalid return code from aria"); - } - auto responseContent = stringstream.str(); - MethodResponse response(responseContent); - if (response.isFault()) { - throw AriaError(response.getFault().getFaultString()); - } - return responseContent; } - pthread_mutex_unlock(&m_lock); - throw std::runtime_error("Cannot perform request"); + + if (response_code != 200) { + throw std::runtime_error("Invalid return code from aria"); + } + auto responseContent = stringstream.str(); + MethodResponse response(responseContent); + if (response.isFault()) { + throw AriaError(response.getFault().getFaultString()); + } + return responseContent; } std::string Aria2::addUri(const std::vector& uris, const std::vector>& options) diff --git a/src/aria2.h b/src/aria2.h index 9108d221e..3401310eb 100644 --- a/src/aria2.h +++ b/src/aria2.h @@ -12,8 +12,8 @@ #include "xmlrpc.h" #include +#include #include -#include namespace kiwix { @@ -25,7 +25,7 @@ class Aria2 std::string m_secret; std::string m_downloadDir; CURL* mp_curl; - pthread_mutex_t m_lock; + std::mutex m_lock; std::string doRequest(const MethodCall& methodCall); From 72d3f8f8e248a08e71ec5184aaaacd04cb877159 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 26 Aug 2020 12:25:21 +0200 Subject: [PATCH 3/5] Fix segmentation fault with curl requests. Use a heap allocated buffer (with lifetime of Aria2 class) instead of a stack allocated one. Original fix made by @ZaWertun. Kudos to him. Fix #kiwix/kiwix-desktop#123, kiwix/kiwix-desktop#513 and kiwix/kiwix-desktop#423 --- src/aria2.cpp | 10 +++++----- src/aria2.h | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/aria2.cpp b/src/aria2.cpp index 5aea07f45..9e0f5eacc 100644 --- a/src/aria2.cpp +++ b/src/aria2.cpp @@ -26,6 +26,7 @@ Aria2::Aria2(): mp_aria(nullptr), m_port(42042), m_secret("kiwixariarpc"), + m_curlErrorBuffer(new char[CURL_ERROR_SIZE]), mp_curl(nullptr) { m_downloadDir = getDataDirectory(); @@ -83,24 +84,23 @@ Aria2::Aria2(): } mp_aria = Subprocess::run(callCmd); mp_curl = curl_easy_init(); - char errbuf[CURL_ERROR_SIZE]; curl_easy_setopt(mp_curl, CURLOPT_URL, "http://localhost/rpc"); curl_easy_setopt(mp_curl, CURLOPT_PORT, m_port); curl_easy_setopt(mp_curl, CURLOPT_POST, 1L); - curl_easy_setopt(mp_curl, CURLOPT_ERRORBUFFER, errbuf); + curl_easy_setopt(mp_curl, CURLOPT_ERRORBUFFER, m_curlErrorBuffer.get()); int watchdog = 50; while(--watchdog) { sleep(10); - errbuf[0] = 0; + m_curlErrorBuffer[0] = 0; auto res = curl_easy_perform(mp_curl); if (res == CURLE_OK) { break; } else if (watchdog == 1) { std::cerr <<" curl_easy_perform() failed." << std::endl; fprintf(stderr, "\nlibcurl: (%d) ", res); - if (errbuf[0] != 0) { - std::cerr << errbuf << std::endl; + if (m_curlErrorBuffer[0] != 0) { + std::cerr << m_curlErrorBuffer.get() << std::endl; } else { std::cerr << curl_easy_strerror(res) << std::endl; } diff --git a/src/aria2.h b/src/aria2.h index 3401310eb..b07018f00 100644 --- a/src/aria2.h +++ b/src/aria2.h @@ -24,6 +24,7 @@ class Aria2 int m_port; std::string m_secret; std::string m_downloadDir; + std::unique_ptr m_curlErrorBuffer; CURL* mp_curl; std::mutex m_lock; From ea3180cb8c5a58e48c4f25cd4a9b0111b36a7a61 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 26 Aug 2020 12:25:41 +0200 Subject: [PATCH 4/5] Better error printing. --- src/aria2.cpp | 21 +++++++++++++-------- src/downloader.cpp | 4 ++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/src/aria2.cpp b/src/aria2.cpp index 9e0f5eacc..11fb48b2d 100644 --- a/src/aria2.cpp +++ b/src/aria2.cpp @@ -19,6 +19,11 @@ #endif +#define LOG_ARIA_ERROR() \ + { \ + std::cerr << "ERROR: aria2 RPC request failed. (" << res << ")." << std::endl; \ + std::cerr << (m_curlErrorBuffer[0] ? m_curlErrorBuffer.get() : curl_easy_strerror(res)) << std::endl; \ + } namespace kiwix { @@ -84,6 +89,7 @@ Aria2::Aria2(): } mp_aria = Subprocess::run(callCmd); mp_curl = curl_easy_init(); + curl_easy_setopt(mp_curl, CURLOPT_URL, "http://localhost/rpc"); curl_easy_setopt(mp_curl, CURLOPT_PORT, m_port); curl_easy_setopt(mp_curl, CURLOPT_POST, 1L); @@ -97,13 +103,7 @@ Aria2::Aria2(): if (res == CURLE_OK) { break; } else if (watchdog == 1) { - std::cerr <<" curl_easy_perform() failed." << std::endl; - fprintf(stderr, "\nlibcurl: (%d) ", res); - if (m_curlErrorBuffer[0] != 0) { - std::cerr << m_curlErrorBuffer.get() << std::endl; - } else { - std::cerr << curl_easy_strerror(res) << std::endl; - } + LOG_ARIA_ERROR(); } } if (!watchdog) { @@ -143,17 +143,22 @@ std::string Aria2::doRequest(const MethodCall& methodCall) curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDS, requestContent.c_str()); curl_easy_setopt(mp_curl, CURLOPT_WRITEFUNCTION, &write_callback_to_iss); curl_easy_setopt(mp_curl, CURLOPT_WRITEDATA, &stringstream); + m_curlErrorBuffer[0] = 0; res = curl_easy_perform(mp_curl); if (res != CURLE_OK) { + LOG_ARIA_ERROR(); throw std::runtime_error("Cannot perform request"); } curl_easy_getinfo(mp_curl, CURLINFO_RESPONSE_CODE, &response_code); } + auto responseContent = stringstream.str(); if (response_code != 200) { + std::cerr << "ERROR: Invalid return code (" << response_code << ") from aria :" << std::endl; + std::cerr << responseContent << std::endl; throw std::runtime_error("Invalid return code from aria"); } - auto responseContent = stringstream.str(); + MethodResponse response(responseContent); if (response.isFault()) { throw AriaError(response.getFault().getFaultString()); diff --git a/src/downloader.cpp b/src/downloader.cpp index 8f103df5b..9bffad385 100644 --- a/src/downloader.cpp +++ b/src/downloader.cpp @@ -133,7 +133,7 @@ Downloader::Downloader() : m_knownDownloads[gid]->updateStatus(); } } catch (std::exception& e) { - std::cerr << "aria2 tellActive failed : " << e.what(); + std::cerr << "aria2 tellActive failed : " << e.what() << std::endl; } try { for (auto gid : mp_aria->tellWaiting()) { @@ -141,7 +141,7 @@ Downloader::Downloader() : m_knownDownloads[gid]->updateStatus(); } } catch (std::exception& e) { - std::cerr << "aria2 tellWaiting failed : " << e.what(); + std::cerr << "aria2 tellWaiting failed : " << e.what() << std::endl; } } From 470bfc3f1f076fe0a5952424af43f9db8ac0e360 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Wed, 26 Aug 2020 12:40:14 +0200 Subject: [PATCH 5/5] Better variable name for outStream. --- src/aria2.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/aria2.cpp b/src/aria2.cpp index 11fb48b2d..f059754ad 100644 --- a/src/aria2.cpp +++ b/src/aria2.cpp @@ -126,15 +126,15 @@ void Aria2::close() size_t write_callback_to_iss(char* ptr, size_t size, size_t nmemb, void* userdata) { - auto str = static_cast(userdata); - str->write(ptr, nmemb); + auto outStream = static_cast(userdata); + outStream->write(ptr, nmemb); return nmemb; } std::string Aria2::doRequest(const MethodCall& methodCall) { auto requestContent = methodCall.toString(); - std::stringstream stringstream; + std::stringstream outStream; CURLcode res; long response_code; { @@ -142,7 +142,7 @@ std::string Aria2::doRequest(const MethodCall& methodCall) curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDSIZE, requestContent.size()); curl_easy_setopt(mp_curl, CURLOPT_POSTFIELDS, requestContent.c_str()); curl_easy_setopt(mp_curl, CURLOPT_WRITEFUNCTION, &write_callback_to_iss); - curl_easy_setopt(mp_curl, CURLOPT_WRITEDATA, &stringstream); + curl_easy_setopt(mp_curl, CURLOPT_WRITEDATA, &outStream); m_curlErrorBuffer[0] = 0; res = curl_easy_perform(mp_curl); if (res != CURLE_OK) { @@ -152,7 +152,7 @@ std::string Aria2::doRequest(const MethodCall& methodCall) curl_easy_getinfo(mp_curl, CURLINFO_RESPONSE_CODE, &response_code); } - auto responseContent = stringstream.str(); + auto responseContent = outStream.str(); if (response_code != 200) { std::cerr << "ERROR: Invalid return code (" << response_code << ") from aria :" << std::endl; std::cerr << responseContent << std::endl;