Cleanup Download code, fixing SDL_Thread leaking

The download thread was setting itself to nullptr (d->mThread = nullptr)
in a number of locations. This caused a later call to SDL_WaitThread to
be unable to perform cleanup.

This reverts most of 1eb02f83a5d3895e4e18db30ea10d88da94ba4c0 (including
making Download::cancel no longer blocking), but keeps the necessary
waiting for the thread to finish before freeing the memory buffer in
~UpdaterWindow(), which might have been the bug fixed by that change.

Fixed removal of downloaded .part file when its checksum failed. It
trying to remove the file without .part appended instead.

Fixed download progress indication in ServerDialog to not be reversed,
though this is rarely visible due to the server list being so small.

Fixed reporting of curl error.

2 files changed, 124 insertions, 159 deletions

diff --git a/src/net/download.cpp b/src/net/download.cpp
index 8a41ebfa..2cfdc3a1 100644
--- a/src/net/download.cpp
+++ b/src/net/download.cpp
@@ -31,7 +31,7 @@
 
 #include <zlib.h>
 
-const char *DOWNLOAD_ERROR_MESSAGE_THREAD = "Could not create download thread!";
+constexpr char DOWNLOAD_ERROR_MESSAGE_THREAD[] = "Could not create download thread!";
 
 namespace Net {
 
@@ -59,12 +59,12 @@ unsigned long Download::fadler32(FILE *file)
     return adler;
 }
 
-
-Download::Download(void *ptr, const std::string &url,
-                   DownloadUpdate updateFunction):
-        mPtr(ptr),
-        mUrl(url),
-        mUpdateFunction(updateFunction)
+Download::Download(void *ptr,
+                   const std::string &url,
+                   DownloadUpdate updateFunction)
+    : mPtr(ptr)
+    , mUrl(url)
+    , mUpdateFunction(updateFunction)
 {
     mError = (char*) malloc(CURL_ERROR_SIZE);
     mError[0] = 0;
@@ -74,17 +74,15 @@ Download::Download(void *ptr, const std::string &url,
 
 Download::~Download()
 {
-    if (mHeaders)
-        curl_slist_free_all(mHeaders);
+    SDL_WaitThread(mThread, nullptr);
 
-    int status;
-    SDL_WaitThread(mThread, &status);
+    curl_slist_free_all(mHeaders);
     free(mError);
 }
 
-void Download::addHeader(const std::string &header)
+void Download::addHeader(const char *header)
 {
-    mHeaders = curl_slist_append(mHeaders, header.c_str());
+    mHeaders = curl_slist_append(mHeaders, header);
 }
 
 void Download::noCache()
@@ -101,7 +99,7 @@ void Download::setFile(const std::string &filename,
     mAdler = adler32;
 }
 
-void Download::setWriteFunction(WriteFunction write)
+void Download::setWriteFunction(curl_write_callback write)
 {
     mOptions.memoryWrite = true;
     mWriteFunction = write;
@@ -116,7 +114,7 @@ bool Download::start()
     if (!mThread)
     {
         logger->log("%s", DOWNLOAD_ERROR_MESSAGE_THREAD);
-        strcpy(mError, DOWNLOAD_ERROR_MESSAGE_THREAD);
+        strncpy(mError, DOWNLOAD_ERROR_MESSAGE_THREAD, CURL_ERROR_SIZE - 1);
         mUpdateFunction(mPtr, DOWNLOAD_STATUS_THREAD_ERROR, 0, 0);
 
         return false;
@@ -128,13 +126,7 @@ bool Download::start()
 void Download::cancel()
 {
     logger->log("Canceling download: %s", mUrl.c_str());
-
     mOptions.cancel = true;
-    if (mThread && SDL_GetThreadID(mThread) != 0)
-    {
-        SDL_WaitThread(mThread, nullptr);
-        mThread = nullptr;
-    }
 }
 
 const char *Download::getError() const
@@ -147,177 +139,149 @@ int Download::downloadProgress(void *clientp,
                                curl_off_t ultotal, curl_off_t ulnow)
 {
     auto *d = reinterpret_cast<Download*>(clientp);
+    DownloadStatus status = d->mOptions.cancel ? DOWNLOAD_STATUS_CANCELLED
+                                               : DOWNLOAD_STATUS_IN_PROGRESS;
 
-    if (d->mOptions.cancel)
-    {
-        return d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_CANCELLED,
-                                  (size_t) dltotal, (size_t) dlnow);
-        return -5;
-    }
-
-    return d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_IDLE,
-                              (size_t) dltotal, (size_t) dlnow);
+    return d->mUpdateFunction(d->mPtr, status, (size_t) dltotal, (size_t) dlnow);
 }
 
 int Download::downloadThread(void *ptr)
 {
-    int attempts = 0;
-    bool complete = false;
     auto *d = reinterpret_cast<Download*>(ptr);
-    CURLcode res;
+    bool complete = false;
     std::string outFilename;
 
-    if (!d)
-    {
-        return 0;
-    }
     if (!d->mOptions.memoryWrite)
-    {
         outFilename = d->mFileName + ".part";
-    }
 
-    while (attempts < 3 && !complete && !d->mOptions.cancel)
+    for (int attempts = 0; attempts < 3 && !complete && !d->mOptions.cancel; ++attempts)
     {
-        FILE *file = nullptr;
-
         d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_STARTING, 0, 0);
 
-        if (d->mOptions.cancel)
-        {
-            d->mThread = nullptr;
-            return 0;
-        }
+        CURL *curl = curl_easy_init();
+        if (!curl)
+            break;
+
+        logger->log("Downloading: %s", d->mUrl.c_str());
 
-        d->mCurl = curl_easy_init();
+        curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1);
+        curl_easy_setopt(curl, CURLOPT_HTTPHEADER, d->mHeaders);
 
-        if (d->mCurl && !d->mOptions.cancel)
+        FILE *file = nullptr;
+
+        if (d->mOptions.memoryWrite)
+        {
+            curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1);
+            curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, d->mWriteFunction);
+            curl_easy_setopt(curl, CURLOPT_WRITEDATA, d->mPtr);
+        }
+        else
         {
-            logger->log("Downloading: %s", d->mUrl.c_str());
+            file = fopen(outFilename.c_str(), "w+b");
+            curl_easy_setopt(curl, CURLOPT_WRITEDATA, file);
+        }
 
