common/Util.cpp | 10 ++++++++-- kit/ForKit.cpp | 12 ++++++++---- kit/Kit.cpp | 16 +++++++--------- kit/Kit.hpp | 1 + wsd/DocumentBroker.cpp | 2 +- wsd/DocumentBroker.hpp | 8 +++++++- wsd/LOOLWSD.cpp | 15 +++++++++++++-- 7 files changed, 45 insertions(+), 19 deletions(-)
New commits: commit 9798eafb8c71ef9eafedb58478d260573eda2e71 Author: Ashod Nakashian <ashod.nakash...@collabora.co.uk> Date: Sun May 7 11:05:34 2017 -0400 wsd: random jail paths instead of pid Jail paths are now generate from a PRNG instead of using the PID of the kit process. The PRN is converted to base-64 and used as the directory name where a given kit is jailed. Change-Id: I8e4bc35d9ccdfdae0e542ab707c417cd29ad52f3 Reviewed-on: https://gerrit.libreoffice.org/37372 Reviewed-by: Ashod Nakashian <ashnak...@gmail.com> Tested-by: Ashod Nakashian <ashnak...@gmail.com> diff --git a/common/Util.cpp b/common/Util.cpp index 3260d130..612f971e 100644 --- a/common/Util.cpp +++ b/common/Util.cpp @@ -99,8 +99,14 @@ namespace Util std::string getFilename(const size_t length) { - std::string s = getB64String(length); - std::replace(s.begin(), s.end(), '/', '_'); + std::string s = getB64String(length * 2); + s.erase(std::remove_if(s.begin(), s.end(), + [](const std::string::value_type& c) + { + // Remove undesirable characters in a filename. + return c == '/' || c == ' ' || c == '+'; + }), + s.end()); return s.substr(0, length); } } diff --git a/kit/ForKit.cpp b/kit/ForKit.cpp index 64d17f45..ab11f626 100644 --- a/kit/ForKit.cpp +++ b/kit/ForKit.cpp @@ -197,6 +197,7 @@ static void cleanupChildren() std::vector<std::string> jails; Process::PID exitedChildPid; int status; + // Reap quickly without doing slow cleanup so WSD can spawn more rapidly. while ((exitedChildPid = waitpid(-1, &status, WUNTRACED | WNOHANG)) > 0) { const auto it = childJails.find(exitedChildPid); @@ -225,7 +226,10 @@ static int createLibreOfficeKit(const std::string& childRoot, const std::string& loSubPath, bool queryVersion = false) { - LOG_DBG("Forking a loolkit process."); + // Generate a jail ID to be used for in the jail path. + const std::string jailId = Util::rng::getFilename(16); + + LOG_DBG("Forking a loolkit process with jailId: " << jailId << "."); const Process::PID pid = fork(); if (!pid) @@ -252,9 +256,9 @@ static int createLibreOfficeKit(const std::string& childRoot, } #ifndef KIT_IN_PROCESS - lokit_main(childRoot, sysTemplate, loTemplate, loSubPath, NoCapsForKit, queryVersion, DisplayVersion); + lokit_main(childRoot, jailId, sysTemplate, loTemplate, loSubPath, NoCapsForKit, queryVersion, DisplayVersion); #else - lokit_main(childRoot, sysTemplate, loTemplate, loSubPath, true, queryVersion, DisplayVersion); + lokit_main(childRoot, jailId, sysTemplate, loTemplate, loSubPath, true, queryVersion, DisplayVersion); #endif } else @@ -267,7 +271,7 @@ static int createLibreOfficeKit(const std::string& childRoot, else { LOG_INF("Forked kit [" << pid << "]."); - childJails[pid] = childRoot + std::to_string(pid); + childJails[pid] = childRoot + jailId; } #ifndef KIT_IN_PROCESS diff --git a/kit/Kit.cpp b/kit/Kit.cpp index 144c3b2c..628e9295 100644 --- a/kit/Kit.cpp +++ b/kit/Kit.cpp @@ -1577,6 +1577,7 @@ void documentViewCallback(const int type, const char* payload, void* data) #ifndef BUILDING_TESTS void lokit_main(const std::string& childRoot, + const std::string& jailId, const std::string& sysTemplate, const std::string& loTemplate, const std::string& loSubPath, @@ -1610,12 +1611,6 @@ void lokit_main(const std::string& childRoot, assert(!loTemplate.empty()); assert(!loSubPath.empty()); - // Ideally this will be a random ID, but forkit will cleanup - // our jail directory when we die, and it's simpler to know - // the jailId (i.e. the path) implicitly by knowing our pid. - static const std::string pid = std::to_string(Process::id()); - static const std::string jailId = pid; - LOG_DBG("Process started."); std::string userdir_url; @@ -1772,7 +1767,10 @@ void lokit_main(const std::string& childRoot, assert(loKit); LOG_INF("Process is ready."); - std::string requestUrl = std::string(NEW_CHILD_URI) + "pid=" + pid; + static const std::string pid = std::to_string(Process::id()); + + std::string requestUrl = NEW_CHILD_URI; + requestUrl += "pid=" + pid + "&jailid=" + jailId; if (queryVersion) { char* versionInfo = loKit->getVersionInfo(); @@ -1796,9 +1794,9 @@ void lokit_main(const std::string& childRoot, auto queue = std::make_shared<TileQueue>(); - const std::string socketName = "child_ws_" + std::to_string(getpid()); + const std::string socketName = "child_ws_" + pid; IoUtil::SocketProcessor(ws, socketName, - [&socketName, &ws, &loKit, &queue](const std::vector<char>& data) + [&socketName, &ws, &loKit, &jailId, &queue](const std::vector<char>& data) { std::string message(data.data(), data.size()); diff --git a/kit/Kit.hpp b/kit/Kit.hpp index 6895bbdb..f003e26a 100644 --- a/kit/Kit.hpp +++ b/kit/Kit.hpp @@ -10,6 +10,7 @@ #define INCLUDED_LOOLKIT_HPP void lokit_main(const std::string& childRoot, + const std::string& jailId, const std::string& sysTemplate, const std::string& loTemplate, const std::string& loSubPath, diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index 999cab12..ad958964 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -804,7 +804,7 @@ size_t DocumentBroker::addSession(const std::shared_ptr<ClientSession>& session) try { // First load the document, since this can fail. - if (!load(session, std::to_string(_childProcess->getPid()))) + if (!load(session, _childProcess->getJailId())) { const auto msg = "Failed to load document with URI [" + session->getPublicUri().toString() + "]."; LOG_ERR(msg); diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp index 5b64d7ed..3f7ebc67 100644 --- a/wsd/DocumentBroker.hpp +++ b/wsd/DocumentBroker.hpp @@ -59,9 +59,13 @@ class ChildProcess public: /// @param pid is the process ID of the child. /// @param socket is the underlying Sockeet to the child. - ChildProcess(const Poco::Process::PID pid, const std::shared_ptr<StreamSocket>& socket, const Poco::Net::HTTPRequest &request) : + ChildProcess(const Poco::Process::PID pid, + const std::string& jailId, + const std::shared_ptr<StreamSocket>& socket, + const Poco::Net::HTTPRequest &request) : _pid(pid), + _jailId(jailId), _ws(std::make_shared<WebSocketHandler>(socket, request)), _socket(socket) { @@ -143,6 +147,7 @@ public: } Poco::Process::PID getPid() const { return _pid; } + const std::string& getJailId() const { return _jailId; } /// Send a text payload to the child-process WS. bool sendTextFrame(const std::string& data) @@ -185,6 +190,7 @@ public: private: Poco::Process::PID _pid; + const std::string _jailId; std::shared_ptr<WebSocketHandler> _ws; std::shared_ptr<Socket> _socket; std::weak_ptr<DocumentBroker> _docBroker; diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp index f06f67ef..9c497e13 100644 --- a/wsd/LOOLWSD.cpp +++ b/wsd/LOOLWSD.cpp @@ -1441,12 +1441,17 @@ private: // New Child is spawned. const auto params = Poco::URI(request.getURI()).getQueryParameters(); Poco::Process::PID pid = -1; + std::string jailId; for (const auto& param : params) { if (param.first == "pid") { pid = std::stoi(param.second); } + else if (param.first == "jailid") + { + jailId = param.second; + } else if (param.first == "version") { LOOLWSD::LOKitVersion = param.second; @@ -1459,13 +1464,19 @@ private: return; } + if (jailId.empty()) + { + LOG_ERR("Invalid JailId in child URI [" << request.getURI() << "]."); + return SocketHandlerInterface::SocketOwnership::UNCHANGED; + } + in.clear(); - LOG_INF("New child [" << pid << "]."); + LOG_INF("New child [" << pid << "], jailId: " << jailId << "."); UnitWSD::get().newChild(*this); - auto child = std::make_shared<ChildProcess>(pid, socket, request); + auto child = std::make_shared<ChildProcess>(pid, jailId, socket, request); _childProcess = child; // weak _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits