[Libreoffice-commits] online.git: Branch 'feature/paralleltest' - 6 commits - common/FileUtil.cpp common/FileUtil.hpp common/SigUtil.cpp common/Unit.cpp common/Unit.hpp kit/ForKit.cpp test/helpers.hpp test/integration-http-server.cpp test/Makefile.am test/run_unit.sh.in test/test.cpp test/test.hpp test/UnitClient.cpp wsd/Auth.cpp wsd/Auth.hpp wsd/DocumentBroker.cpp wsd/LOOLWSD.cpp wsd/LOOLWSD.hpp wsd/Storage.cpp wsd/TraceFile.hpp

Michael Meeks (via logerrit) logerrit at kemper.freedesktop.org
Sat Jan 18 16:40:54 UTC 2020


Rebased ref, commits from common ancestor:
commit e9c3e0752b8c00cab931dcf109ed81418f801242
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Jan 17 21:18:42 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 16:40:05 2020 +0000

    test: dung out redundant LOOL_TEST_CLIENT_PORT.
    
    And cleanup other related oddities.
    
    Change-Id: I2d179a2ece6a8553e10e406ad4e5da08a2ff58e5

diff --git a/kit/ForKit.cpp b/kit/ForKit.cpp
index 164489589..ac0191e33 100644
--- a/kit/ForKit.cpp
+++ b/kit/ForKit.cpp
@@ -396,15 +396,6 @@ int main(int argc, char** argv)
     std::string sysTemplate;
     std::string loTemplate;
 
-#if ENABLE_DEBUG
-    static const char* clientPort = std::getenv("LOOL_TEST_CLIENT_PORT");
-    if (clientPort)
-        ClientPortNumber = std::stoi(clientPort);
-    static const char* masterPort = std::getenv("LOOL_TEST_MASTER_PORT");
-    if (masterPort)
-        MasterLocation = masterPort;
-#endif
-
     for (int i = 0; i < argc; ++i)
     {
         char *cmd = argv[i];
diff --git a/test/Makefile.am b/test/Makefile.am
index 2b369465a..e10cb7d8c 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -2,12 +2,10 @@
 export MAX_CONCURRENCY=4
 AUTOMAKE_OPTION = serial-tests
 
-# unittest: tests that do not need loolwsd running, and that are run during a
-# normal build
-# test: tests that need loolwsd running, and that are run via 'make check'
-check_PROGRAMS = test fakesockettest
+# unittest: tests that run a captive loolwsd as part of themselves.
+check_PROGRAMS = fakesockettest
 
-noinst_PROGRAMS = test fakesockettest unittest
+noinst_PROGRAMS = fakesockettest unittest
 
 AM_CXXFLAGS = $(CPPUNIT_CFLAGS) -DTDOC=\"$(abs_top_srcdir)/test/data\" \
 	-I${top_srcdir}/common -I${top_srcdir}/net -I${top_srcdir}/wsd -I${top_srcdir}/kit
@@ -87,10 +85,6 @@ unittest_CPPFLAGS = -I$(top_srcdir) -DBUILDING_TESTS
 unittest_SOURCES = $(test_base_source) test.cpp
 unittest_LDADD = $(CPPUNIT_LIBS)
 
-test_CPPFLAGS = -I$(top_srcdir) -DBUILDING_TESTS
-test_SOURCES = $(test_all_source) test.cpp
-test_LDADD = $(CPPUNIT_LIBS)
-
 fakesockettest_SOURCES = fakesockettest.cpp ../net/FakeSocket.cpp
 fakesockettest_LDADD = $(CPPUNIT_LIBS)
 
diff --git a/test/helpers.hpp b/test/helpers.hpp
index dd638d191..19bd8804d 100644
--- a/test/helpers.hpp
+++ b/test/helpers.hpp
@@ -187,12 +187,15 @@ Poco::Net::HTTPClientSession* createSession(const Poco::URI& uri)
     return new Poco::Net::HTTPClientSession(uri.getHost(), uri.getPort());
 }
 
+#ifndef UNIT_CLIENT_TESTS
+
 inline int getClientPort()
 {
-    static const char* clientPort = std::getenv("LOOL_TEST_CLIENT_PORT");
-    return clientPort? atoi(clientPort) : DEFAULT_CLIENT_PORT_NUMBER;
+    return DEFAULT_CLIENT_PORT_NUMBER;
 }
 
+#endif
+
 inline std::shared_ptr<Poco::Net::StreamSocket> createRawSocket()
 {
     return
diff --git a/test/integration-http-server.cpp b/test/integration-http-server.cpp
index 4db04737a..5d9342b76 100644
--- a/test/integration-http-server.cpp
+++ b/test/integration-http-server.cpp
@@ -92,15 +92,13 @@ public:
     // A server URI which was not added to loolwsd.xml as post_allow IP or a wopi storage host
     Poco::URI getNotAllowedTestServerURI()
     {
-        static const char* clientPort = std::getenv("LOOL_TEST_CLIENT_PORT");
-
         static std::string serverURI(
 #if ENABLE_SSL
-        "https://165.227.162.232:"
+            "https://165.227.162.232:9980"
 #else
-        "http://165.227.162.232:"
+            "http://165.227.162.232:9980"
 #endif
-            + (clientPort? std::string(clientPort) : std::to_string(DEFAULT_CLIENT_PORT_NUMBER)));
+            );
 
         return Poco::URI(serverURI);
     }
