Skip to content

Commit

Permalink
SyncEngine: Fix a crash caused by an invalid DiscoveryDirectoryResult…
Browse files Browse the repository at this point in the history 
…::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.
  • Loading branch information
@jturcotte
jturcotte committed Apr 8, 2015
1 parent 760e11b commit 4a890ea
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 50 deletions.
29 changes: 8 additions & 21 deletions src/libsync/discoveryphase.cpp
View file
Expand Up @@ -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.";
Expand Down Expand Up @@ -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
Expand All @@ -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)),
Expand All @@ -364,19 +355,17 @@ 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
}
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();
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
}
Expand All @@ -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);
}
}

Expand Down
58 changes: 29 additions & 29 deletions src/libsync/discoveryphase.h
View file
Expand Up @@ -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
Expand All @@ -53,14 +77,14 @@ class DiscoverySingleDirectoryJob : public QObject {
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;
Expand All @@ -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;
Expand All @@ -87,33 +106,16 @@ 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<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:
Expand Down Expand Up @@ -175,8 +177,6 @@ class DiscoveryJob : public QObject {

// 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);
};

}

0 comments on commit 4a890ea

Please sign in to comment.