tdf#123482 - cleanup convert-to folder more reliably.3.4.4-1

    Change-Id: I029bb4136984e05485e462c92da80b92b00fdebc

also squashes:

    Simpify DocumentBroker constructor.
    Change-Id: I0bf29df9316b129d34862c7464bb6636d42a850d

    Avoid using un-necessary reference.
    Change-Id: Id5a9fed8fb790f2af8facac119e9e0da476b1e47

Change-Id: I40eb5ae5b4721ffd709db6ecc7754dff8106475d
Reviewed-on: https://gerrit.libreoffice.org/68623
Reviewed-by: Andras Timar <andras.timar@collabora.com>
Tested-by: Andras Timar <andras.timar@collabora.com>

4 files changed, 52 insertions, 27 deletions

diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp
index 2eee1f797..cc325b44b 100644
--- a/wsd/ClientSession.cpp
+++ b/wsd/ClientSession.cpp
@@ -850,11 +850,6 @@ bool ClientSession::handleKitToClientMessage(const char* buffer, const int lengt
 
             // Now terminate.
             docBroker->stop("Finished saveas handler.");
-
-            // Remove file and directory
-            Poco::Path path = docBroker->getDocKey();
-            Poco::File(path).remove();
-            Poco::File(path.makeParent()).remove();
         }
 
         return true;
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 5b976caf3..465549c27 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -37,6 +37,7 @@
 #include "common/Message.hpp"
 #include "common/Protocol.hpp"
 #include "common/Unit.hpp"
+#include "common/FileUtil.hpp"
 
 #include <sys/types.h>
 #include <sys/wait.h>
@@ -145,13 +146,11 @@ std::atomic<unsigned> DocumentBroker::DocBrokerId(1);
 
 DocumentBroker::DocumentBroker(const std::string& uri,
                                const Poco::URI& uriPublic,
-                               const std::string& docKey,
-                               const std::string& childRoot) :
+                               const std::string& docKey) :
     _uriOrig(uri),
     _uriPublic(uriPublic),
     _docKey(docKey),
     _docId(Util::encodeId(DocBrokerId++, 3)),
-    _childRoot(childRoot),
     _cacheRoot(getCachePath(uriPublic.toString())),
     _documentChangedInStorage(false),
     _lastSaveTime(std::chrono::steady_clock::now()),
@@ -171,10 +170,10 @@ DocumentBroker::DocumentBroker(const std::string& uri,
     _debugRenderedTileCount(0)
 {
     assert(!_docKey.empty());
-    assert(!_childRoot.empty());
+    assert(!LOOLWSD::ChildRoot.empty());
 
     LOG_INF("DocumentBroker [" << LOOLWSD::anonymizeUrl(_uriPublic.toString()) <<
-            "] created with docKey [" << _docKey << "] and root [" << _childRoot << "]");
+            "] created with docKey [" << _docKey << "]");
 }
 
 void DocumentBroker::startThread()
@@ -1027,7 +1026,7 @@ bool DocumentBroker::sendUnoSave(const std::string& sessionId, bool dontTerminat
 std::string DocumentBroker::getJailRoot() const
 {
     assert(!_jailId.empty());
-    return Poco::Path(_childRoot, _jailId).toString();
+    return Poco::Path(LOOLWSD::ChildRoot, _jailId).toString();
 }
 
 size_t DocumentBroker::addSession(const std::shared_ptr<ClientSession>& session)
@@ -1826,6 +1825,18 @@ void DocumentBroker::getIOStats(uint64_t &sent, uint64_t &recv)
     }
 }
 
+ConvertToBroker::~ConvertToBroker()
+{
+    if (!_uriOrig.empty())
+    {
+        // Remove source file and directory
+        Poco::Path path = _uriOrig;
+        Poco::File(path).remove();
+        Poco::File(path.makeParent()).remove();
+        FileUtil::removeFile(_uriOrig);
+    }
+}
+
 void DocumentBroker::dumpState(std::ostream& os)
 {
     std::unique_lock<std::mutex> lock(_mutex);
diff --git a/wsd/DocumentBroker.hpp b/wsd/DocumentBroker.hpp
index d0fc1a747..8a284f965 100644
--- a/wsd/DocumentBroker.hpp
+++ b/wsd/DocumentBroker.hpp
@@ -213,10 +213,9 @@ public:
     /// Construct DocumentBroker with URI, docKey, and root path.
     DocumentBroker(const std::string& uri,
                    const Poco::URI& uriPublic,
-                   const std::string& docKey,
-                   const std::string& childRoot);
+                   const std::string& docKey);
 
-    ~DocumentBroker();
+    virtual ~DocumentBroker();
 
     /// Start processing events
     void startThread();
@@ -393,8 +392,9 @@ private:
     /// Sum the I/O stats from all connected sessions
     void getIOStats(uint64_t &sent, uint64_t &recv);
 