diff --git a/test/run_unit.sh.in b/test/run_unit.sh.in
index 5fb8a6936..4b893f23c 100755
--- a/test/run_unit.sh.in
+++ b/test/run_unit.sh.in
@@ -70,55 +70,10 @@ fi
 echo "Test output is '$test_output'"
 echo > $test_output
 
-if test "z$tst" == "z"; then
-     # run the test on a dedicated port
-     export LOOL_TEST_CLIENT_PORT=9984
-     export LOOL_TEST_MASTER_PORT=9985
-
-     echo "Executing external tests"
-     ${trace} \
-     ${abs_top_builddir}/loolwsd --o:sys_template_path="$systemplate_path" \
-                                 --o:child_root_path="$jails_path" \
-                                 --o:storage.filesystem[@allow]=true \
-                                 --o:logging.level=trace \
-                                 --o:logging.file[@enable]=false \
-                                 --o:ssl.key_file_path="${abs_top_builddir}/etc/key.pem" \
-                                 --o:ssl.cert_file_path="${abs_top_builddir}/etc/cert.pem" \
-                                 --o:ssl.ca_file_path="${abs_top_builddir}/etc/ca-chain.cert.pem" \
-                                 --o:admin_console.username=admin --o:admin_console.password=admin \
-                                 --o:storage.ssl.enable=false \
-                                 > "$tst_log" 2>&1 &
-     if test "z${SLEEPFORDEBUGGER}${SLEEPKITFORDEBUGGER}" != "z"; then
-	  echo "sleeping for debugger"
-          sleep ${SLEEPFORDEBUGGER:-0}
-          sleep ${SLEEPKITFORDEBUGGER:-0}
-     fi
-
-     echo "  executing test"
-
-     oldpath=`pwd`
-     cd "${abs_top_builddir}/test"
-     if eval ${trace} ./test ${verbose}; then
-	 echo "Test run_test.sh passed."
-	 echo ":test-result: PASS run_test.sh" >> $oldpath/$test_output
-	 retval=0
-     else
-	 echo ":test-result: FAIL run_test.sh" >> $oldpath/$test_output
-	 retval=1
-     fi
-
-     if test "z${SLEEPFORDEBUGGER}${SLEEPKITFORDEBUGGER}" == "z"; then
-         echo "killing $!"
-         kill $!
-     fi
-
-     exit $retval
-
-else # newer unit tests.
-    echo "Running $tst | $tst_log ...";
-    if ${trace} \
+echo "Running $tst | $tst_log ...";
+if ${trace} \
        ${abs_top_builddir}/loolwsd --o:sys_template_path="$systemplate_path" \
-                                   --o:child_root_path="$jails_path" \
+       --o:child_root_path="$jails_path" \
                                    --o:storage.filesystem[@allow]=true \
                                    --o:logging.level=trace \
                                    --o:ssl.key_file_path="${abs_top_builddir}/etc/key.pem" \
@@ -129,7 +84,7 @@ else # newer unit tests.
                                    --unitlib="${abs_top_builddir}/test/.libs/$tst.so" 2> "$tst_log"; then
         echo "Test $tst passed."
         echo ":test-result: PASS $tst" >> $test_output
-    else
+else
 	cat $tst_log
         echo "============================================================="
         echo "Test failed on unit: $tst re-run with:"
@@ -147,7 +102,6 @@ else # newer unit tests.
 	echo "   $ less $tst_log # for detailed failure log files"
         echo "============================================================="
         echo ":test-result: FAIL $tst" >> $test_output
-    fi
 fi
 
 # vim:set shiftwidth=4 expandtab:
diff --git a/test/test.cpp b/test/test.cpp
index 4a5b0f6d0..f80dab522 100644
--- a/test/test.cpp
+++ b/test/test.cpp
@@ -314,6 +314,11 @@ int getLoolKitProcessCount()
     return getKitPids().size();
 }
 
+int getClientPort()
+{
+    return LOOLWSD::getClientPortNumber();
+}
+
 #endif // UNIT_CLIENT_TESTS
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/test/test.hpp b/test/test.hpp
index 29b6e76d3..a5a2b2280 100644
--- a/test/test.hpp
+++ b/test/test.hpp
@@ -31,6 +31,9 @@ std::vector<int> getDocKitPids();
 /// Get the PID of the forkit
 std::vector<int> getForKitPids();
 
+/// Which port should we connect to get to WSD.
+int getClientPort();
+
 /// How many live loolkit processes do we have ?
 int getLoolKitProcessCount();
 
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 2122d9e8e..1dbc92d7e 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -1404,10 +1404,6 @@ void LOOLWSD::handleOption(const std::string& optionName,
     else if (optionName == "careerspan")
         careerSpanMs = std::stoi(value) * 1000; // Convert second to ms
 
-    static const char* clientPort = std::getenv("LOOL_TEST_CLIENT_PORT");
-    if (clientPort)
-        ClientPortNumber = std::stoi(clientPort);
-
     static const char* latencyMs = std::getenv("LOOL_DELAY_SOCKET_MS");
     if (latencyMs)
         SimulatedLatencyMs = std::stoi(latencyMs);
@@ -3670,6 +3666,11 @@ std::vector<std::shared_ptr<DocumentBroker>> LOOLWSD::getBrokersTestOnly()
     return result;
 }
 
