From 85dbb4a9afcc144680718f77af00200adb5d60e5 Mon Sep 17 00:00:00 2001 From: Miklos Vajna Date: Fri, 16 Aug 2019 09:05:54 +0200 Subject: [PATCH] kit: fix UB in ChildSession::disconnect() Finally unit-copy-paste passes under sanitizers with this. Details: ==8988==ERROR: AddressSanitizer: heap-use-after-free on address 0x60d0005e6de0 at pc 0x000000988e85 bp 0x7fff753316d0 sp 0x7fff753316c8 READ of size 4 at 0x60d0005e6de0 thread T0 (loolkit) #0 0x988e84 in std::pair::pair(std::pair const&) /home/vmiklos/git/libreoffice/lode/opt_private/gcc-7.3.0/lib64/gcc/x86_64-pc-linux-gnu/7.3.0/../../../../include/c++/7.3.0/bits/stl_pair.h:292:17 ... #12 0x9322af in Document::notifyViewInfo() /home/vmiklos/git/libreoffice/online-san/kit/Kit.cpp:1600:53 #13 0x9303f9 in Document::onUnload(ChildSession const&) /home/vmiklos/git/libreoffice/online-san/kit/Kit.cpp:1566:13 #14 0x616dcd in ChildSession::disconnect() /home/vmiklos/git/libreoffice/online-san/kit/ChildSession.cpp:96:25 #15 0x616535 in ChildSession::~ChildSession() /home/vmiklos/git/libreoffice/online-san/kit/ChildSession.cpp:85:5 freed by thread T0 (loolkit) here: #0 0x60f9b0 in operator delete(void*) _asan_rtl_:0 ... #8 0x939292 in Document::~Document() /home/vmiklos/git/libreoffice/online-san/kit/Kit.cpp:913:5 I.e. when the Document dtor clears Document::_sessions, the ChildSession dtor may be called. But ChildSession expected that it has a valid Document during its lifetime, which is not a promise we can hold, see the above trace. Fix the problem by having a pointer (and not a reference) to a Document in ChildSession and then: 1) Clear that Document pointer in ChildSessions at the end of the Document dtor using a new resetDocManager() 2) Check if the Document is nullptr in ChildSession::disconnect() instead of dereferencing it unconditionally. Change-Id: I19d3d6bfe9e142a52c199f49aaa347d1a2edbf87 --- kit/ChildSession.cpp | 23 +++++++++++++---------- kit/ChildSession.hpp | 15 ++++++++++----- kit/Kit.cpp | 5 +++++ 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/kit/ChildSession.cpp b/kit/ChildSession.cpp index 428808835..62566c7be 100644 --- a/kit/ChildSession.cpp +++ b/kit/ChildSession.cpp @@ -70,7 +70,7 @@ ChildSession::ChildSession(const std::string& id, DocumentManagerInterface& docManager) : Session("ToMaster-" + id, id, false), _jailId(jailId), - _docManager(docManager), + _docManager(&docManager), _viewId(-1), _isDocLoaded(false), _copyToClipboard(false) @@ -93,7 +93,10 @@ void ChildSession::disconnect() if (_viewId >= 0) { - _docManager.onUnload(*this); + if (_docManager) + { + _docManager->onUnload(*this); + } } else { @@ -132,7 +135,7 @@ bool ChildSession::_handleInput(const char *buffer, int length) curPart = getLOKitDocument()->getPart(); // Notify all views about updated view info - _docManager.notifyViewInfo(); + _docManager->notifyViewInfo(); if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT) { @@ -565,7 +568,7 @@ bool ChildSession::loadDocument(const char * /*buffer*/, int /*length*/, const s std::unique_lock lock(Mutex); - const bool loaded = _docManager.onLoad(getId(), getJailedFilePath(), getJailedFilePathAnonym(), + const bool loaded = _docManager->onLoad(getId(), getJailedFilePath(), getJailedFilePathAnonym(), getUserName(), getUserNameAnonym(), getDocPassword(), renderOpts, getHaveDocPassword(), getLang(), getWatermarkText(), doctemplate); @@ -607,8 +610,8 @@ bool ChildSession::loadDocument(const char * /*buffer*/, int /*length*/, const s } // Inform everyone (including this one) about updated view info - _docManager.notifyViewInfo(); - sendTextFrame("editor: " + std::to_string(_docManager.getEditorId())); + _docManager->notifyViewInfo(); + sendTextFrame("editor: " + std::to_string(_docManager->getEditorId())); LOG_INF("Loaded session " << getId()); @@ -750,7 +753,7 @@ bool ChildSession::getCommandValues(const char* /*buffer*/, int /*length*/, cons std::string(values == nullptr ? "" : values), std::string(undo == nullptr ? "" : undo)); // json only contains view IDs, insert matching user names. - std::map viewInfo = _docManager.getViewInfo(); + std::map viewInfo = _docManager->getViewInfo(); insertUserNames(viewInfo, json); success = sendTextFrame("commandvalues: " + json); std::free(values); @@ -848,7 +851,7 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const std: } // Obfuscate the new name. - Util::mapAnonymized(Util::getFilenameFromURL(name), _docManager.getObfuscatedFileId()); + Util::mapAnonymized(Util::getFilenameFromURL(name), _docManager->getObfuscatedFileId()); getTokenString(tokens[3], "format", format); @@ -1654,7 +1657,7 @@ bool ChildSession::exportSignAndUploadDocument(const char* buffer, int length, c std::vector binaryCertificate = decodeBase64(extractCertificate(x509Certificate)); std::vector binaryPrivateKey = decodeBase64(extractPrivateKey(privateKey)); - bResult = _docManager.getLOKit()->signDocument(aTempDocumentURL.c_str(), + bResult = _docManager->getLOKit()->signDocument(aTempDocumentURL.c_str(), binaryCertificate.data(), binaryCertificate.size(), binaryPrivateKey.data(), binaryPrivateKey.size()); @@ -2046,7 +2049,7 @@ void ChildSession::updateSpeed() { _cursorInvalidatedEvent.pop(); } _cursorInvalidatedEvent.push(now); - _docManager.updateEditorSpeeds(_viewId, _cursorInvalidatedEvent.size()); + _docManager->updateEditorSpeeds(_viewId, _cursorInvalidatedEvent.size()); } int ChildSession::getSpeed() { diff --git a/kit/ChildSession.hpp b/kit/ChildSession.hpp index a1a4532bb..3a2f6b3cf 100644 --- a/kit/ChildSession.hpp +++ b/kit/ChildSession.hpp @@ -221,20 +221,25 @@ public: { const auto msg = "client-" + getId() + ' ' + std::string(buffer, length); const std::unique_lock lock = getLock(); - return _docManager.sendFrame(msg.data(), msg.size(), WSOpCode::Text); + return _docManager->sendFrame(msg.data(), msg.size(), WSOpCode::Text); } bool sendBinaryFrame(const char* buffer, int length) override { const auto msg = "client-" + getId() + ' ' + std::string(buffer, length); const std::unique_lock lock = getLock(); - return _docManager.sendFrame(msg.data(), msg.size(), WSOpCode::Binary); + return _docManager->sendFrame(msg.data(), msg.size(), WSOpCode::Binary); } using Session::sendTextFrame; bool getClipboard(const char* buffer, int length, const std::vector& tokens); + void resetDocManager() + { + _docManager = nullptr; + } + private: bool loadDocument(const char* buffer, int length, const std::vector& tokens); @@ -278,12 +283,12 @@ private: std::shared_ptr getLOKitDocument() { - return _docManager.getLOKitDocument(); + return _docManager->getLOKitDocument(); } std::string getLOKitLastError() { - char *lastErr = _docManager.getLOKit()->getError(); + char *lastErr = _docManager->getLOKit()->getError(); std::string ret; if (lastErr) { @@ -295,7 +300,7 @@ private: private: const std::string _jailId; - DocumentManagerInterface& _docManager; + DocumentManagerInterface* _docManager; std::queue _cursorInvalidatedEvent; const unsigned _eventStorageIntervalMs = 15*1000; diff --git a/kit/Kit.cpp b/kit/Kit.cpp index 42962fe41..bd6dc7f8c 100644 --- a/kit/Kit.cpp +++ b/kit/Kit.cpp @@ -910,6 +910,11 @@ public: _stop = true; _tileQueue->put("eof"); + + for (const auto& session : _sessions) + { + session.second->resetDocManager(); + } } const std::string& getUrl() const { return _url; }