-private:
+protected:
     const std::string _uriOrig;
+private:
     const Poco::URI _uriPublic;
     /// URL-based key. May be repeated during the lifetime of WSD.
     const std::string _docKey;
@@ -463,6 +463,19 @@ private:
     static std::atomic<unsigned> DocBrokerId;
 };
 
+class ConvertToBroker : public DocumentBroker
+{
+public:
+    /// Construct DocumentBroker with URI and docKey
+    ConvertToBroker(const std::string& uri,
+                    const Poco::URI& uriPublic,
+                    const std::string& docKey)
+        : DocumentBroker(uri, uriPublic, docKey)
+    {
+    }
+    virtual ~ConvertToBroker();
+};
+
 #endif
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 75c4ae6e8..e692e3735 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -490,18 +490,24 @@ std::shared_ptr<ChildProcess> getNewChild_Blocks()
     return nullptr;
 }
 
-/// Handles the filename part of the convert-to POST request payload.
+/// Handles the filename part of the convert-to POST request payload,
+/// Also owns the file - cleaning it up when destroyed.
 class ConvertToPartHandler : public PartHandler
 {
-    std::string& _filename;
+    std::string _filename;
 
     /// Is it really a convert-to, ie. use an especially formed path?
     bool _convertTo;
 
 public:
-    ConvertToPartHandler(std::string& filename, bool convertTo = false)
-        : _filename(filename)
-        , _convertTo(convertTo)
+    std::string getFilename() const { return _filename; }
+
+    ConvertToPartHandler(bool convertTo = false)
+        : _convertTo(convertTo)
+    {
+    }
+
+    virtual ~ConvertToPartHandler()
     {
     }
 
@@ -519,6 +525,7 @@ public:
         if (!params.has("filename"))
             return;
 
+        // FIXME: needs wrapping - until then - keep in sync with ~ConvertToBroker
         Path tempPath = _convertTo? Path::forDirectory(Poco::TemporaryFile::tempName("/tmp/convert-to") + "/") :
                                     Path::forDirectory(Poco::TemporaryFile::tempName() + "/");
         File(tempPath).createDirectories();
@@ -1592,7 +1599,7 @@ static std::shared_ptr<DocumentBroker> findOrCreateDocBroker(WebSocketHandler& w
 
         // Set the one we just created.
         LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-        docBroker = std::make_shared<DocumentBroker>(uri, uriPublic, docKey, LOOLWSD::ChildRoot);
+        docBroker = std::make_shared<DocumentBroker>(uri, uriPublic, docKey);
         DocBrokers.emplace(docKey, docBroker);
         LOG_TRC("Have " << DocBrokers.size() << " DocBrokers after inserting [" << docKey << "].");
     }
@@ -2187,8 +2194,7 @@ private:
         StringTokenizer tokens(request.getURI(), "/?");
         if (tokens.count() > 2 && tokens[2] == "convert-to")
         {
-            std::string fromPath;
-            ConvertToPartHandler handler(fromPath, /*convertTo =*/ true);
+            ConvertToPartHandler handler(/*convertTo =*/ true);
             HTMLForm form(request, message, handler);
 
             std::string format = (form.has("format") ? form.get("format") : "");
@@ -2211,6 +2217,7 @@ private:
                 format = tokens[3];
 
             bool sent = false;
+            std::string fromPath = handler.getFilename();
             LOG_INF("Conversion request for URI [" << fromPath << "] format [" << format << "].");
             if (!fromPath.empty())
             {
@@ -2226,7 +2233,7 @@ private:
                     std::unique_lock<std::mutex> docBrokersLock(DocBrokersMutex);
 
                     LOG_DBG("New DocumentBroker for docKey [" << docKey << "].");
-                    auto docBroker = std::make_shared<DocumentBroker>(fromPath, uriPublic, docKey, LOOLWSD::ChildRoot);
+                    auto docBroker = std::make_shared<ConvertToBroker>(fromPath, uriPublic, docKey);
 
                     cleanupDocBrokers();
 
@@ -2300,8 +2307,7 @@ private:
         {
             LOG_INF("Insert file request.");
 
-            std::string tmpPath;
-            ConvertToPartHandler handler(tmpPath);
+            ConvertToPartHandler handler;
             HTMLForm form(request, message, handler);
 
             if (form.has("childid") && form.has("name"))
@@ -2331,7 +2337,7 @@ private:
                                               + JAILED_DOCUMENT_ROOT + "insertfile";
                     File(dirPath).createDirectories();
                     std::string fileName = dirPath + "/" + form.get("name");
-                    File(tmpPath).moveTo(fileName);
+                    File(handler.getFilename()).moveTo(fileName);
                     response.setContentLength(0);
                     socket->send(response);
                     return;