-            curl_easy_setopt(d->mCurl, CURLOPT_FOLLOWLOCATION, 1);
-            curl_easy_setopt(d->mCurl, CURLOPT_HTTPHEADER, d->mHeaders);
+        const std::string appShort = branding.getStringValue("appShort");
+        const std::string userAgent =
+                strprintf(PACKAGE_EXTENDED_VERSION, appShort.c_str());
 
-            if (d->mOptions.memoryWrite)
-            {
-                curl_easy_setopt(d->mCurl, CURLOPT_FAILONERROR, 1);
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEFUNCTION, d->mWriteFunction);
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEDATA, d->mPtr);
-            }
-            else
-            {
-                file = fopen(outFilename.c_str(), "w+b");
-                curl_easy_setopt(d->mCurl, CURLOPT_WRITEDATA, file);
-            }
+        curl_easy_setopt(curl, CURLOPT_USERAGENT, userAgent.c_str());
+        curl_easy_setopt(curl, CURLOPT_ERRORBUFFER, d->mError);
+        curl_easy_setopt(curl, CURLOPT_URL, d->mUrl.c_str());
+        curl_easy_setopt(curl, CURLOPT_NOPROGRESS, 0);
+        curl_easy_setopt(curl, CURLOPT_XFERINFOFUNCTION, downloadProgress);
+        curl_easy_setopt(curl, CURLOPT_XFERINFODATA, ptr);
+        curl_easy_setopt(curl, CURLOPT_NOSIGNAL, 1);
+        curl_easy_setopt(curl, CURLOPT_CONNECTTIMEOUT, 15);
 
