From 81bc95704dd5960e18b5a61e2b0110e961114ee7 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 "/.."). Change-Id: Iddf6f46edf6f3b6c3222fffd1e1e5479f0be92a9 Reviewed-by: Ahmad Samir (cherry picked from commit 7bd7df5aa170c240061144a9210a13b62949935c) Reviewed-by: Qt Cherry-pick Bot (cherry picked from commit c617cc95934ae3c0896082d61a88487b34cf96be) --- 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 240e496f591..40ab67bb34f 100644 --- a/src/corelib/io/qfilesystemengine_unix.cpp +++ b/src/corelib/io/qfilesystemengine_unix.cpp @@ -1820,6 +1820,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 @@ -1833,10 +1842,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 0e5b997eccb..344a9bff75c 100644 --- a/tests/auto/corelib/io/qdir/tst_qdir.cpp +++ b/tests/auto/corelib/io/qdir/tst_qdir.cpp @@ -1223,7 +1223,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 } @@ -1237,21 +1238,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()