[Libreoffice-commits] online.git: Branch 'distro/collabora/collabora-online-2-1' - wsd/DocumentBroker.cpp

[Ashod Nakashian]

 wsd/DocumentBroker.cpp |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

New commits:
commit 9765d242717446aba5a036c5aba86dda666b825b
Author: Ashod Nakashian <ashod.nakashian at collabora.co.uk>
Date:   Mon Jun 19 21:53:58 2017 -0400

    wsd: fix access-after-free error
    
    Valgrind spotted one case, and the other is possible but
    not common it seems.
    
    Change-Id: Id5e41145f597c3564263adb25b7b765db1c90bf7
    Reviewed-on: https://gerrit.libreoffice.org/38991
    Reviewed-by: Ashod Nakashian <ashnakash at gmail.com>
    Tested-by: Ashod Nakashian <ashnakash at gmail.com>
    (cherry picked from commit 7ae37aff0d2d6a337551b8b5a3d9daadc93d128c)
    Reviewed-on: https://gerrit.libreoffice.org/38994
    Reviewed-by: Michael Meeks <michael.meeks at collabora.com>
    Tested-by: Michael Meeks <michael.meeks at collabora.com>

diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index deae3326..ea107df4 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -900,9 +900,8 @@ size_t DocumentBroker::removeSessionInternal(const std::string& id)
 
             const auto readonly = (it->second ? it->second->isReadOnly() : false);
 
-            //FIXME: We might be called from the session we are removing,
-            //FIXME: and if this is the last/only reference, we destroy it.
-            //FIXME: Should flag and remove from the poll thread.
+            // Remove. The caller must have a reference to the session
+            // in question, lest we destroy from underneith them.
             _sessions.erase(it);
 
             const auto count = _sessions.size();
@@ -1298,7 +1297,9 @@ bool DocumentBroker::forwardToClient(const std::shared_ptr<Message>& payload)
         if (sid == "all")
         {
             // Broadcast to all.
-            for (const auto& pair : _sessions)
+            // Events could cause the removal of sessions.
+            std::map<std::string, std::shared_ptr<ClientSession>> sessions(_sessions);
+            for (const auto& pair : sessions)
             {
                 pair.second->handleKitToClientMessage(data, size);
             }
@@ -1308,7 +1309,10 @@ bool DocumentBroker::forwardToClient(const std::shared_ptr<Message>& payload)
             const auto it = _sessions.find(sid);
             if (it != _sessions.end())
             {
-                return it->second->handleKitToClientMessage(data, size);
+                // Take a ref as the session could be removed from _sessions
+                // if it's the save confirmation keeping a stopped session alive.
+                std::shared_ptr<ClientSession> session = it->second;
+                return session->handleKitToClientMessage(data, size);
             }
             else
             {