-            const std::string appShort = branding.getStringValue("appShort");
-            const std::string userAgent =
-                    strprintf(PACKAGE_EXTENDED_VERSION, appShort.c_str());
+        const CURLcode res = curl_easy_perform(curl);
+        curl_easy_cleanup(curl);
 
-            curl_easy_setopt(d->mCurl, CURLOPT_USERAGENT, userAgent.c_str());
-            curl_easy_setopt(d->mCurl, CURLOPT_ERRORBUFFER, d->mError);
-            curl_easy_setopt(d->mCurl, CURLOPT_URL, d->mUrl.c_str());
-            curl_easy_setopt(d->mCurl, CURLOPT_NOPROGRESS, 0);
-            curl_easy_setopt(d->mCurl, CURLOPT_XFERINFOFUNCTION, downloadProgress);
-            curl_easy_setopt(d->mCurl, CURLOPT_XFERINFODATA, ptr);
-            curl_easy_setopt(d->mCurl, CURLOPT_NOSIGNAL, 1);
-            curl_easy_setopt(d->mCurl, CURLOPT_CONNECTTIMEOUT, 15);
+        if (res == CURLE_ABORTED_BY_CALLBACK)
+        {
+            d->mOptions.cancel = true;
 
-            if ((res = curl_easy_perform(d->mCurl)) != 0 && !d->mOptions.cancel)
+            if (file)
             {
-                switch (res)
-                {
-                case CURLE_ABORTED_BY_CALLBACK:
-                    d->mOptions.cancel = true;
-                    break;
-                case CURLE_COULDNT_CONNECT:
-                default:
-                    logger->log("curl error %d: %s host: %s",
-                                res, d->mError, d->mUrl.c_str());
-                    break;
-                }
+                fclose(file);
+                ::remove(outFilename.c_str());
+            }
 
-                if (d->mOptions.cancel)
-                {
-                    break;
-                }
+            break;
+        }
 
-                d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
+        if (res != CURLE_OK)
+        {
+            logger->log("curl error %d: %s host: %s",
+                        res, d->mError, d->mUrl.c_str());
 
-                if (!d->mOptions.memoryWrite)
-                {
-                    fclose(file);
-                    ::remove(outFilename.c_str());
-                }
-                attempts++;
-                continue;
+            if (file)
+            {
+                fclose(file);
+                ::remove(outFilename.c_str());
             }
 
-            curl_easy_cleanup(d->mCurl);
+            break;
+        }
 
-            if (!d->mOptions.memoryWrite)
+        if (!d->mOptions.memoryWrite)
+        {
+            // Don't check resources.xml checksum
+            if (d->mAdler)
             {
-                // Don't check resources.xml checksum
-                if (d->mAdler)
-                {
-                    unsigned long adler = fadler32(file);
+                unsigned long adler = fadler32(file);
 
-                    if (d->mAdler != adler)
-                    {
+                if (d->mAdler != adler)
+                {
+                    if (file)
                         fclose(file);
 
-                        // Remove the corrupted file
-                        ::remove(d->mFileName.c_str());
-                        logger->log("Checksum for file %s failed: (%lx/%lx)",
-                            d->mFileName.c_str(),
-                            adler, *d->mAdler);
-                        attempts++;
-                        continue; // Bail out here to avoid the renaming
-                    }
+                    // Remove the corrupted file
+                    ::remove(outFilename.c_str());
+                    logger->log("Checksum for file %s failed: (%lx/%lx)",
+                        d->mFileName.c_str(),
+                        adler, *d->mAdler);
+
+                    continue; // Bail out here to avoid the renaming
                 }
+            }
+
+            if (file)
                 fclose(file);
 
-                // Any existing file with this name is deleted first, otherwise
-                // the rename will fail on Windows.
-                ::remove(d->mFileName.c_str());
-                ::rename(outFilename.c_str(), d->mFileName.c_str());
+            // Any existing file with this name is deleted first, otherwise
+            // the rename will fail on Windows.
+            ::remove(d->mFileName.c_str());
+            ::rename(outFilename.c_str(), d->mFileName.c_str());
 
-                // Check if we can open it and no errors were encountered
-                // during renaming
-                file = fopen(d->mFileName.c_str(), "rb");
-                if (file)
-                {
-                    fclose(file);
-                    complete = true;
-                }
-            }
-            else
+            // Check if we can open it and no errors were encountered
+            // during renaming
+            file = fopen(d->mFileName.c_str(), "rb");
+            if (file)
             {
-                // It's stored in memory, we're done
+                fclose(file);
+                file = nullptr;
                 complete = true;
             }
         }
