QFileSystemEngine: Consistently check for invalid file names

stat() and friends expect a null-terminated C string. There is no way to generate anything useful from a string that has null bytes in the middle. It's important to catch this early, as otherwise, for example, a QDir::exists() on such a path can return true, as the path is silently truncated. Extend the checks for empty file names to windows and add checks for null bytes. Change-Id: Ie9794c3a7c4fd57f9a66bdbbab8b45a08b6f9170 Reviewed-by: Thiago Macieira <[email protected]>
wx984490083 · Oct 24, 2019 · 9ab043b · 9ab043b
1 parent f2edc6c
commit 9ab043b
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 34 deletions.
diff --git a/src/corelib/io/qfilesystemengine_p.h b/src/corelib/io/qfilesystemengine_p.h
@@ -58,6 +58,36 @@
 
 QT_BEGIN_NAMESPACE
 
+#define Q_RETURN_ON_INVALID_FILENAME(message, result) \
+    { \
+        QMessageLogger(QT_MESSAGELOG_FILE, QT_MESSAGELOG_LINE, QT_MESSAGELOG_FUNC).warning(message); \
+        errno = EINVAL; \
+        return (result); \
+    }
+
+inline bool qIsFilenameBroken(const QByteArray &name)
+{
+    return name.contains('\0');
+}
+
+inline bool qIsFilenameBroken(const QString &name)
+{
+    return name.contains(QLatin1Char('\0'));
+}
+
+inline bool qIsFilenameBroken(const QFileSystemEntry &entry)
+{
+    return qIsFilenameBroken(entry.nativeFilePath());
+}
+
+#define Q_CHECK_FILE_NAME(name, result) \
+    do { \
+        if (Q_UNLIKELY((name).isEmpty())) \
+            Q_RETURN_ON_INVALID_FILENAME("Empty filename passed to function", (result)); \
+        if (Q_UNLIKELY(qIsFilenameBroken(name))) \
+            Q_RETURN_ON_INVALID_FILENAME("Broken filename passed to function", (result)); \
+    } while (false)
+
 class QFileSystemEngine
 {
 public:

diff --git a/src/corelib/io/qfilesystemengine_unix.cpp b/src/corelib/io/qfilesystemengine_unix.cpp
@@ -118,13 +118,6 @@ enum {
 #endif
 };
 
-#define emptyFileEntryWarning() emptyFileEntryWarning_(QT_MESSAGELOG_FILE, QT_MESSAGELOG_LINE, QT_MESSAGELOG_FUNC)
-static void emptyFileEntryWarning_(const char *file, int line, const char *function)
-{
-    QMessageLogger(file, line, function).warning("Empty filename passed to function");
-    errno = EINVAL;
-}
-
 #if defined(Q_OS_DARWIN)
 static inline bool hasResourcePropertyFlag(const QFileSystemMetaData &data,
                                            const QFileSystemEntry &entry,
@@ -625,8 +618,7 @@ void QFileSystemMetaData::fillFromDirEnt(const QT_DIRENT &entry)
 //static
 QFileSystemEntry QFileSystemEngine::getLinkTarget(const QFileSystemEntry &link, QFileSystemMetaData &data)
 {
-    if (Q_UNLIKELY(link.isEmpty()))
-        return emptyFileEntryWarning(), link;
+    Q_CHECK_FILE_NAME(link, link);
 
     QByteArray s = qt_readlink(link.nativeFilePath().constData());
     if (s.length() > 0) {
@@ -685,10 +677,7 @@ QFileSystemEntry QFileSystemEngine::getLinkTarget(const QFileSystemEntry &link,
 //static
 QFileSystemEntry QFileSystemEngine::canonicalName(const QFileSystemEntry &entry, QFileSystemMetaData &data)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), entry;
-    if (entry.isRoot())
-        return entry;
+    Q_CHECK_FILE_NAME(entry, entry);
 
 #if !defined(Q_OS_MAC) && !defined(Q_OS_QNX) && !defined(Q_OS_ANDROID) && !defined(Q_OS_HAIKU) && _POSIX_VERSION < 200809L
     // realpath(X,0) is not supported
@@ -738,8 +727,8 @@ QFileSystemEntry QFileSystemEngine::canonicalName(const QFileSystemEntry &entry,
 //static
 QFileSystemEntry QFileSystemEngine::absoluteName(const QFileSystemEntry &entry)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), entry;
+    Q_CHECK_FILE_NAME(entry, entry);
+
     if (entry.isAbsolute() && entry.isClean())
         return entry;
 
@@ -773,8 +762,7 @@ QFileSystemEntry QFileSystemEngine::absoluteName(const QFileSystemEntry &entry)
 //static
 QByteArray QFileSystemEngine::id(const QFileSystemEntry &entry)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), QByteArray();
+    Q_CHECK_FILE_NAME(entry, QByteArray());
 
     QT_STATBUF statResult;
     if (QT_STAT(entry.nativeFilePath().constData(), &statResult)) {
@@ -887,8 +875,7 @@ QString QFileSystemEngine::bundleName(const QFileSystemEntry &entry)
 bool QFileSystemEngine::fillMetaData(const QFileSystemEntry &entry, QFileSystemMetaData &data,
         QFileSystemMetaData::MetaDataFlags what)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(entry, false);
 
 #if defined(Q_OS_DARWIN)
     if (what & QFileSystemMetaData::BundleType) {
@@ -1157,8 +1144,7 @@ static bool createDirectoryWithParents(const QByteArray &nativeName, bool should
 bool QFileSystemEngine::createDirectory(const QFileSystemEntry &entry, bool createParents)
 {
     QString dirName = entry.filePath();
-    if (Q_UNLIKELY(dirName.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(dirName, false);
 
     // Darwin doesn't support trailing /'s, so remove for everyone
     while (dirName.size() > 1 && dirName.endsWith(QLatin1Char('/')))
@@ -1177,8 +1163,7 @@ bool QFileSystemEngine::createDirectory(const QFileSystemEntry &entry, bool crea
 //static
 bool QFileSystemEngine::removeDirectory(const QFileSystemEntry &entry, bool removeEmptyParents)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(entry, false);
 
     if (removeEmptyParents) {
         QString dirName = QDir::cleanPath(entry.filePath());
@@ -1203,8 +1188,9 @@ bool QFileSystemEngine::removeDirectory(const QFileSystemEntry &entry, bool remo
 //static
 bool QFileSystemEngine::createLink(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
 {
-    if (Q_UNLIKELY(source.isEmpty() || target.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(source, false);
+    Q_CHECK_FILE_NAME(target, false);
+
     if (::symlink(source.nativeFilePath().constData(), target.nativeFilePath().constData()) == 0)
         return true;
     error = QSystemError(errno, QSystemError::StandardLibraryError);
@@ -1233,8 +1219,9 @@ bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSy
 {
     QFileSystemEntry::NativePath srcPath = source.nativeFilePath();
     QFileSystemEntry::NativePath tgtPath = target.nativeFilePath();
-    if (Q_UNLIKELY(srcPath.isEmpty() || tgtPath.isEmpty()))
-        return emptyFileEntryWarning(), false;
+
+    Q_CHECK_FILE_NAME(srcPath, false);
+    Q_CHECK_FILE_NAME(tgtPath, false);
 
 #if defined(RENAME_NOREPLACE) && QT_CONFIG(renameat2)
     if (renameat2(AT_FDCWD, srcPath, AT_FDCWD, tgtPath, RENAME_NOREPLACE) == 0)
@@ -1302,8 +1289,9 @@ bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSy
 //static
 bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
 {
-    if (Q_UNLIKELY(source.isEmpty() || target.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(source, false);
+    Q_CHECK_FILE_NAME(target, false);
+
     if (::rename(source.nativeFilePath().constData(), target.nativeFilePath().constData()) == 0)
         return true;
     error = QSystemError(errno, QSystemError::StandardLibraryError);
@@ -1313,8 +1301,7 @@ bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, cons
 //static
 bool QFileSystemEngine::removeFile(const QFileSystemEntry &entry, QSystemError &error)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(entry, false);
     if (unlink(entry.nativeFilePath().constData()) == 0)
         return true;
     error = QSystemError(errno, QSystemError::StandardLibraryError);
@@ -1349,8 +1336,7 @@ static mode_t toMode_t(QFile::Permissions permissions)
 //static
 bool QFileSystemEngine::setPermissions(const QFileSystemEntry &entry, QFile::Permissions permissions, QSystemError &error, QFileSystemMetaData *data)
 {
-    if (Q_UNLIKELY(entry.isEmpty()))
-        return emptyFileEntryWarning(), false;
+    Q_CHECK_FILE_NAME(entry, false);
 
     mode_t mode = toMode_t(permissions);
     bool success = ::chmod(entry.nativeFilePath().constData(), mode) == 0;

diff --git a/src/corelib/io/qfilesystemengine_win.cpp b/src/corelib/io/qfilesystemengine_win.cpp
@@ -461,7 +461,9 @@ void QFileSystemEngine::clearWinStatData(QFileSystemMetaData &data)
 QFileSystemEntry QFileSystemEngine::getLinkTarget(const QFileSystemEntry &link,
                                                   QFileSystemMetaData &data)
 {
-   if (data.missingFlags(QFileSystemMetaData::LinkType))
+    Q_CHECK_FILE_NAME(link, link);
+
+    if (data.missingFlags(QFileSystemMetaData::LinkType))
        QFileSystemEngine::fillMetaData(link, data, QFileSystemMetaData::LinkType);
 
     QString target;
@@ -480,6 +482,8 @@ QFileSystemEntry QFileSystemEngine::getLinkTarget(const QFileSystemEntry &link,
 //static
 QFileSystemEntry QFileSystemEngine::canonicalName(const QFileSystemEntry &entry, QFileSystemMetaData &data)
 {
+    Q_CHECK_FILE_NAME(entry, entry);
+
     if (data.missingFlags(QFileSystemMetaData::ExistsAttribute))
        QFileSystemEngine::fillMetaData(entry, data, QFileSystemMetaData::ExistsAttribute);
 
@@ -492,6 +496,8 @@ QFileSystemEntry QFileSystemEngine::canonicalName(const QFileSystemEntry &entry,
 //static
 QString QFileSystemEngine::nativeAbsoluteFilePath(const QString &path)
 {
+    Q_CHECK_FILE_NAME(path, QString());
+
     // can be //server or //server/share
     QString absPath;
     QVarLengthArray<wchar_t, MAX_PATH> buf(qMax(MAX_PATH, path.size() + 1));
@@ -527,6 +533,8 @@ QString QFileSystemEngine::nativeAbsoluteFilePath(const QString &path)
 //static
 QFileSystemEntry QFileSystemEngine::absoluteName(const QFileSystemEntry &entry)
 {
+    Q_CHECK_FILE_NAME(entry, entry);
+
     QString ret;
 
     if (!entry.isRelative()) {
@@ -609,6 +617,8 @@ QByteArray fileIdWin8(HANDLE handle)
 //static
 QByteArray QFileSystemEngine::id(const QFileSystemEntry &entry)
 {
+    Q_CHECK_FILE_NAME(entry, QByteArray());
+
     QByteArray result;
 
 #ifndef Q_OS_WINRT
@@ -999,6 +1009,7 @@ static bool isDirPath(const QString &dirPath, bool *existed);
 bool QFileSystemEngine::fillMetaData(const QFileSystemEntry &entry, QFileSystemMetaData &data,
                                      QFileSystemMetaData::MetaDataFlags what)
 {
+    Q_CHECK_FILE_NAME(entry, false);
     what |= QFileSystemMetaData::WinLnkType | QFileSystemMetaData::WinStatFlags;
     data.entryFlags &= ~what;
 
@@ -1116,6 +1127,8 @@ static bool isDirPath(const QString &dirPath, bool *existed)
 bool QFileSystemEngine::createDirectory(const QFileSystemEntry &entry, bool createParents)
 {
     QString dirName = entry.filePath();
+    Q_CHECK_FILE_NAME(dirName, false);
+
     if (createParents) {
         dirName = QDir::toNativeSeparators(QDir::cleanPath(dirName));
         // We spefically search for / so \ would break it..
@@ -1177,6 +1190,8 @@ bool QFileSystemEngine::createDirectory(const QFileSystemEntry &entry, bool crea
 bool QFileSystemEngine::removeDirectory(const QFileSystemEntry &entry, bool removeEmptyParents)
 {
     QString dirName = entry.filePath();
+    Q_CHECK_FILE_NAME(dirName, false);
+
     if (removeEmptyParents) {
         dirName = QDir::toNativeSeparators(QDir::cleanPath(dirName));
         for (int oldslash = 0, slash=dirName.length(); slash > 0; oldslash = slash) {
@@ -1381,6 +1396,9 @@ bool QFileSystemEngine::copyFile(const QFileSystemEntry &source, const QFileSyst
 //static
 bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
 {
+    Q_CHECK_FILE_NAME(source, false);
+    Q_CHECK_FILE_NAME(target, false);
+
 #ifndef Q_OS_WINRT
     bool ret = ::MoveFile((wchar_t*)source.nativeFilePath().utf16(),
                           (wchar_t*)target.nativeFilePath().utf16()) != 0;
@@ -1396,6 +1414,9 @@ bool QFileSystemEngine::renameFile(const QFileSystemEntry &source, const QFileSy
 //static
 bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, const QFileSystemEntry &target, QSystemError &error)
 {
+    Q_CHECK_FILE_NAME(source, false);
+    Q_CHECK_FILE_NAME(target, false);
+
     bool ret = ::MoveFileEx(reinterpret_cast<const wchar_t *>(source.nativeFilePath().utf16()),
                             reinterpret_cast<const wchar_t *>(target.nativeFilePath().utf16()),
                             MOVEFILE_REPLACE_EXISTING) != 0;
@@ -1407,6 +1428,8 @@ bool QFileSystemEngine::renameOverwriteFile(const QFileSystemEntry &source, cons
 //static
 bool QFileSystemEngine::removeFile(const QFileSystemEntry &entry, QSystemError &error)
 {
+    Q_CHECK_FILE_NAME(entry, false);
+
     bool ret = ::DeleteFile((wchar_t*)entry.nativeFilePath().utf16()) != 0;
     if(!ret)
         error = QSystemError(::GetLastError(), QSystemError::NativeError);
@@ -1417,6 +1440,8 @@ bool QFileSystemEngine::removeFile(const QFileSystemEntry &entry, QSystemError &
 bool QFileSystemEngine::setPermissions(const QFileSystemEntry &entry, QFile::Permissions permissions, QSystemError &error,
                                        QFileSystemMetaData *data)
 {
+    Q_CHECK_FILE_NAME(entry, false);
+
     Q_UNUSED(data);
     int mode = 0;
 

diff --git a/tests/auto/corelib/io/qfile/tst_qfile.cpp b/tests/auto/corelib/io/qfile/tst_qfile.cpp
@@ -550,6 +550,10 @@ void tst_QFile::exists()
     QFile unc(uncPath);
     QVERIFY2(unc.exists(), msgFileDoesNotExist(uncPath).constData());
 #endif
+
+    QTest::ignoreMessage(QtWarningMsg, "Broken filename passed to function");
+    QVERIFY(!QFile::exists(QDir::currentPath() + QLatin1Char('/') +
+                           QChar(QChar::Null) + QLatin1String("x/y")));
 }
 
 void tst_QFile::open_data()