SyncEngine: Fix a crash caused by an invalid DiscoveryDirectoryResult::iterator #3051

The default constructor of the iterator points to NULL, which makes
it != end() but invalid to dereference.

Use an integer index instead to keep 0 as a valid default value that
can always correctly be checked against size().

Also make sure that no data is shared between threads by making the
csync_vio_file_stat_t copyable and passing it as const.

2 files changed, 37 insertions, 50 deletions

diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp
index f68a6974b..b8c580a12 100644
--- a/src/libsync/discoveryphase.cpp
+++ b/src/libsync/discoveryphase.cpp
@@ -268,7 +268,7 @@ void DiscoverySingleDirectoryJob::directoryListingIteratedSlot(QString file,QMap
         }
 
 
-        csync_vio_file_stat_t *file_stat = propertyMapToFileStat(map);
+        FileStatPointer file_stat(propertyMapToFileStat(map));
         file_stat->name = strdup(file.toUtf8());
         if (!file_stat->etag || strlen(file_stat->etag) == 0) {
             qDebug() << "WARNING: etag of" << file_stat->name << "is" << file_stat->etag << " This must not happen.";
@@ -319,15 +319,6 @@ void DiscoveryMainThread::setupHooks(DiscoveryJob *discoveryJob, const QString &
     connect(discoveryJob, SIGNAL(doOpendirSignal(QString,DiscoveryDirectoryResult*)),
             this, SLOT(doOpendirSlot(QString,DiscoveryDirectoryResult*)),
             Qt::QueuedConnection);
-    connect(discoveryJob, SIGNAL(doClosedirSignal(QString)),
-            this, SLOT(doClosedirSlot(QString)),
-            Qt::QueuedConnection);
-}
-
-void DiscoveryMainThread::doClosedirSlot(QString path)
-{
-    //qDebug() << Q_FUNC_INFO << "Invalidating" << path;
-    deleteCacheEntry(path);
 }
 
 // Coming from owncloud_opendir -> DiscoveryJob::vio_opendir_hook -> doOpendirSignal
@@ -352,8 +343,8 @@ void DiscoveryMainThread::doOpendirSlot(QString subPath, DiscoveryDirectoryResul
 
     // Schedule the DiscoverySingleDirectoryJob
     _singleDirJob = new DiscoverySingleDirectoryJob(_account, fullPath, this);
-    QObject::connect(_singleDirJob, SIGNAL(finishedWithResult(QLinkedList<csync_vio_file_stat_t *>)),
-                     this, SLOT(singleDirectoryJobResultSlot(QLinkedList<csync_vio_file_stat_t*>)));
+    QObject::connect(_singleDirJob, SIGNAL(finishedWithResult(const QList<FileStatPointer> &)),
+                     this, SLOT(singleDirectoryJobResultSlot(const QList<FileStatPointer> &)));
     QObject::connect(_singleDirJob, SIGNAL(finishedWithError(int,QString)),
                      this, SLOT(singleDirectoryJobFinishedWithErrorSlot(int,QString)));
     QObject::connect(_singleDirJob, SIGNAL(firstDirectoryPermissions(QString)),
@@ -364,7 +355,7 @@ void DiscoveryMainThread::doOpendirSlot(QString subPath, DiscoveryDirectoryResul
 }
 
 
-void DiscoveryMainThread::singleDirectoryJobResultSlot(QLinkedList<csync_vio_file_stat_t *> result)
+void DiscoveryMainThread::singleDirectoryJobResultSlot(const QList<FileStatPointer> & result)
 {
     if (!_currentDiscoveryDirectoryResult) {
         return; // possibly aborted
@@ -372,11 +363,9 @@ void DiscoveryMainThread::singleDirectoryJobResultSlot(QLinkedList<csync_vio_fil
     qDebug() << Q_FUNC_INFO << "Have" << result.count() << "results for " << _currentDiscoveryDirectoryResult->path;
 
 
-    _directoryContents.insert(_currentDiscoveryDirectoryResult->path, result);
-
     _currentDiscoveryDirectoryResult->list = result;
     _currentDiscoveryDirectoryResult->code = 0;
-    _currentDiscoveryDirectoryResult->iterator = _currentDiscoveryDirectoryResult->list.begin();
+    _currentDiscoveryDirectoryResult->listIndex = 0;
      _currentDiscoveryDirectoryResult = 0; // the sync thread owns it now
 
     _discoveryJob->_vioMutex.lock();
@@ -414,7 +403,7 @@ void DiscoveryMainThread::abort() {
     if (_singleDirJob) {
         _singleDirJob->disconnect(SIGNAL(finishedWithError(int,QString)), this);
         _singleDirJob->disconnect(SIGNAL(firstDirectoryPermissions(QString)), this);
-        _singleDirJob->disconnect(SIGNAL(finishedWithResult(QLinkedList<csync_vio_file_stat_t*>)), this);
+        _singleDirJob->disconnect(SIGNAL(finishedWithResult(const QList<FileStatPointer> &)), this);
         _singleDirJob->abort();
     }
     if (_currentDiscoveryDirectoryResult) {
@@ -469,9 +458,8 @@ csync_vio_file_stat_t* DiscoveryJob::remote_vio_readdir_hook (csync_vio_handle_t
     DiscoveryJob *discoveryJob = static_cast<DiscoveryJob*>(userdata);
     if (discoveryJob) {
         DiscoveryDirectoryResult *directoryResult = static_cast<DiscoveryDirectoryResult*>(dhandle);
-        if (directoryResult->iterator != directoryResult->list.end()) {
-            csync_vio_file_stat_t *file_stat = *(directoryResult->iterator);
-            directoryResult->iterator++;
+        if (directoryResult->listIndex < directoryResult->list.size()) {
+            csync_vio_file_stat_t *file_stat = directoryResult->list.at(directoryResult->listIndex++).data();
             // Make a copy, csync_update will delete the copy
             return csync_vio_file_stat_copy(file_stat);
         }
@@ -487,7 +475,6 @@ void DiscoveryJob::remote_vio_closedir_hook (csync_vio_handle_t *dhandle,  void
         QString path = directoryResult->path;
         qDebug() << Q_FUNC_INFO << discoveryJob << path;
         delete directoryResult; // just deletes the struct and the iterator, the data itself is owned by the SyncEngine/DiscoveryMainThread
-        emit discoveryJob->doClosedirSignal(path);
     }
 }
 
diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
index b1989d0a0..5ceb477e3 100644
--- a/src/libsync/discoveryphase.h
+++ b/src/libsync/discoveryphase.h
@@ -34,12 +34,36 @@ class Account;
  * if the files are new, or changed.
  */
 
+class FileStatPointer {
+public:
+    FileStatPointer(csync_vio_file_stat_t *stat)
+        : _stat(stat)
+    { }
+    FileStatPointer(const FileStatPointer &other)
+        : _stat(csync_vio_file_stat_copy(other._stat))
+    { }
+    ~FileStatPointer() {
+        csync_vio_file_stat_destroy(_stat);
+    }
+    FileStatPointer &operator=(const FileStatPointer &other) {
+        csync_vio_file_stat_destroy(_stat);
+        _stat = csync_vio_file_stat_copy(other._stat);
+        return *this;
+    }
+    inline csync_vio_file_stat_t *data() const { return _stat; }
+    inline csync_vio_file_stat_t *operator->() const { return _stat; }
+
+private:
+    csync_vio_file_stat_t *_stat;
+};
+
 struct DiscoveryDirectoryResult {
     QString path;
     QString msg;
     int code;
-    QLinkedList<csync_vio_file_stat_t*>::iterator iterator;
-    QLinkedList<csync_vio_file_stat_t *> list;
+    QList<FileStatPointer> list;
+    int listIndex;
+    DiscoveryDirectoryResult() : code(0), listIndex(0) { }
 };
 
 // Run in the main thread, reporting to the DiscoveryJobMainThread object
@@ -53,14 +77,14 @@ public:
 signals:
     void firstDirectoryPermissions(const QString &);
     void etagConcatenation(const QString &);
-    void finishedWithResult(QLinkedList<csync_vio_file_stat_t*>);
+    void finishedWithResult(const QList<FileStatPointer> &);
     void finishedWithError(int csyncErrnoCode, QString msg);
 private slots:
     void directoryListingIteratedSlot(QString,QMap<QString,QString>);
     void lsJobFinishedWithoutErrorSlot();
     void lsJobFinishedWithErrorSlot(QNetworkReply*);
 private:
-    QLinkedList<csync_vio_file_stat_t*> _results;
+    QList<FileStatPointer> _results;
     QString _subPath;
     QString _etagConcatenation;
     AccountPtr _account;
@@ -73,11 +97,6 @@ class DiscoveryJob;
 class DiscoveryMainThread : public QObject {
     Q_OBJECT
 
-    // For non-recursive and recursive
-    // If it is not in this map it needs to be requested
-    QMap<QString, QLinkedList<csync_vio_file_stat_t*> > _directoryContents;
-
-
     QPointer<DiscoveryJob> _discoveryJob;
     QPointer<DiscoverySingleDirectoryJob> _singleDirJob;
     QString _pathPrefix;
@@ -88,32 +107,15 @@ public:
     DiscoveryMainThread(AccountPtr account) : QObject(), _account(account), _currentDiscoveryDirectoryResult(0) {
 
     }
-    void deleteCacheEntry(QString path) {
-        //qDebug() << path << _directoryContents.value(path).count();
-        foreach (csync_vio_file_stat_t* stat, _directoryContents.value(path)) {
-            csync_vio_file_stat_destroy(stat);
-        }
-        _directoryContents.remove(path);
-    }
-
-    ~DiscoveryMainThread() {
-        // Delete the _contents_ of the list-map explicitly:
-        foreach (const QLinkedList<csync_vio_file_stat_t*> & list, _directoryContents) {
-            foreach (csync_vio_file_stat_t* stat, list) {
-                csync_vio_file_stat_destroy(stat);
-            }
-        }
-    }
     void abort();
 
 
 public slots:
     // From DiscoveryJob:
     void doOpendirSlot(QString url, DiscoveryDirectoryResult* );
-    void doClosedirSlot(QString path);
 
     // From Job:
-    void singleDirectoryJobResultSlot(QLinkedList<csync_vio_file_stat_t*>);
+    void singleDirectoryJobResultSlot(const QList<FileStatPointer> &);
     void singleDirectoryJobFinishedWithErrorSlot(int csyncErrnoCode, QString msg);
     void singleDirectoryJobFirstDirectoryPermissionsSlot(QString);
 signals:
@@ -175,8 +177,6 @@ signals:
 
     // After the discovery job has been woken up again (_vioWaitCondition)
     void doOpendirSignal(QString url, DiscoveryDirectoryResult*);
-    // to tell the main thread to invalidate its directory data
-    void doClosedirSignal(QString path);
 };
 
 }