Fix race condition with a std::string access

The downloading thread was writing to a std::string while the main
thread was trying to draw it, for example. Now access to the label
caption is guarded with a mutex.

Should fix crashes while downloading updates.

2 files changed, 29 insertions, 15 deletions

diff --git a/src/gui/updatewindow.cpp b/src/gui/updatewindow.cpp
index ea0f86cd..6083725e 100644
--- a/src/gui/updatewindow.cpp
+++ b/src/gui/updatewindow.cpp
@@ -47,7 +47,7 @@
 /**
  * Calculates the Alder-32 checksum for the given file.
  */
-unsigned long fadler32(FILE *file)
+static unsigned long fadler32(FILE *file)
 {
     // Obtain file size
     fseek(file, 0, SEEK_END);
@@ -146,15 +146,9 @@ UpdaterWindow::UpdaterWindow(const std::string &updateHost,
 UpdaterWindow::~UpdaterWindow()
 {
     if (mThread)
-    {
-         SDL_WaitThread(mThread, NULL);
-         mThread = NULL;
-    }
+        SDL_WaitThread(mThread, NULL);
 
-    if (mMemoryBuffer)
-    {
-        free(mMemoryBuffer);
-    }
+    free(mMemoryBuffer);
 
     // Remove possibly leftover temporary download
     ::remove((mUpdatesDir + "/download.temp").c_str());
@@ -169,8 +163,9 @@ void UpdaterWindow::setProgress(float p)
 
 void UpdaterWindow::setLabel(const std::string &str)
 {
-    mLabel->setCaption(str);
-    mLabel->adjustSize();
+    // Do delayed label text update, since Guichan isn't thread-safe
+    MutexLocker lock(mLabelMutex);
+    mNewLabelCaption = str;
 }
 
 void UpdaterWindow::enable()
@@ -413,6 +408,17 @@ void UpdaterWindow::logic()
     // Update Scroll logic
     mScrollArea->logic();
 
+    // Synchronize label caption when necessary
+    {
+        MutexLocker lock(mLabelMutex);
+
+        if (mLabel->getCaption() != mNewLabelCaption)
+        {
+            mLabel->setCaption(mNewLabelCaption);
+            mLabel->adjustSize();
+        }
+    }
+
     switch (mDownloadStatus)
     {
         case UPDATE_ERROR:
@@ -434,7 +440,8 @@ void UpdaterWindow::logic()
             mBrowserBox->addRow("##1  It is strongly recommended that");
             mBrowserBox->addRow("##1  you try again later");
             mBrowserBox->addRow(mCurlError);
-            mScrollArea->setVerticalScrollAmount(mScrollArea->getVerticalMaxScroll());
+            mScrollArea->setVerticalScrollAmount(
+                    mScrollArea->getVerticalMaxScroll());
             mDownloadStatus = UPDATE_COMPLETE;
             break;
         case UPDATE_NEWS:
diff --git a/src/gui/updatewindow.h b/src/gui/updatewindow.h
index d7e3c4c7..a7dfe2cb 100644
--- a/src/gui/updatewindow.h
+++ b/src/gui/updatewindow.h
@@ -30,12 +30,13 @@
 
 #include "../guichanfwd.h"
 
+#include "../utils/mutex.h"
+
 class BrowserBox;
 class Button;
 class ProgressBar;
 class ScrollArea;
 
-struct SDL_mutex;
 struct SDL_Thread;
 
 /**
@@ -88,7 +89,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener
 
     int updateState;
 
- protected:
+private:
     void download();
 
     /**
@@ -133,6 +134,12 @@ class UpdaterWindow : public Window, public gcn::ActionListener
     /** The file currently downloading. */
     std::string mCurrentFile;
 
+    /** The new label caption to be set in the logic method. */
+    std::string mNewLabelCaption;
+
+    /** The mutex used to guard access to mNewLabelCaption. */
+    Mutex mLabelMutex;
+
     /** The Adler32 checksum of the file currently downloading. */
     unsigned long mCurrentChecksum;
 
@@ -164,7 +171,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener
     Button *mCancelButton;        /**< Button to stop the update process. */
     Button *mPlayButton;          /**< Button to start playing. */
     ProgressBar *mProgressBar;    /**< Update progress bar. */
-    BrowserBox* mBrowserBox;      /**< Box to display news. */
+    BrowserBox *mBrowserBox;      /**< Box to display news. */
     ScrollArea *mScrollArea;      /**< Used to scroll news box. */
 };