From 4a890eae38f86d5f689983e84da951a1b42c17d8 Mon Sep 17 00:00:00 2001 From: Jocelyn Turcotte Date: Thu, 2 Apr 2015 15:16:33 +0200 Subject: [PATCH] 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. --- src/libsync/discoveryphase.cpp | 29 +++++------------ src/libsync/discoveryphase.h | 58 +++++++++++++++++----------------- 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)), - this, SLOT(singleDirectoryJobResultSlot(QLinkedList))); + QObject::connect(_singleDirJob, SIGNAL(finishedWithResult(const QList &)), + this, SLOT(singleDirectoryJobResultSlot(const QList &))); 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 result) +void DiscoveryMainThread::singleDirectoryJobResultSlot(const QList & result) { if (!_currentDiscoveryDirectoryResult) { return; // possibly aborted @@ -372,11 +363,9 @@ void DiscoveryMainThread::singleDirectoryJobResultSlot(QLinkedListpath; - _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)), this); + _singleDirJob->disconnect(SIGNAL(finishedWithResult(const QList &)), 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(userdata); if (discoveryJob) { DiscoveryDirectoryResult *directoryResult = static_cast(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::iterator iterator; - QLinkedList list; + QList 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); + void finishedWithResult(const QList &); void finishedWithError(int csyncErrnoCode, QString msg); private slots: void directoryListingIteratedSlot(QString,QMap); void lsJobFinishedWithoutErrorSlot(); void lsJobFinishedWithErrorSlot(QNetworkReply*); private: - QLinkedList _results; + QList _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 > _directoryContents; - - QPointer _discoveryJob; QPointer _singleDirJob; QString _pathPrefix; @@ -87,22 +106,6 @@ class DiscoveryMainThread : public QObject { 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 & list, _directoryContents) { - foreach (csync_vio_file_stat_t* stat, list) { - csync_vio_file_stat_destroy(stat); - } - } } void abort(); @@ -110,10 +113,9 @@ public: public slots: // From DiscoveryJob: void doOpendirSlot(QString url, DiscoveryDirectoryResult* ); - void doClosedirSlot(QString path); // From Job: - void singleDirectoryJobResultSlot(QLinkedList); + void singleDirectoryJobResultSlot(const QList &); 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); }; } -- 2.30.2