+int LOOLWSD::getClientPortNumber()
+{
+    return ClientPortNumber;
+}
+
 std::vector<int> LOOLWSD::getKitPids()
 {
     std::vector<int> pids;
diff --git a/wsd/LOOLWSD.hpp b/wsd/LOOLWSD.hpp
index 71ddb2155..35c283288 100644
--- a/wsd/LOOLWSD.hpp
+++ b/wsd/LOOLWSD.hpp
@@ -80,6 +80,8 @@ public:
     static std::set<const Poco::Util::AbstractConfiguration*> PluginConfigurations;
     static std::chrono::time_point<std::chrono::system_clock> StartTime;
 
+    /// For testing only [!]
+    static int getClientPortNumber();
     /// For testing only [!]
     static std::vector<int> getKitPids();
     /// For testing only [!] DocumentBrokers are mostly single-threaded with their own thread
commit 536195be9027a8a4dc003dc1418c129115c1f0cd
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Jan 17 20:49:38 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 16:40:05 2020 +0000

    test: switch to parallel tests based on Unit framework.
    
    Increase a few timeouts, bin old-style standalone unit tests,
    fix a number of bugs.
    
    Change-Id: Ia3d59466ecb9a9443807ba3445d04dd5f77e3dba

diff --git a/common/Unit.cpp b/common/Unit.cpp
index 5dc01d4ea..875c97686 100644
--- a/common/Unit.cpp
+++ b/common/Unit.cpp
@@ -26,7 +26,7 @@
 #include <common/SigUtil.hpp>
 
 UnitBase *UnitBase::Global = nullptr;
-
+char * UnitBase::UnitLibPath;
 static std::thread TimeoutThread;
 static std::atomic<bool> TimeoutThreadRunning(false);
 std::timed_mutex TimeoutThreadMutex;
@@ -41,6 +41,9 @@ UnitBase *UnitBase::linkAndCreateUnit(UnitType type, const std::string &unitLibP
         return nullptr;
     }
 
+    // avoid std:string de-allocation during failure / exit.
+    UnitLibPath = strdup(unitLibPath.c_str());
+
     const char *symbol = nullptr;
     switch (type)
     {
diff --git a/common/Unit.hpp b/common/Unit.hpp
index 63975514b..c403100c2 100644
--- a/common/Unit.hpp
+++ b/common/Unit.hpp
@@ -138,11 +138,17 @@ public:
         return *Global;
     }
 
+    static std::string getUnitLibPath() { return std::string(UnitLibPath); }
+
 private:
-    void setHandle(void *dlHandle) { _dlHandle = dlHandle; }
+    void setHandle(void *dlHandle)
+    {
+        _dlHandle = dlHandle;
+    }
     static UnitBase *linkAndCreateUnit(UnitType type, const std::string& unitLibPath);
 
     void *_dlHandle;
+    static char *UnitLibPath;
     bool _setRetValue;
     int _retValue;
     int _timeoutMilliSeconds;
diff --git a/test/Makefile.am b/test/Makefile.am
index f1d4963b1..2b369465a 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -13,9 +13,10 @@ AM_CXXFLAGS = $(CPPUNIT_CFLAGS) -DTDOC=\"$(abs_top_srcdir)/test/data\" \
 	-I${top_srcdir}/common -I${top_srcdir}/net -I${top_srcdir}/wsd -I${top_srcdir}/kit
 
 noinst_LTLIBRARIES = \
+	unit-base.la unit-tiletest.la \
+	unit-integration.la unit-httpws.la unit-crash.la \
 	unit-convert.la unit-typing.la unit-copy-paste.la \
-	unit-timeout.la unit-prefork.la \
-	unit-storage.la unit-client.la \
+	unit-timeout.la unit-prefork.la unit-storage.la \
 	unit-admin.la unit-tilecache.la \
 	unit-fuzz.la unit-oob.la unit-http.la unit-oauth.la \
 	unit-wopi.la unit-wopi-saveas.la \
@@ -82,13 +83,6 @@ test_base_source = \
 	DeltaTests.cpp \
 	$(wsd_sources)
 
-test_all_source = \
-	$(test_base_source) \
-	TileCacheTests.cpp \
-	integration-http-server.cpp \
-	httpwstest.cpp \
-	httpcrashtest.cpp
-
 unittest_CPPFLAGS = -I$(top_srcdir) -DBUILDING_TESTS
 unittest_SOURCES = $(test_base_source) test.cpp
 unittest_LDADD = $(CPPUNIT_LIBS)
@@ -100,6 +94,18 @@ test_LDADD = $(CPPUNIT_LIBS)
 fakesockettest_SOURCES = fakesockettest.cpp ../net/FakeSocket.cpp
 fakesockettest_LDADD = $(CPPUNIT_LIBS)
 
+# old-style unit tests - bootstrapped via UnitClient
+unit_base_la_SOURCES = UnitClient.cpp ${test_base_source}
+unit_base_la_LIBADD = $(CPPUNIT_LIBS)
+unit_tiletest_la_SOURCES = UnitClient.cpp TileCacheTests.cpp
+unit_tiletest_la_LIBADD = $(CPPUNIT_LIBS)
+unit_integration_la_SOURCES = UnitClient.cpp integration-http-server.cpp
+unit_integration_la_LIBADD = $(CPPUNIT_LIBS)
+unit_httpws_la_SOURCES = UnitClient.cpp httpwstest.cpp
+unit_httpws_la_LIBADD = $(CPPUNIT_LIBS)
+unit_crash_la_SOURCES = UnitClient.cpp httpcrashtest.cpp
+unit_crash_la_LIBADD = $(CPPUNIT_LIBS)
+
 # unit test modules:
 unit_oob_la_SOURCES = UnitOOB.cpp
 unit_http_la_SOURCES = UnitHTTP.cpp
@@ -107,8 +113,6 @@ unit_http_la_LIBADD = $(CPPUNIT_LIBS)
 unit_fuzz_la_SOURCES = UnitFuzz.cpp
 unit_admin_la_SOURCES = UnitAdmin.cpp
 unit_admin_la_LIBADD = $(CPPUNIT_LIBS)
-unit_client_la_SOURCES = UnitClient.cpp ${test_all_source}
-unit_client_la_LIBADD = $(CPPUNIT_LIBS)
 unit_typing_la_SOURCES = UnitTyping.cpp
 unit_typing_la_LIBADD = $(CPPUNIT_LIBS)
 unit_copy_paste_la_SOURCES = UnitCopyPaste.cpp
@@ -183,16 +187,20 @@ if HAVE_LO_PATH
 check-local:
 	./fakesockettest
 	@fc-cache "@LO_PATH@"/share/fonts/truetype
-	./run_unit.sh --log-file test.log --trs-file test.trs
+
 # FIXME 2: unit-oob.la fails with symbol undefined:
 # UnitWSD::testHandleRequest(UnitWSD::TestRequest, UnitHTTPServerRequest&, UnitHTTPServerResponse&) ,
-TESTS = unit-copy-paste.la unit-typing.la unit-convert.la unit-prefork.la unit-tilecache.la unit-timeout.la \
+TESTS = \
+	unit-base.la unit-tiletest.la \
+	unit-integration.la unit-httpws.la unit-crash.la \
+	\
+	unit-copy-paste.la unit-typing.la unit-convert.la unit-prefork.la unit-tilecache.la unit-timeout.la \
         unit-oauth.la unit-wopi.la unit-wopi-saveas.la \
         unit-wopi-ownertermination.la unit-wopi-versionrestore.la \
         unit-wopi-documentconflict.la unit_wopi_renamefile.la unit_wopi_watermark.la \
 	unit-http.la \
 	unit-tiff-load.la \
-	unit-large-paste.la \
+	unit.large-paste.la \
 	unit-paste.la \
 	unit-load-torture.la \
 	unit-rendering-options.la \
@@ -209,9 +217,8 @@ TESTS = unit-copy-paste.la unit-typing.la unit-convert.la unit-prefork.la unit-t
 	unit-bad-doc-load.la \
 	unit-hosting.la \
 	unit-wopi-loadencoded.la unit-wopi-temp.la
-# TESTS = unit-client.la
-# TESTS += unit-admin.la
-# TESTS += unit-storage.la
+# TESTS += unit-admin.test
+# TESTS += unit-storage.test
 else
 TESTS = ${top_builddir}/test/test
 endif
diff --git a/test/UnitClient.cpp b/test/UnitClient.cpp
index e3580f90e..03682fdc5 100644
--- a/test/UnitClient.cpp
+++ b/test/UnitClient.cpp
@@ -7,7 +7,9 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
-// Runs client tests in their own thread inside a WSD process.
+// Runs old-style CPPUNIT tests in their own thread inside a WSD process.
+// Depending which cppunit objects this is linked with this runs different
+// tests.
 
 #include <config.h>
 
diff --git a/test/helpers.hpp b/test/helpers.hpp
index f09de8bbb..dd638d191 100644
--- a/test/helpers.hpp
+++ b/test/helpers.hpp
@@ -390,8 +390,12 @@ inline
 bool isDocumentLoaded(LOOLWebSocket& ws, const std::string& testname, bool isView = true)
 {
     const std::string prefix = isView ? "status:" : "statusindicatorfinish:";
-    const auto message = getResponseString(ws, prefix, testname);
-    return LOOLProtocol::matchPrefix(prefix, message);
+    // Allow 20 secs to load
+    const auto message = getResponseString(ws, prefix, testname, 30 * 1000);
+    bool success = LOOLProtocol::matchPrefix(prefix, message);
+    if (!success)
+        TST_LOG("ERR: Timed out loading document");
+    return success;
 }
 
 inline
diff --git a/test/test.cpp b/test/test.cpp
index cc00b2fde..4a5b0f6d0 100644
--- a/test/test.cpp
+++ b/test/test.cpp
@@ -183,7 +183,8 @@ bool runClientTests(bool standalone, bool verbose)
             std::cerr << "  (cd test; CPPUNIT_TEST_NAME=\"" << (*failures.begin())->failedTestName() << "\" gdb --args " << cmd << ")\n\n";
         }
 #else
