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.

4 files changed, 124 insertions, 15 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index 76773d49..75434d8e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -229,6 +229,7 @@ aethyra_SOURCES = gui/widgets/dropdown.cpp \
 	      utils/strprintf.h \
 	      utils/tostring.h \
 	      utils/trim.h \
+	      utils/mutex.h \
 	      utils/xml.cpp \
 	      utils/xml.h \
 	      animatedsprite.cpp \
diff --git a/src/gui/updatewindow.cpp b/src/gui/updatewindow.cpp
index 64f02f54..9d9f5616 100644
--- a/src/gui/updatewindow.cpp
+++ b/src/gui/updatewindow.cpp
@@ -46,7 +46,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);
@@ -145,15 +145,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());
@@ -168,8 +162,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()
@@ -426,6 +421,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:
@@ -447,7 +453,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 4c302e85..9cd3405e 100644
--- a/src/gui/updatewindow.h
+++ b/src/gui/updatewindow.h
@@ -31,12 +31,13 @@
 
 #include "../guichanfwd.h"
 
+#include "../utils/mutex.h"
+
 class BrowserBox;
 class Button;
 class ProgressBar;
 class ScrollArea;
 
-struct SDL_mutex;
 struct SDL_Thread;
 
 /**
@@ -89,7 +90,7 @@ class UpdaterWindow : public Window, public gcn::ActionListener
 
     int updateState;
 
- protected:
+private:
     void download();
 
     /**
@@ -134,6 +135,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;
 
@@ -165,7 +172,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. */
 };
 
diff --git a/src/utils/mutex.h b/src/utils/mutex.h
new file mode 100644
index 00000000..6c35987c
--- /dev/null
+++ b/src/utils/mutex.h
@@ -0,0 +1,94 @@
+/*
+ *  The Mana World
+ *  Copyright 2008 The Mana World Development Team
+ *
+ *  This file is part of The Mana World.
+ *
+ *  The Mana World is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  any later version.
+ *
+ *  The Mana World is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with The Mana World; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef TMW_MUTEX_H
+#define TMW_MUTEX_H
+
+#include <SDL_thread.h>
+
+#include "../log.h"
+
+/**
+ * A mutex provides mutual exclusion of access to certain data that is
+ * accessed by multiple threads.
+ */
+class Mutex
+{
+public:
+    Mutex();
+    ~Mutex();
+
+    void lock();
+    void unlock();
+
+private:
+    SDL_mutex *mMutex;
+};
+
+/**
+ * A convenience class for locking a mutex.
+ */
+class MutexLocker
+{
+public:
+    MutexLocker(Mutex mutex);
+    ~MutexLocker();
+
+private:
+    Mutex mMutex;
+};
+
+
+inline Mutex::Mutex()
+{
+    mMutex = SDL_CreateMutex();
+}
+
+inline Mutex::~Mutex()
+{
+    SDL_DestroyMutex(mMutex);
+}
+
+inline void Mutex::lock()
+{
+    if (SDL_mutexP(mMutex) == -1)
+        logger->log("Mutex locking failed: %s", SDL_GetError());
+}
+
+inline void Mutex::unlock()
+{
+    if (SDL_mutexV(mMutex) == -1)
+        logger->log("Mutex unlocking failed: %s", SDL_GetError());
+}
+
+
+inline MutexLocker::MutexLocker(Mutex mutex):
+    mMutex(mutex)
+{
+    mMutex.lock();
+}
+
+inline MutexLocker::~MutexLocker()
+{
+    mMutex.unlock();
+}
+
+#endif // TMW_MUTEX_H