From 7bd7df5aa170c240061144a9210a13b62949935c Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 8 May 2025 11:01:39 +0200 Subject: [PATCH] QFileSystemEngine::tempPath: bypass QDir and go straight to QFSEngine Temporary paths coming from the environment must be real filesystem things, never a Qt file engine, so we don't need to create QDir with its QDirPrivate, in order to call QFileSystemEngine. The replacing of canonicalPath() with QFSE::absoluteName() is fine because canonicalizing *after* cleanPath() is the wrong thing. For example, if you had: $ ln -s $HOME/tmp /tmp/symlink $ TMPDIR=/tmp/symlink/.. then cleanPath($TMPDIR) = /tmp absolute($TMPDIR) = /tmp # QFSE::absoluteName calls cleanPath canonical($TMPDIR) = $HOME canonical(cleanPath($TMPDIR)) = /tmp The lack of canonicalization now only affects when the final path is a symlink. Doing so bought us little security if it is a symlink and it could change, because the result is not cached and could change from call to call. That changing is probably worse than any attack, because you could end up with QDir::tempPath() != QDir::tempPath() [ChangeLog][QtCore][QDir] tempPath() may now return a non-canonical path. This means going up from it (cdUp()) may result in different paths from string manipulation (adding "/.."). Pick-to: 6.10 6.9 Change-Id: Iddf6f46edf6f3b6c3222fffd1e1e5479f0be92a9 Reviewed-by: Ahmad Samir --- src/corelib/io/qfilesystemengine_unix.cpp | 20 ++++++++++++++-- tests/auto/corelib/io/qdir/tst_qdir.cpp | 29 +++++++++++++---------- 2 files changed, 35 insertions(+), 14 deletions(-) diff --git a/src/corelib/io/qfilesystemengine_unix.cpp b/src/corelib/io/qfilesystemengine_unix.cpp index eaba1368355..61b7ecbde17 100644 --- a/src/corelib/io/qfilesystemengine_unix.cpp +++ b/src/corelib/io/qfilesystemengine_unix.cpp @@ -1858,6 +1858,15 @@ QString QFileSystemEngine::rootPath() return u"/"_s; } +static constexpr QLatin1StringView nativeTempPath() noexcept +{ + // _PATH_TMP usually ends in '/' and we don't want that + QLatin1StringView temp = _PATH_TMP ""_L1; + static_assert(_PATH_TMP[0] == '/', "_PATH_TMP needs to be absolute"); + static_assert(_PATH_TMP[1] != '\0', "Are you really sure _PATH_TMP should be the root dir??"); + return temp; +} + QString QFileSystemEngine::tempPath() { #ifdef QT_UNIX_TEMP_PATH_OVERRIDE @@ -1871,10 +1880,17 @@ QString QFileSystemEngine::tempPath() temp = QString::fromCFString((CFStringRef)nsPath); #endif } else { - temp = _PATH_TMP ""_L1; + constexpr auto nativeTemp = nativeTempPath(); + temp = nativeTemp; } } - return QDir(QDir::cleanPath(temp)).canonicalPath(); + + // the environment variable may also end in '/' + if (temp.size() > 1 && temp.endsWith(u'/')) + temp.chop(1); + + QFileSystemEntry e(temp, QFileSystemEntry::FromInternalPath{}); + return QFileSystemEngine::absoluteName(e).filePath(); #endif } diff --git a/tests/auto/corelib/io/qdir/tst_qdir.cpp b/tests/auto/corelib/io/qdir/tst_qdir.cpp index b7313cf5481..99232c852b6 100644 --- a/tests/auto/corelib/io/qdir/tst_qdir.cpp +++ b/tests/auto/corelib/io/qdir/tst_qdir.cpp @@ -1240,7 +1240,8 @@ void tst_QDir::current() #if defined(Q_OS_WIN) QCOMPARE(newCurrent.absolutePath().toLower(), currentDir.toLower()); #else - QCOMPARE(newCurrent.absolutePath(), currentDir); + // getcwd(2) on Unix returns the canonical path + QCOMPARE(newCurrent.absolutePath(), QDir(currentDir).canonicalPath()); #endif } @@ -1254,21 +1255,25 @@ void tst_QDir::cd_data() QTest::addColumn("successExpected"); QTest::addColumn("newDir"); - int index = m_dataPath.lastIndexOf(QLatin1Char('/')); - QTest::newRow("cdUp") << m_dataPath << ".." << true << m_dataPath.left(index==0?1:index); + // use the canonical path for m_dataPath here, because if TMPDIR points to + // a symlink like what happens on Apple systems (/tmp -> /private/tmp), + // then /tmp/.. will not be the same as / (it's /private). + QString canonicalPath = QDir(m_dataPath).canonicalPath(); + int index = canonicalPath.lastIndexOf(QLatin1Char('/')); + QTest::newRow("cdUp") << canonicalPath << ".." << true << canonicalPath.left(index==0?1:index); QTest::newRow("cdUp non existent (relative dir)") << "anonexistingDir" << ".." - << true << m_dataPath; - QTest::newRow("cdUp non existent (absolute dir)") << m_dataPath + "/anonexistingDir" << ".." - << true << m_dataPath; - QTest::newRow("noChange") << m_dataPath << "." << true << m_dataPath; + << true << canonicalPath; + QTest::newRow("cdUp non existent (absolute dir)") << canonicalPath + "/anonexistingDir" << ".." + << true << canonicalPath; + QTest::newRow("noChange") << canonicalPath << "." << true << canonicalPath; #if defined(Q_OS_WIN) // on windows QDir::root() is usually c:/ but cd "/" will not force it to be root - QTest::newRow("absolute") << m_dataPath << "/" << true << "/"; + QTest::newRow("absolute") << canonicalPath << "/" << true << "/"; #else - QTest::newRow("absolute") << m_dataPath << "/" << true << QDir::root().absolutePath(); + QTest::newRow("absolute") << canonicalPath << "/" << true << QDir::root().absolutePath(); #endif - QTest::newRow("non existant") << "." << "../anonexistingdir" << false << m_dataPath; - QTest::newRow("self") << "." << (QString("../") + QFileInfo(m_dataPath).fileName()) << true << m_dataPath; - QTest::newRow("file") << "." << "qdir.pro" << false << m_dataPath; + QTest::newRow("non existant") << "." << "../anonexistingdir" << false << canonicalPath; + QTest::newRow("self") << "." << (QString("../") + QFileInfo(canonicalPath).fileName()) << true << canonicalPath; + QTest::newRow("file") << "." << "qdir.pro" << false << canonicalPath; } void tst_QDir::cd()