-        std::cerr << "(cd test; CPPUNIT_TEST_NAME=\"" << (*failures.begin())->failedTestName() << "\" make check)\n\n";
+        std::cerr << "(cd test; CPPUNIT_TEST_NAME=\"" << (*failures.begin())->failedTestName() <<
+            "\" ./run_unit.sh --test-name " << UnitBase::get().getUnitLibPath() << ")\n\n";
 #endif
     }
 
diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp
index 9bf7088c6..876436abe 100644
--- a/wsd/DocumentBroker.cpp
+++ b/wsd/DocumentBroker.cpp
@@ -309,9 +309,9 @@ void DocumentBroker::pollThread()
 
         if (!_isLoaded && (limit_load_secs > 0) && (now > loadDeadline))
         {
-            LOG_WRN("Doc [" << _docKey << "] is taking too long to load. Will kill process ["
-                            << _childProcess->getPid() << "]. per_document.limit_load_secs set to "
-                            << limit_load_secs << " secs.");
+            LOG_ERR("Doc [" << _docKey << "] is taking too long to load. Will kill process ["
+                    << _childProcess->getPid() << "]. per_document.limit_load_secs set to "
+                    << limit_load_secs << " secs.");
             broadcastMessage("error: cmd=load kind=docloadtimeout");
 
             // Brutal but effective.
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index d47253e93..2122d9e8e 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -3283,14 +3283,19 @@ private:
 
         std::shared_ptr<ServerSocket> socket = getServerSocket(
             ClientListenAddr, port, WebServerPoll, factory);
+
+        while (!socket &&
 #ifdef BUILDING_TESTS
-        while (!socket)
+               true
+#else
+               UnitWSD::isUnitTesting()
+#endif
+            )
         {
             ++port;
             LOG_INF("Client port " << (port - 1) << " is busy, trying " << port << ".");
-            socket = getServerSocket(port, WebServerPoll, factory);
+            socket = getServerSocket(ClientListenAddr, port, WebServerPoll, factory);
         }
-#endif
 
         if (!socket)
         {
commit f13d626a03ecd16d8e33d19c7d1771dcbf0f6981
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Sat Jan 18 15:56:01 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 16:40:05 2020 +0000

    copyFile: de-poco-ize and handle EINTR and short writes.
    
    Change-Id: I2046881c786a9f31f45c53f282de9ddd9a9cebcf

diff --git a/common/FileUtil.cpp b/common/FileUtil.cpp
index 5066d2938..cb8f07f13 100644
--- a/common/FileUtil.cpp
+++ b/common/FileUtil.cpp
@@ -88,6 +88,79 @@ namespace FileUtil
         return name;
     }
 
+    void copyFileTo(const std::string &fromPath, const std::string &toPath)
+    {
+        int from = -1, to = -1;
+        try {
+            from = open(fromPath.c_str(), O_RDONLY);
+            if (from < 0)
+            {
+                LOG_SYS("Failed to open src " << anonymizeUrl(fromPath));
+                throw;
+            }
+
+            struct stat st;
+            if (fstat(from, &st) != 0)
+            {
+                LOG_SYS("Failed to fstat src " << anonymizeUrl(fromPath));
+                throw;
+            }
+
+            to = open(toPath.c_str(), O_CREAT | O_TRUNC | O_WRONLY, st.st_mode);
+            if (to < 0)
+            {
+                LOG_SYS("Failed to fstat dest " << anonymizeUrl(toPath));
+                throw;
+            }
+
+            LOG_INF("Copying " << st.st_size << " bytes from " << anonymizeUrl(fromPath) << " to " << anonymizeUrl(toPath));
+
+            char buffer[4096*64];
+
+            int n;
+            off_t bytesIn = 0;
+            do {
+                while ((n = ::read(from, buffer, sizeof(buffer))) < 0 && errno == EINTR)
+                    LOG_TRC("EINTR reading from " << anonymizeUrl(fromPath));
+                if (n < 0)
+                {
+                    LOG_SYS("Failed to read from " << anonymizeUrl(fromPath) << " at " << bytesIn << " bytes in");
+                    throw;
+                }
+                bytesIn += n;
+                if (n == 0) // EOF
+                    break;
+                // Handle short writes and EINTR
+                for (int j = 0; j < n;)
+                {
+                    int written;
+                    while ((written = ::write(to, buffer + j, sizeof(buffer) - j)) < 0 && errno == EINTR)
+                        LOG_TRC("EINTR writing to " << anonymizeUrl(toPath));
+                    if (written < 0)
+                    {
+                        LOG_SYS("Failed to write " << n << " bytes to " << anonymizeUrl(toPath) << " at " <<
+                                bytesIn << " bytes into " << anonymizeUrl(fromPath));
+                        throw;
+                    }
+                    j += written;
+                }
+            } while(true);
+            if (bytesIn != st.st_size)
+            {
+                LOG_WRN("Unusual: file " << anonymizeUrl(fromPath) << " changed size "
+                        "during copy from " << st.st_size << " to " << bytesIn);
+            }
+        }
+        catch (...)
+        {
+            LOG_SYS("Failed to copy from " << anonymizeUrl(fromPath) << " to " << anonymizeUrl(toPath));
+            close(from);
+            close(to);
+            unlink(toPath.c_str());
+            throw Poco::Exception("failed to copy");
+        }
+    }
+
     std::string getTempFilePath(const std::string& srcDir, const std::string& srcFilename, const std::string& dstFilenamePrefix)
     {
         const std::string srcPath = srcDir + '/' + srcFilename;
@@ -100,7 +173,7 @@ namespace FileUtil
         fileDeleter.registerForDeletion(dstPath);
 #else
         const std::string dstPath = Poco::Path(Poco::Path::temp(), dstFilename).toString();
-        Poco::File(srcPath).copyTo(dstPath);
+        copyFileTo(srcPath, dstPath);
         Poco::TemporaryFile::registerForDeletion(dstPath);
 #endif
 
diff --git a/common/FileUtil.hpp b/common/FileUtil.hpp
index a57aa5414..864188308 100644
--- a/common/FileUtil.hpp
+++ b/common/FileUtil.hpp
@@ -80,6 +80,9 @@ namespace FileUtil
         removeFile(path.toString(), recursive);
     }
 
+    /// Copy a file from @fromPath to @toPath, throws on failure.
+    void copyFileTo(const std::string &fromPath, const std::string &toPath);
+
     /// Make a temp copy of a file, and prepend it with a prefix.
     std::string getTempFilePath(const std::string& srcDir, const std::string& srcFilename,
                                 const std::string& dstFilenamePrefix);
diff --git a/test/run_unit.sh.in b/test/run_unit.sh.in
index d05e2f48f..5fb8a6936 100755
--- a/test/run_unit.sh.in
+++ b/test/run_unit.sh.in
@@ -56,16 +56,6 @@ echo "	$cmd_line"
 # drop .la suffix
 tst=`echo $tst | sed "s/\.la//"`;
 
-if test "z$tst" != "z" && test "z$CPPUNIT_TEST_NAME" != "z"; then
-    # $tst is not empty, but $CPPUNIT_TEST_NAME is set, exit early if they
-    # don't match.
-    if test "z$tst" != "z$CPPUNIT_TEST_NAME"; then
-        touch $tst_log
-        echo ":test-result: SKIP $tst (disabled by CPPUNIT_TEST_NAME)" > $test_output
-        exit 0;
-    fi
-fi
-
 export LOOL_LOGLEVEL=trace
 
 if test "z$enable_debug" != "ztrue"; then
diff --git a/wsd/Storage.cpp b/wsd/Storage.cpp
index e1efa864c..f35bf7c3e 100644
--- a/wsd/Storage.cpp
+++ b/wsd/Storage.cpp
@@ -331,8 +331,7 @@ std::string LocalStorage::loadStorageFileToLocal(const Authorization& /*auth*/,
         // Fallback to copying.
         if (!Poco::File(getRootFilePath()).exists())
         {
-            LOG_INF("Copying " << LOOLWSD::anonymizeUrl(publicFilePath) << " to " << getRootFilePathAnonym());
-            Poco::File(publicFilePath).copyTo(getRootFilePath());
+            FileUtil::copyFileTo(publicFilePath, getRootFilePath());
             _isCopy = true;
         }
     }
@@ -371,10 +370,7 @@ StorageBase::SaveResult LocalStorage::saveLocalFileToStorage(const Authorization
         LOG_TRC("Saving local file to local file storage (isCopy: " << _isCopy << ") for " << getRootFilePathAnonym());
         // Copy the file back.
         if (_isCopy && Poco::File(getRootFilePath()).exists())
-        {
-            LOG_INF("Copying " << getRootFilePathAnonym() << " to " << LOOLWSD::anonymizeUrl(getUri().getPath()));
-            Poco::File(getRootFilePath()).copyTo(getUri().getPath());
-        }
+            FileUtil::copyFileTo(getRootFilePath(), getUri().getPath());
 
         // update its fileinfo object. This is used later to check if someone else changed the
         // document while we are/were editing it
diff --git a/wsd/TraceFile.hpp b/wsd/TraceFile.hpp
index 82f5a75ce..e95fbb2e2 100644
--- a/wsd/TraceFile.hpp
+++ b/wsd/TraceFile.hpp
@@ -25,6 +25,7 @@
 #include "Protocol.hpp"
 #include "Log.hpp"
 #include "Util.hpp"
+#include "FileUtil.hpp"
 
 /// Dumps commands and notification trace.
 class TraceFileRecord
@@ -141,8 +142,7 @@ public:
                 filename += '.' + origPath.getExtension();
                 snapshot = Poco::Path(_path, filename).toString();
 
-                LOG_TRC("TraceFile: Copying local file [" << localPath << "] to snapshot [" << snapshot << "].");
-                Poco::File(localPath).copyTo(snapshot);
+                FileUtil::copyFileTo(localPath, snapshot);
                 snapshot = Poco::URI(Poco::URI("file://"), snapshot).toString();
 
                 LOG_TRC("TraceFile: Mapped URL " << url << " to " << snapshot);
commit b8fee3f24f8cdac32460223b24af562f4ed08330
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Jan 17 22:31:41 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 16:40:05 2020 +0000

    Move file url anonymization down from LOOLWSD into FileUtil.
    
    Change-Id: I415c73b10621d5c7c942367bbf38a3bbd9bf8f27

diff --git a/common/FileUtil.cpp b/common/FileUtil.cpp
index f94911606..5066d2938 100644
--- a/common/FileUtil.cpp
+++ b/common/FileUtil.cpp
@@ -343,6 +343,30 @@ namespace FileUtil
         return true;
     }
 
+    namespace {
+        bool AnonymizeUserData = false;
+        std::uint64_t AnonymizationSalt = 82589933;
+    }
+
+    void setUrlAnonymization(bool anonymize, const std::uint64_t salt)
+    {
+        AnonymizeUserData = anonymize;
+        AnonymizationSalt = salt;
+    }
+
+    /// Anonymize the basename of filenames, preserving the path and extension.
+    std::string anonymizeUrl(const std::string& url)
+    {
+        return AnonymizeUserData ? Util::anonymizeUrl(url, AnonymizationSalt) : url;
+    }
+
+    /// Anonymize user names and IDs.
+    /// Will use the Obfuscated User ID if one is provied via WOPI.
+    std::string anonymizeUsername(const std::string& username)
+    {
+        return AnonymizeUserData ? Util::anonymize(username, AnonymizationSalt) : username;
+    }
+
 } // namespace FileUtil
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/common/FileUtil.hpp b/common/FileUtil.hpp
index 84b42f65c..a57aa5414 100644
--- a/common/FileUtil.hpp
+++ b/common/FileUtil.hpp
@@ -17,6 +17,16 @@
 
 namespace FileUtil
 {
+    /// Used for anonymizing URLs
+    void setUrlAnonymization(bool anonymize, const std::uint64_t salt);
+
+    /// Anonymize the basename of filenames, preserving the path and extension.
+    std::string anonymizeUrl(const std::string& url);
+
+    /// Anonymize user names and IDs.
+    /// Will use the Obfuscated User ID if one is provied via WOPI.
+    std::string anonymizeUsername(const std::string& username);
+
     /// Create a secure, random directory path.
     std::string createRandomDir(const std::string& path);
 
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 3b45fd3de..d47253e93 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -715,8 +715,7 @@ std::string LOOLWSD::HostIdentifier;
 std::string LOOLWSD::ConfigFile = LOOLWSD_CONFIGDIR "/loolwsd.xml";
 std::string LOOLWSD::ConfigDir = LOOLWSD_CONFIGDIR "/conf.d";
 std::string LOOLWSD::LogLevel = "trace";
-bool LOOLWSD::AnonymizeUserData = false;
-std::uint64_t LOOLWSD::AnonymizationSalt = 82589933;
+bool LOOLWSD::AnonymizeUserData = 82589933;
 #if ENABLE_SSL
 Util::RuntimeConstant<bool> LOOLWSD::SSLEnabled;
 Util::RuntimeConstant<bool> LOOLWSD::SSLTermination;
@@ -1003,14 +1002,16 @@ void LOOLWSD::initialize(Application& self)
         }
     }
 
