Make const functions in QDir re-entrant by protecting mutable variables

QDir caches data in mutable variables. Because of no protection, two
threads calling for instance QDir::count on two independent but shared
copies of a QDir object caused a data race.

Running the tst_QDir_tree and tst_QDir_10000 three times with and
without this change show no change beyond the difference between
each result. For instance tst_QDir_tree::thousandFiles:"src" took
36/35/37 ms before the change and 35/35/35 ms after.

This makes sense because the time to handle mutexes is very little
compared the time to make file operations.

Fixes: QTBUG-105753
Change-Id: I6886f84521410f88cce2b25f0cce8cfc3fea1a0b
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
This commit is contained in:
Øystein Heskestad 2022-12-15 16:01:40 +01:00
parent a5387f3c16
commit fc3942114d
2 changed files with 85 additions and 60 deletions

View File

@ -22,7 +22,8 @@
#include <qstringbuilder.h> #include <qstringbuilder.h>
#ifndef QT_BOOTSTRAPPED #ifndef QT_BOOTSTRAPPED
# include "qreadwritelock.h" # include "qreadwritelock.h"
# include "qmutex.h"
#endif #endif
#include <algorithm> #include <algorithm>
@ -77,12 +78,9 @@ static qsizetype rootLength(QStringView name, bool allowUncPaths)
} }
//************* QDirPrivate //************* QDirPrivate
QDirPrivate::QDirPrivate(const QString &path, const QStringList &nameFilters_, QDir::SortFlags sort_, QDir::Filters filters_) QDirPrivate::QDirPrivate(const QString &path, const QStringList &nameFilters_,
: QSharedData() QDir::SortFlags sort_, QDir::Filters filters_)
, fileListsInitialized(false) : QSharedData(), nameFilters(nameFilters_), sort(sort_), filters(filters_)
, nameFilters(nameFilters_)
, sort(sort_)
, filters(filters_)
{ {
setPath(path.isEmpty() ? QString::fromLatin1(".") : path); setPath(path.isEmpty() ? QString::fromLatin1(".") : path);
@ -93,22 +91,31 @@ QDirPrivate::QDirPrivate(const QString &path, const QStringList &nameFilters_, Q
} }
QDirPrivate::QDirPrivate(const QDirPrivate &copy) QDirPrivate::QDirPrivate(const QDirPrivate &copy)
: QSharedData(copy) : QSharedData(copy),
, fileListsInitialized(false) // mutex is not copied
, nameFilters(copy.nameFilters) nameFilters(copy.nameFilters),
, sort(copy.sort) sort(copy.sort),
, filters(copy.filters) filters(copy.filters),
, dirEntry(copy.dirEntry) // fileEngine is not copied
, metaData(copy.metaData) dirEntry(copy.dirEntry)
{ {
QMutexLocker locker(&copy.fileCache.mutex);
fileCache.fileListsInitialized = copy.fileCache.fileListsInitialized;
fileCache.files = copy.fileCache.files;
fileCache.fileInfos = copy.fileCache.fileInfos;
fileCache.absoluteDirEntry = copy.fileCache.absoluteDirEntry;
fileCache.metaData = copy.fileCache.metaData;
} }
bool QDirPrivate::exists() const bool QDirPrivate::exists() const
{ {
if (!fileEngine) { if (!fileEngine) {
QFileSystemEngine::fillMetaData(dirEntry, metaData, QMutexLocker locker(&fileCache.mutex);
QFileSystemMetaData::ExistsAttribute | QFileSystemMetaData::DirectoryType); // always stat QFileSystemEngine::fillMetaData(
return metaData.exists() && metaData.isDirectory(); dirEntry, fileCache.metaData,
QFileSystemMetaData::ExistsAttribute
| QFileSystemMetaData::DirectoryType); // always stat
return fileCache.metaData.exists() && fileCache.metaData.isDirectory();
} }
const QAbstractFileEngine::FileFlags info = const QAbstractFileEngine::FileFlags info =
fileEngine->fileFlags(QAbstractFileEngine::DirectoryType fileEngine->fileFlags(QAbstractFileEngine::DirectoryType
@ -151,30 +158,35 @@ inline void QDirPrivate::setPath(const QString &path)
) { ) {
p.truncate(p.size() - 1); p.truncate(p.size() - 1);
} }
dirEntry = QFileSystemEntry(p, QFileSystemEntry::FromInternalPath()); dirEntry = QFileSystemEntry(p, QFileSystemEntry::FromInternalPath());
clearCache(IncludingMetaData); clearCache(IncludingMetaData);
absoluteDirEntry = QFileSystemEntry(); fileCache.absoluteDirEntry = QFileSystemEntry();
} }
inline void QDirPrivate::resolveAbsoluteEntry() const inline QString QDirPrivate::resolveAbsoluteEntry() const
{ {
if (!absoluteDirEntry.isEmpty() || dirEntry.isEmpty()) QMutexLocker locker(&fileCache.mutex);
return; if (!fileCache.absoluteDirEntry.isEmpty())
return fileCache.absoluteDirEntry.filePath();
if (dirEntry.isEmpty())
return dirEntry.filePath();
QString absoluteName; QString absoluteName;
if (!fileEngine) { if (!fileEngine) {
if (!dirEntry.isRelative() && dirEntry.isClean()) { if (!dirEntry.isRelative() && dirEntry.isClean()) {
absoluteDirEntry = dirEntry; fileCache.absoluteDirEntry = dirEntry;
return; return dirEntry.filePath();
} }
absoluteName = QFileSystemEngine::absoluteName(dirEntry).filePath(); absoluteName = QFileSystemEngine::absoluteName(dirEntry).filePath();
} else { } else {
absoluteName = fileEngine->fileName(QAbstractFileEngine::AbsoluteName); absoluteName = fileEngine->fileName(QAbstractFileEngine::AbsoluteName);
} }
auto absoluteFileSystemEntry =
absoluteDirEntry = QFileSystemEntry(QDir::cleanPath(absoluteName), QFileSystemEntry::FromInternalPath()); QFileSystemEntry(QDir::cleanPath(absoluteName), QFileSystemEntry::FromInternalPath());
fileCache.absoluteDirEntry = absoluteFileSystemEntry;
return absoluteFileSystemEntry.filePath();
} }
/* For sorting */ /* For sorting */
@ -297,24 +309,28 @@ inline void QDirPrivate::sortFileList(QDir::SortFlags sort, const QFileInfoList
} }
inline void QDirPrivate::initFileLists(const QDir &dir) const inline void QDirPrivate::initFileLists(const QDir &dir) const
{ {
if (!fileListsInitialized) { QMutexLocker locker(&fileCache.mutex);
if (!fileCache.fileListsInitialized) {
QFileInfoList l; QFileInfoList l;
QDirIterator it(dir); QDirIterator it(dir);
while (it.hasNext()) while (it.hasNext())
l.append(it.nextFileInfo()); l.append(it.nextFileInfo());
sortFileList(sort, l, &files, &fileInfos);
fileListsInitialized = true; sortFileList(sort, l, &fileCache.files, &fileCache.fileInfos);
fileCache.fileListsInitialized = true;
} }
} }
inline void QDirPrivate::clearCache(MetaDataClearing mode) inline void QDirPrivate::clearCache(MetaDataClearing mode)
{ {
QMutexLocker locker(&fileCache.mutex);
if (mode == IncludingMetaData) if (mode == IncludingMetaData)
metaData.clear(); fileCache.metaData.clear();
fileListsInitialized = false; fileCache.fileListsInitialized = false;
files.clear(); fileCache.files.clear();
fileInfos.clear(); fileCache.fileInfos.clear();
fileEngine.reset(QFileSystemEngine::resolveEntryAndCreateLegacyEngine(dirEntry, metaData)); fileEngine.reset(
QFileSystemEngine::resolveEntryAndCreateLegacyEngine(dirEntry, fileCache.metaData));
} }
/*! /*!
@ -612,8 +628,7 @@ QString QDir::path() const
QString QDir::absolutePath() const QString QDir::absolutePath() const
{ {
Q_D(const QDir); Q_D(const QDir);
d->resolveAbsoluteEntry(); return d->resolveAbsoluteEntry();
return d->absoluteDirEntry.filePath();
} }
/*! /*!
@ -636,7 +651,9 @@ QString QDir::canonicalPath() const
{ {
Q_D(const QDir); Q_D(const QDir);
if (!d->fileEngine) { if (!d->fileEngine) {
QFileSystemEntry answer = QFileSystemEngine::canonicalName(d->dirEntry, d->metaData); QMutexLocker locker(&d->fileCache.mutex);
QFileSystemEntry answer =
QFileSystemEngine::canonicalName(d->dirEntry, d->fileCache.metaData);
return answer.filePath(); return answer.filePath();
} }
return d->fileEngine->fileName(QAbstractFileEngine::CanonicalName); return d->fileEngine->fileName(QAbstractFileEngine::CanonicalName);
@ -753,8 +770,7 @@ QString QDir::absoluteFilePath(const QString &fileName) const
return fileName; return fileName;
Q_D(const QDir); Q_D(const QDir);
d->resolveAbsoluteEntry(); QString absoluteDirPath = d->resolveAbsoluteEntry();
const QString absoluteDirPath = d->absoluteDirEntry.filePath();
if (fileName.isEmpty()) if (fileName.isEmpty())
return absoluteDirPath; return absoluteDirPath;
#ifdef Q_OS_WIN #ifdef Q_OS_WIN
@ -1262,7 +1278,7 @@ qsizetype QDir::count(QT6_IMPL_NEW_OVERLOAD) const
{ {
Q_D(const QDir); Q_D(const QDir);
d->initFileLists(*this); d->initFileLists(*this);
return d->files.size(); return d->fileCache.files.size();
} }
/*! /*!
@ -1278,7 +1294,7 @@ QString QDir::operator[](qsizetype pos) const
{ {
Q_D(const QDir); Q_D(const QDir);
d->initFileLists(*this); d->initFileLists(*this);
return d->files[pos]; return d->fileCache.files[pos];
} }
/*! /*!
@ -1357,7 +1373,7 @@ QStringList QDir::entryList(const QStringList &nameFilters, Filters filters,
if (filters == d->filters && sort == d->sort && nameFilters == d->nameFilters) { if (filters == d->filters && sort == d->sort && nameFilters == d->nameFilters) {
d->initFileLists(*this); d->initFileLists(*this);
return d->files; return d->fileCache.files;
} }
QFileInfoList l; QFileInfoList l;
@ -1397,7 +1413,7 @@ QFileInfoList QDir::entryInfoList(const QStringList &nameFilters, Filters filter
if (filters == d->filters && sort == d->sort && nameFilters == d->nameFilters) { if (filters == d->filters && sort == d->sort && nameFilters == d->nameFilters) {
d->initFileLists(*this); d->initFileLists(*this);
return d->fileInfos; return d->fileCache.fileInfos;
} }
QFileInfoList l; QFileInfoList l;
@ -1612,10 +1628,12 @@ bool QDir::isReadable() const
Q_D(const QDir); Q_D(const QDir);
if (!d->fileEngine) { if (!d->fileEngine) {
if (!d->metaData.hasFlags(QFileSystemMetaData::UserReadPermission)) QMutexLocker locker(&d->fileCache.mutex);
QFileSystemEngine::fillMetaData(d->dirEntry, d->metaData, QFileSystemMetaData::UserReadPermission); if (!d->fileCache.metaData.hasFlags(QFileSystemMetaData::UserReadPermission)) {
QFileSystemEngine::fillMetaData(d->dirEntry, d->fileCache.metaData,
return d->metaData.permissions().testAnyFlag(QFile::ReadUser); QFileSystemMetaData::UserReadPermission);
}
return d->fileCache.metaData.permissions().testAnyFlag(QFile::ReadUser);
} }
const QAbstractFileEngine::FileFlags info = const QAbstractFileEngine::FileFlags info =
@ -1722,9 +1740,9 @@ bool QDir::makeAbsolute()
dir.reset(new QDirPrivate(*d_ptr.constData())); dir.reset(new QDirPrivate(*d_ptr.constData()));
dir->setPath(absolutePath); dir->setPath(absolutePath);
} else { // native FS } else { // native FS
d->resolveAbsoluteEntry(); QString absoluteFilePath = d->resolveAbsoluteEntry();
dir.reset(new QDirPrivate(*d_ptr.constData())); dir.reset(new QDirPrivate(*d_ptr.constData()));
dir->setPath(d->absoluteDirEntry.filePath()); dir->setPath(absoluteFilePath);
} }
d_ptr = dir.release(); // actually detach d_ptr = dir.release(); // actually detach
return true; return true;
@ -1775,9 +1793,9 @@ bool QDir::operator==(const QDir &dir) const
if (dir.exists()) if (dir.exists())
return false; //can't be equal if only one exists return false; //can't be equal if only one exists
// Neither exists, compare absolute paths rather than canonical (which would be empty strings) // Neither exists, compare absolute paths rather than canonical (which would be empty strings)
d->resolveAbsoluteEntry(); QString thisFilePath = d->resolveAbsoluteEntry();
other->resolveAbsoluteEntry(); QString otherFilePath = other->resolveAbsoluteEntry();
return d->absoluteDirEntry.filePath().compare(other->absoluteDirEntry.filePath(), sensitive) == 0; return thisFilePath.compare(otherFilePath, sensitive) == 0;
} }
} }
return false; return false;

View File

@ -18,6 +18,8 @@
#include "qfilesystementry_p.h" #include "qfilesystementry_p.h"
#include "qfilesystemmetadata_p.h" #include "qfilesystemmetadata_p.h"
#include <QtCore/qmutex.h>
#include <memory> #include <memory>
QT_BEGIN_NAMESPACE QT_BEGIN_NAMESPACE
@ -37,7 +39,7 @@ public:
QDir::SortFlags sort_ = QDir::SortFlags(QDir::Name | QDir::IgnoreCase), QDir::SortFlags sort_ = QDir::SortFlags(QDir::Name | QDir::IgnoreCase),
QDir::Filters filters_ = QDir::AllEntries); QDir::Filters filters_ = QDir::AllEntries);
explicit QDirPrivate(const QDirPrivate &copy); explicit QDirPrivate(const QDirPrivate &copy); // Copies everything except mutex and fileEngine
bool exists() const; bool exists() const;
@ -54,11 +56,7 @@ public:
enum MetaDataClearing { KeepMetaData, IncludingMetaData }; enum MetaDataClearing { KeepMetaData, IncludingMetaData };
void clearCache(MetaDataClearing mode); void clearCache(MetaDataClearing mode);
void resolveAbsoluteEntry() const; QString resolveAbsoluteEntry() const;
mutable bool fileListsInitialized;
mutable QStringList files;
mutable QFileInfoList fileInfos;
QStringList nameFilters; QStringList nameFilters;
QDir::SortFlags sort; QDir::SortFlags sort;
@ -67,8 +65,17 @@ public:
std::unique_ptr<QAbstractFileEngine> fileEngine; std::unique_ptr<QAbstractFileEngine> fileEngine;
QFileSystemEntry dirEntry; QFileSystemEntry dirEntry;
mutable QFileSystemEntry absoluteDirEntry;
mutable QFileSystemMetaData metaData; struct FileCache
{
QMutex mutex;
QStringList files;
QFileInfoList fileInfos;
bool fileListsInitialized = false;
QFileSystemEntry absoluteDirEntry;
QFileSystemMetaData metaData;
};
mutable FileCache fileCache;
}; };
Q_DECLARE_OPERATORS_FOR_FLAGS(QDirPrivate::PathNormalizations) Q_DECLARE_OPERATORS_FOR_FLAGS(QDirPrivate::PathNormalizations)