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<int const, UserInfo>::pair(std::pair<int const, UserInfo> 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
This commit is contained in:
Miklos Vajna 2019-08-16 09:05:54 +02:00
parent 13884468c3
commit 85dbb4a9af
3 changed files with 28 additions and 15 deletions

View file

@ -70,7 +70,7 @@ ChildSession::ChildSession(const std::string& id,
DocumentManagerInterface& docManager) : DocumentManagerInterface& docManager) :
Session("ToMaster-" + id, id, false), Session("ToMaster-" + id, id, false),
_jailId(jailId), _jailId(jailId),
_docManager(docManager), _docManager(&docManager),
_viewId(-1), _viewId(-1),
_isDocLoaded(false), _isDocLoaded(false),
_copyToClipboard(false) _copyToClipboard(false)
@ -93,7 +93,10 @@ void ChildSession::disconnect()
if (_viewId >= 0) if (_viewId >= 0)
{ {
_docManager.onUnload(*this); if (_docManager)
{
_docManager->onUnload(*this);
}
} }
else else
{ {
@ -132,7 +135,7 @@ bool ChildSession::_handleInput(const char *buffer, int length)
curPart = getLOKitDocument()->getPart(); curPart = getLOKitDocument()->getPart();
// Notify all views about updated view info // Notify all views about updated view info
_docManager.notifyViewInfo(); _docManager->notifyViewInfo();
if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT) if (getLOKitDocument()->getDocumentType() != LOK_DOCTYPE_TEXT)
{ {
@ -565,7 +568,7 @@ bool ChildSession::loadDocument(const char * /*buffer*/, int /*length*/, const s
std::unique_lock<std::recursive_mutex> lock(Mutex); std::unique_lock<std::recursive_mutex> lock(Mutex);
const bool loaded = _docManager.onLoad(getId(), getJailedFilePath(), getJailedFilePathAnonym(), const bool loaded = _docManager->onLoad(getId(), getJailedFilePath(), getJailedFilePathAnonym(),
getUserName(), getUserNameAnonym(), getUserName(), getUserNameAnonym(),
getDocPassword(), renderOpts, getHaveDocPassword(), getDocPassword(), renderOpts, getHaveDocPassword(),
getLang(), getWatermarkText(), doctemplate); 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 // Inform everyone (including this one) about updated view info
_docManager.notifyViewInfo(); _docManager->notifyViewInfo();
sendTextFrame("editor: " + std::to_string(_docManager.getEditorId())); sendTextFrame("editor: " + std::to_string(_docManager->getEditorId()));
LOG_INF("Loaded session " << getId()); 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(values == nullptr ? "" : values),
std::string(undo == nullptr ? "" : undo)); std::string(undo == nullptr ? "" : undo));
// json only contains view IDs, insert matching user names. // json only contains view IDs, insert matching user names.
std::map<int, UserInfo> viewInfo = _docManager.getViewInfo(); std::map<int, UserInfo> viewInfo = _docManager->getViewInfo();
insertUserNames(viewInfo, json); insertUserNames(viewInfo, json);
success = sendTextFrame("commandvalues: " + json); success = sendTextFrame("commandvalues: " + json);
std::free(values); std::free(values);
@ -848,7 +851,7 @@ bool ChildSession::downloadAs(const char* /*buffer*/, int /*length*/, const std:
} }
// Obfuscate the new name. // Obfuscate the new name.
Util::mapAnonymized(Util::getFilenameFromURL(name), _docManager.getObfuscatedFileId()); Util::mapAnonymized(Util::getFilenameFromURL(name), _docManager->getObfuscatedFileId());
getTokenString(tokens[3], "format", format); getTokenString(tokens[3], "format", format);
@ -1654,7 +1657,7 @@ bool ChildSession::exportSignAndUploadDocument(const char* buffer, int length, c
std::vector<unsigned char> binaryCertificate = decodeBase64(extractCertificate(x509Certificate)); std::vector<unsigned char> binaryCertificate = decodeBase64(extractCertificate(x509Certificate));
std::vector<unsigned char> binaryPrivateKey = decodeBase64(extractPrivateKey(privateKey)); std::vector<unsigned char> binaryPrivateKey = decodeBase64(extractPrivateKey(privateKey));
bResult = _docManager.getLOKit()->signDocument(aTempDocumentURL.c_str(), bResult = _docManager->getLOKit()->signDocument(aTempDocumentURL.c_str(),
binaryCertificate.data(), binaryCertificate.size(), binaryCertificate.data(), binaryCertificate.size(),
binaryPrivateKey.data(), binaryPrivateKey.size()); binaryPrivateKey.data(), binaryPrivateKey.size());
@ -2046,7 +2049,7 @@ void ChildSession::updateSpeed() {
_cursorInvalidatedEvent.pop(); _cursorInvalidatedEvent.pop();
} }
_cursorInvalidatedEvent.push(now); _cursorInvalidatedEvent.push(now);
_docManager.updateEditorSpeeds(_viewId, _cursorInvalidatedEvent.size()); _docManager->updateEditorSpeeds(_viewId, _cursorInvalidatedEvent.size());
} }
int ChildSession::getSpeed() { int ChildSession::getSpeed() {

View file

@ -221,20 +221,25 @@ public:
{ {
const auto msg = "client-" + getId() + ' ' + std::string(buffer, length); const auto msg = "client-" + getId() + ' ' + std::string(buffer, length);
const std::unique_lock<std::mutex> lock = getLock(); const std::unique_lock<std::mutex> 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 bool sendBinaryFrame(const char* buffer, int length) override
{ {
const auto msg = "client-" + getId() + ' ' + std::string(buffer, length); const auto msg = "client-" + getId() + ' ' + std::string(buffer, length);
const std::unique_lock<std::mutex> lock = getLock(); const std::unique_lock<std::mutex> lock = getLock();
return _docManager.sendFrame(msg.data(), msg.size(), WSOpCode::Binary); return _docManager->sendFrame(msg.data(), msg.size(), WSOpCode::Binary);
} }
using Session::sendTextFrame; using Session::sendTextFrame;
bool getClipboard(const char* buffer, int length, const std::vector<std::string>& tokens); bool getClipboard(const char* buffer, int length, const std::vector<std::string>& tokens);
void resetDocManager()
{
_docManager = nullptr;
}
private: private:
bool loadDocument(const char* buffer, int length, const std::vector<std::string>& tokens); bool loadDocument(const char* buffer, int length, const std::vector<std::string>& tokens);
@ -278,12 +283,12 @@ private:
std::shared_ptr<lok::Document> getLOKitDocument() std::shared_ptr<lok::Document> getLOKitDocument()
{ {
return _docManager.getLOKitDocument(); return _docManager->getLOKitDocument();
} }
std::string getLOKitLastError() std::string getLOKitLastError()
{ {
char *lastErr = _docManager.getLOKit()->getError(); char *lastErr = _docManager->getLOKit()->getError();
std::string ret; std::string ret;
if (lastErr) if (lastErr)
{ {
@ -295,7 +300,7 @@ private:
private: private:
const std::string _jailId; const std::string _jailId;
DocumentManagerInterface& _docManager; DocumentManagerInterface* _docManager;
std::queue<std::chrono::steady_clock::time_point> _cursorInvalidatedEvent; std::queue<std::chrono::steady_clock::time_point> _cursorInvalidatedEvent;
const unsigned _eventStorageIntervalMs = 15*1000; const unsigned _eventStorageIntervalMs = 15*1000;

View file

@ -910,6 +910,11 @@ public:
_stop = true; _stop = true;
_tileQueue->put("eof"); _tileQueue->put("eof");
for (const auto& session : _sessions)
{
session.second->resetDocManager();
}
} }
const std::string& getUrl() const { return _url; } const std::string& getUrl() const { return _url; }