-        if (d->mOptions.cancel)
+        else
         {
-            d->mThread = nullptr;
-            return 0;
+            // It's stored in memory, we're done
+            complete = true;
         }
-        attempts++;
-    }
 
-    if (d->mOptions.cancel)
-    {
-        // Nothing to do...
+        if (file)
+            fclose(file);
     }
-    else if (!complete || attempts >= 3)
-    {
-        d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
-    }
-    else
+
+    if (!d->mOptions.cancel)
     {
-        d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_COMPLETE, 0, 0);
+        if (complete)
+            d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_COMPLETE, 0, 0);
+        else
+            d->mUpdateFunction(d->mPtr, DOWNLOAD_STATUS_ERROR, 0, 0);
     }
 
-    d->mThread = nullptr;
     return 0;
 }
 
diff --git a/src/net/download.h b/src/net/download.h
index b0f79e79..be2e374f 100644
--- a/src/net/download.h
+++ b/src/net/download.h
@@ -18,7 +18,6 @@
  *  along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <cstdlib> // pulls in int64_t
 #include <cstdio>
 #include <string>
 #include <optional>
@@ -33,28 +32,31 @@ enum DownloadStatus
     DOWNLOAD_STATUS_THREAD_ERROR = -2,
     DOWNLOAD_STATUS_ERROR = -1,
     DOWNLOAD_STATUS_STARTING = 0,
-    DOWNLOAD_STATUS_IDLE,
+    DOWNLOAD_STATUS_IN_PROGRESS,
     DOWNLOAD_STATUS_COMPLETE
 };
 
-using DownloadUpdate = int (*)(void *, DownloadStatus, size_t, size_t);
-
-// Matches what CURL expects
-using WriteFunction = size_t (*)(void *, size_t, size_t, void *);
-
 struct SDL_Thread;
-using CURL = void;
-struct curl_slist;
 
 namespace Net {
+
 class Download
 {
     public:
-        Download(void *ptr, const std::string &url, DownloadUpdate updateFunction);
+        /**
+         * Callback function for download updates.
+         *
+         * @param ptr       Pointer passed to Download constructor
+         * @param status    Current download status
+         * @param dltotal   Total number of bytes to download
+         * @param dlnow     Number of bytes downloaded so far
+         */
+        using DownloadUpdate = int (*)(void *, DownloadStatus, size_t, size_t);
 
+        Download(void *ptr, const std::string &url, DownloadUpdate updateFunction);
         ~Download();
 
-        void addHeader(const std::string &header);
+        void addHeader(const char *header);
 
         /**
          * Convience method for adding no-cache headers.
@@ -64,7 +66,7 @@ class Download
         void setFile(const std::string &filename,
                      std::optional<unsigned long> adler32 = {});
 
-        void setWriteFunction(WriteFunction write);
+        void setWriteFunction(curl_write_callback write);
 
         /**
          * Starts the download thread.
@@ -76,7 +78,7 @@ class Download
 
         /**
          * Cancels the download. Returns immediately, the cancelled status will
-         * be noted in the next avialable update call.
+         * be noted in the next available update call.
          */
         void cancel();
 
@@ -96,11 +98,10 @@ class Download
             unsigned memoryWrite: 1;
         } mOptions;
         std::string mFileName;
-        WriteFunction mWriteFunction = nullptr;
+        curl_write_callback mWriteFunction = nullptr;
         std::optional<unsigned long> mAdler;
         DownloadUpdate mUpdateFunction;
         SDL_Thread *mThread = nullptr;
-        CURL *mCurl = nullptr;
         curl_slist *mHeaders = nullptr;
         char *mError;
 };