+    std::uint64_t anonymizationSalt;
     LOG_INF("Anonymization of user-data is " << (AnonymizeUserData ? "enabled." : "disabled."));
     if (AnonymizeUserData)
     {
         // Get the salt, if set, otherwise default, and set as envar, so the kits inherit it.
-        AnonymizationSalt = getConfigValue<std::uint64_t>(conf, "logging.anonymize.anonymization_salt", 82589933);
-        const std::string sAnonymizationSalt = std::to_string(AnonymizationSalt);
-        setenv("LOOL_ANONYMIZATION_SALT", sAnonymizationSalt.c_str(), true);
+        anonymizationSalt = getConfigValue<std::uint64_t>(conf, "logging.anonymize.anonymization_salt", 82589933);
+        const std::string anonymizationSaltStr = std::to_string(anonymizationSalt);
+        setenv("LOOL_ANONYMIZATION_SALT", anonymizationSaltStr.c_str(), true);
     }
+    FileUtil::setUrlAnonymization(AnonymizeUserData, anonymizationSalt);
 
     {
         std::string proto = getConfigValue<std::string>(conf, "net.proto", "");
diff --git a/wsd/LOOLWSD.hpp b/wsd/LOOLWSD.hpp
index 6e35bd796..71ddb2155 100644
--- a/wsd/LOOLWSD.hpp
+++ b/wsd/LOOLWSD.hpp
@@ -24,6 +24,7 @@
 #include <Poco/Util/ServerApplication.h>
 
 #include "Util.hpp"
+#include "FileUtil.hpp"
 
 class ChildProcess;
 class TraceFileWriter;
@@ -67,7 +68,6 @@ public:
     static std::string HostIdentifier; ///< A unique random hash that identifies this server
     static std::string LogLevel;
     static bool AnonymizeUserData;
-    static std::uint64_t AnonymizationSalt;
     static std::atomic<unsigned> NumConnections;
     static std::unique_ptr<TraceFileWriter> TraceDumper;
 #if !MOBILEAPP
@@ -184,14 +184,14 @@ public:
     /// Anonymize the basename of filenames, preserving the path and extension.
     static std::string anonymizeUrl(const std::string& url)
     {
-        return AnonymizeUserData ? Util::anonymizeUrl(url, AnonymizationSalt) : url;
+        return FileUtil::anonymizeUrl(url);
     }
 
     /// Anonymize user names and IDs.
     /// Will use the Obfuscated User ID if one is provied via WOPI.
     static std::string anonymizeUsername(const std::string& username)
     {
-        return AnonymizeUserData ? Util::anonymize(username, AnonymizationSalt) : username;
+        return FileUtil::anonymizeUsername(username);
     }
 
     /// get correct server URL with protocol + port number for this running server
commit 64be08cb5a94734bedb291588d988d949dc7740e
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Sat Jan 18 16:36:50 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 16:40:05 2020 +0000

    cleanup JWTAuth token before shutdown.
    
    valgrind had some exciting double-free action on unclean shutdown.
    
    Change-Id: Id7dd3d8ff60387ae51521bd2c74e4d6bcc30ff2e

diff --git a/wsd/Auth.cpp b/wsd/Auth.cpp
index 01f9e4f43..2ffa17b39 100644
--- a/wsd/Auth.cpp
+++ b/wsd/Auth.cpp
@@ -37,7 +37,14 @@ using Poco::Base64Decoder;
 using Poco::Base64Encoder;
 using Poco::OutputLineEndingConverter;
 
-const Poco::Crypto::RSAKey JWTAuth::_key(Poco::Crypto::RSAKey(Poco::Crypto::RSAKey::KL_2048, Poco::Crypto::RSAKey::EXP_LARGE));
+std::unique_ptr<Poco::Crypto::RSAKey> JWTAuth::_key(
+    new Poco::Crypto::RSAKey(Poco::Crypto::RSAKey(Poco::Crypto::RSAKey::KL_2048, Poco::Crypto::RSAKey::EXP_LARGE)));
+
+// avoid obscure doublef rees on exit.
+void JWTAuth::cleanup()
+{
+    _key.reset();
+}
 
 const std::string JWTAuth::getAccessToken()
 {
diff --git a/wsd/Auth.hpp b/wsd/Auth.hpp
index 1220aee21..00256fb1f 100644
--- a/wsd/Auth.hpp
+++ b/wsd/Auth.hpp
@@ -13,6 +13,7 @@
 
 #include <cassert>
 #include <string>
+#include <memory>
 
 #if !MOBILEAPP
 #include <Poco/Crypto/RSADigestEngine.h>
@@ -43,7 +44,7 @@ public:
         : _name(name),
           _sub(sub),
           _aud(aud),
-          _digestEngine(_key, "SHA256")
+          _digestEngine(*_key, "SHA256")
     {
     }
 
@@ -51,6 +52,8 @@ public:
 
     bool verify(const std::string& accessToken) override;
 
+    static void cleanup();
+
 private:
     const std::string createHeader();
 
@@ -65,7 +68,7 @@ private:
     const std::string _sub;
     const std::string _aud;
 
-    static const Poco::Crypto::RSAKey _key;
+    static std::unique_ptr<Poco::Crypto::RSAKey> _key;
     Poco::Crypto::RSADigestEngine _digestEngine;
 };
 
diff --git a/wsd/LOOLWSD.cpp b/wsd/LOOLWSD.cpp
index 0538759ba..3b45fd3de 100644
--- a/wsd/LOOLWSD.cpp
+++ b/wsd/LOOLWSD.cpp
@@ -3592,6 +3592,7 @@ void LOOLWSD::cleanup()
 {
 #if !MOBILEAPP
     FileServerRequestHandler::uninitialize();
+    JWTAuth::cleanup();
 
 #if ENABLE_SSL
     // Finally, we no longer need SSL.
commit b72f37826edf742eb7d9cf4fd0ac73b2a2e2096b
Author:     Michael Meeks <michael.meeks at collabora.com>
AuthorDate: Fri Jan 17 19:03:23 2020 +0000
Commit:     Michael Meeks <michael.meeks at collabora.com>
CommitDate: Sat Jan 18 17:23:02 2020 +0100

    sighandler: break infinite loop with corrupted heap
    
    ignoring the segv can lead to not making progress, while churning debug.
    
    Change-Id: I97af266cec3feefe2dcbd9adb8dbf4b13a4d69bd
    Reviewed-on: https://gerrit.libreoffice.org/c/online/+/87002
    Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice at gmail.com>
    Reviewed-by: Michael Meeks <michael.meeks at collabora.com>

diff --git a/common/SigUtil.cpp b/common/SigUtil.cpp
index 8fe38b721..881326df5 100644
--- a/common/SigUtil.cpp
+++ b/common/SigUtil.cpp
@@ -237,19 +237,16 @@ namespace SigUtil
     void handleFatalSignal(const int signal)
     {
         SigHandlerTrap guard;
-        if (!guard.isExclusive())
-        {
-            Log::signalLogPrefix();
-            Log::signalLog(" Fatal double signal received: ");
-            Log::signalLog(signalName(signal));
-            Log::signalLog("\n Already handling a signal; will ignore this.");
-            return;
-        }
+        bool bReEntered = !guard.isExclusive();
 
         Log::signalLogPrefix();
-        Log::signalLog(" Fatal signal received: ");
+
+        // Heap corruption can re-enter through backtrace.
+        if (bReEntered)
+            Log::signalLog(" Fatal double signal received: ");
+        else
+            Log::signalLog(" Fatal signal received: ");
         Log::signalLog(signalName(signal));
-        Log::signalLog("\n");
 
         struct sigaction action;
 
@@ -259,7 +256,8 @@ namespace SigUtil
 
         sigaction(signal, &action, nullptr);
 
-        dumpBacktrace();
+        if (!bReEntered)
+            dumpBacktrace();
 
         // let default handler process the signal
         kill(getpid(), signal);


More information about the Libreoffice-commits mailing list