From 6ac4f85298f9793cb640d2b0e35d2e0261fbd7d4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 26 Jul 2021 12:02:02 +0200 Subject: [PATCH 1/6] DirectoryDAO: Escape and quote file path strings in SQL query The SQL string quote character ' in path names needs to be escaped. --- src/library/dao/directorydao.cpp | 40 +++++++++++++++++++------------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/src/library/dao/directorydao.cpp b/src/library/dao/directorydao.cpp index 8ba7acae2b2..ab0c4618c29 100644 --- a/src/library/dao/directorydao.cpp +++ b/src/library/dao/directorydao.cpp @@ -6,6 +6,7 @@ #include "library/queryutil.h" #include "util/db/sqllikewildcardescaper.h" #include "util/db/sqllikewildcards.h" +#include "util/db/sqlstringformatter.h" namespace { @@ -80,13 +81,15 @@ int DirectoryDAO::removeDirectory(const QString& dir) const { QList DirectoryDAO::relocateDirectory( const QString& oldFolder, const QString& newFolder) const { + DEBUG_ASSERT(oldFolder == QDir(oldFolder).absolutePath()); + DEBUG_ASSERT(newFolder == QDir(newFolder).absolutePath()); // TODO(rryan): This method could use error reporting. It can fail in // mysterious ways for example if a track in the oldFolder also has a zombie // track location in newFolder then the replace query will fail because the // location column becomes non-unique. QSqlQuery query(m_database); query.prepare("UPDATE " % DIRECTORYDAO_TABLE % " SET " % DIRECTORYDAO_DIR % - "=:newFolder WHERE " % DIRECTORYDAO_DIR % " = :oldFolder"); + "=:newFolder WHERE " % DIRECTORYDAO_DIR % "=:oldFolder"); query.bindValue(":newFolder", newFolder); query.bindValue(":oldFolder", oldFolder); if (!query.exec()) { @@ -95,19 +98,23 @@ QList DirectoryDAO::relocateDirectory( return {}; } - // on Windows the absolute path starts with the drive name - // we also need to check for that - QString startsWithOldFolder = SqlLikeWildcardEscaper::apply( - QDir(oldFolder).absolutePath() + "/", kSqlLikeMatchAll) + kSqlLikeMatchAll; - - // Also update information in the track_locations table. This is where mixxx - // gets the location information for a track. Put marks around %1 so that - // this also works on windows - query.prepare(QString("SELECT library.id, track_locations.id, track_locations.location " - "FROM library INNER JOIN track_locations ON " - "track_locations.id = library.location WHERE " - "track_locations.location LIKE '%1' ESCAPE '%2'") - .arg(startsWithOldFolder, kSqlLikeMatchAll)); + // Appending '/' is required to disambiguate files from parent + // directories, e.g. "a/b.mp3" and "a/b/c.mp3" where "a/b" would + // match both instead of only files in the parent directory "a/b/". + DEBUG_ASSERT(!oldFolder.endsWith('/')); + const QString oldFolderPrefix = oldFolder + '/'; + const QString startsWithOldFolder = SqlLikeWildcardEscaper::apply( + oldFolderPrefix, kSqlLikeMatchAll) + + kSqlLikeMatchAll; + + query.prepare(QStringLiteral( + "SELECT library.id,track_locations.id,track_locations.location " + "FROM library INNER JOIN track_locations ON " + "track_locations.id=library.location WHERE " + "track_locations.location LIKE %1 ESCAPE '%2'") + .arg(SqlStringFormatter::format( + m_database, startsWithOldFolder), + kSqlLikeMatchAll)); if (!query.exec()) { LOG_FAILED_QUERY(query) << "could not relocate path of tracks"; return {}; @@ -131,8 +138,9 @@ QList DirectoryDAO::relocateDirectory( std::move(addedTrackRef))); } - QString replacement = "UPDATE track_locations SET location = :newloc " - "WHERE id = :id"; + // Also update information in the track_locations table. This is where mixxx + // gets the location information for a track. + const QString replacement = "UPDATE track_locations SET location=:newloc WHERE id=:id"; query.prepare(replacement); for (int i = 0; i < loc_ids.size(); ++i) { query.bindValue("newloc", relocatedTracks.at(i).updatedTrackRef().getLocation()); From f2829e4f7f8e64e4c65d0838b081d9ddd9902700 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 26 Jul 2021 12:06:22 +0200 Subject: [PATCH 2/6] DirectoryDAO: Use case-sensitive file path matching --- src/library/dao/directorydao.cpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/library/dao/directorydao.cpp b/src/library/dao/directorydao.cpp index ab0c4618c29..bc6dff4fc1d 100644 --- a/src/library/dao/directorydao.cpp +++ b/src/library/dao/directorydao.cpp @@ -123,16 +123,25 @@ QList DirectoryDAO::relocateDirectory( QList loc_ids; QList relocatedTracks; while (query.next()) { + const auto oldLocation = query.value(2).toString(); + const int oldSuffixLen = oldLocation.size() - oldFolder.size(); + QString newLocation = newFolder + oldLocation.right(oldSuffixLen); + // LIKE is case-insensitive! We cannot decide if the file system + // at the old location was case-sensitive or case-insensitive + // and must assume that the stored path is at least case-correct. + DEBUG_ASSERT(oldLocation.startsWith(oldFolderPrefix, Qt::CaseInsensitive)); + if (!oldLocation.startsWith(oldFolderPrefix, Qt::CaseSensitive)) { + qDebug() << "Skipping relocation of" << oldLocation + << "to" << newLocation; + continue; + } loc_ids.append(DbId(query.value(1).toInt())); - auto trackId = TrackId(query.value(0)); - auto oldLocation = query.value(2).toString(); + const auto trackId = TrackId(query.value(0)); auto missingTrackRef = TrackRef::fromFileInfo( TrackFile(oldLocation), std::move(trackId)); - const int oldSuffixLen = oldLocation.size() - oldFolder.size(); - QString newLocation = newFolder + oldLocation.right(oldSuffixLen); auto addedTrackRef = TrackRef::fromFileInfo( - TrackFile(newLocation) /*without TrackId*/); + TrackFile(newLocation)); // without TrackId, because no new track will be added! relocatedTracks.append(RelocatedTrack( std::move(missingTrackRef), std::move(addedTrackRef))); From e48faec7410c0dc04fe3c9548a7a703d37033017 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 26 Jul 2021 18:48:16 +0200 Subject: [PATCH 3/6] DB: Bind string values in SQL queries --- src/library/dao/directorydao.cpp | 8 +++----- src/library/dao/trackdao.cpp | 17 +++++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/library/dao/directorydao.cpp b/src/library/dao/directorydao.cpp index bc6dff4fc1d..79b1454efc5 100644 --- a/src/library/dao/directorydao.cpp +++ b/src/library/dao/directorydao.cpp @@ -6,7 +6,6 @@ #include "library/queryutil.h" #include "util/db/sqllikewildcardescaper.h" #include "util/db/sqllikewildcards.h" -#include "util/db/sqlstringformatter.h" namespace { @@ -111,10 +110,9 @@ QList DirectoryDAO::relocateDirectory( "SELECT library.id,track_locations.id,track_locations.location " "FROM library INNER JOIN track_locations ON " "track_locations.id=library.location WHERE " - "track_locations.location LIKE %1 ESCAPE '%2'") - .arg(SqlStringFormatter::format( - m_database, startsWithOldFolder), - kSqlLikeMatchAll)); + "track_locations.location LIKE :startsWithOldFolder ESCAPE :escape")); + query.bindValue(":startsWithOldFolder", startsWithOldFolder); + query.bindValue(":escape", kSqlLikeMatchAll); if (!query.exec()) { LOG_FAILED_QUERY(query) << "could not relocate path of tracks"; return {}; diff --git a/src/library/dao/trackdao.cpp b/src/library/dao/trackdao.cpp index 5dab5700fe2..1e82fbccc6b 100644 --- a/src/library/dao/trackdao.cpp +++ b/src/library/dao/trackdao.cpp @@ -1770,14 +1770,19 @@ void TrackDAO::hideAllTracks(const QDir& rootDir) const { // Capture entries that start with the directory prefix dir. // dir needs to end in a slash otherwise we might match other // directories. - QString likeClause = SqlLikeWildcardEscaper::apply(rootDir.absolutePath() + "/", kSqlLikeMatchAll) + kSqlLikeMatchAll; + const QString rootDirPath = rootDir.absolutePath(); + DEBUG_ASSERT(!rootDirPath.endsWith('/')); + const QString likeClause = + SqlLikeWildcardEscaper::apply(rootDirPath + '/', kSqlLikeMatchAll) + + kSqlLikeMatchAll; QSqlQuery query(m_database); - query.prepare(QString("SELECT library.id FROM library INNER JOIN track_locations " - "ON library.location = track_locations.id " - "WHERE track_locations.location LIKE %1 ESCAPE '%2'") - .arg(SqlStringFormatter::format(m_database, likeClause), kSqlLikeMatchAll)); - + query.prepare(QStringLiteral( + "SELECT library.id FROM library INNER JOIN track_locations " + "ON library.location = track_locations.id " + "WHERE track_locations.location LIKE :likeClause ESCAPE :escape")); + query.bindValue(":likeClause", likeClause); + query.bindValue(":escape", kSqlLikeMatchAll); VERIFY_OR_DEBUG_ASSERT(query.exec()) { LOG_FAILED_QUERY(query) << "could not get tracks within directory:" << rootDir; } From bd0baec324273a9628d78604bc0eb58603acb567 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Tue, 27 Jul 2021 08:37:38 +0200 Subject: [PATCH 4/6] Fix scope of __WINDOWS__ define --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e67b1f4b9d8..599ff9aa0ce 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1117,7 +1117,7 @@ endif() if(WIN32) - target_compile_definitions(mixxx-lib PRIVATE __WINDOWS__) + target_compile_definitions(mixxx-lib PUBLIC __WINDOWS__) # Helps prevent duplicate symbols target_compile_definitions(mixxx-lib PUBLIC _ATL_MIN_CRT) From e591747228e3f04e2c5efdb1086c8aa4cd36c50d Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Mon, 26 Jul 2021 12:54:00 +0200 Subject: [PATCH 5/6] Extend DirectoryDAOTest --- src/test/directorydaotest.cpp | 93 ++++++++++++++++++++++++++++------- 1 file changed, 75 insertions(+), 18 deletions(-) diff --git a/src/test/directorydaotest.cpp b/src/test/directorydaotest.cpp index 400929cfbe5..cef96b49a5a 100644 --- a/src/test/directorydaotest.cpp +++ b/src/test/directorydaotest.cpp @@ -137,50 +137,107 @@ TEST_F(DirectoryDAOTest, relocateDirectory) { const QTemporaryDir tempDir2; ASSERT_TRUE(tempDir2.isValid()); - //create temp dirs - QString testdir(tempDir1.filePath("TestDir")); - ASSERT_TRUE(QDir(tempDir1.path()).mkpath(testdir)); - QString test2(tempDir2.filePath("TestDir2")); - ASSERT_TRUE(QDir(tempDir2.path()).mkpath(test2)); - QString testnew(tempDir2.filePath("TestDirNew")); - ASSERT_TRUE(QDir(tempDir2.path()).mkpath(testnew)); + // Create temp dirs (with LIKE/GLOB wildcards and quotes in name) +#if defined(__WINDOWS__) + // Exclude reserved characters from path names: ", *, ? + // https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file#naming-conventions + const QString oldDir = tempDir1.filePath("Test'_%Dir"); + const QString newDir = tempDir2.filePath("TestDir'_%New"); + const QString otherDir = tempDir2.filePath("TestDir'_%Other"); +#else + const QString oldDir = tempDir1.filePath("Test'\"_*%?Dir"); + const QString newDir = tempDir2.filePath("TestDir'\"_*%?New"); + const QString otherDir = tempDir2.filePath("TestDir'\"_*%?Other"); +#endif + ASSERT_TRUE(QDir(tempDir1.path()).mkpath(oldDir)); + ASSERT_TRUE(QDir(tempDir2.path()).mkpath(newDir)); + ASSERT_TRUE(QDir(tempDir2.path()).mkpath(otherDir)); const DirectoryDAO& dao = internalCollection()->getDirectoryDAO(); - ASSERT_EQ(ALL_FINE, dao.addDirectory(testdir)); - ASSERT_EQ(ALL_FINE, dao.addDirectory(test2)); + ASSERT_EQ(ALL_FINE, dao.addDirectory(oldDir)); + ASSERT_EQ(ALL_FINE, dao.addDirectory(otherDir)); + + const QStringList oldDirs = dao.getDirs(); + ASSERT_EQ(2, oldDirs.size()); + ASSERT_THAT(oldDirs, UnorderedElementsAre(oldDir, otherDir)); - // ok now lets create some tracks here + // Add tracks that should be relocated + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(oldDir, "a." + getSupportedFileExt())), + false) + .isValid()); + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(oldDir, "b." + getSupportedFileExt())), + false) + .isValid()); + + // Add tracks that should be unaffected by the relocation + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(newDir, "c." + getSupportedFileExt())), + false) + .isValid()); + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(otherDir, "d." + getSupportedFileExt())), + false) + .isValid()); + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(oldDir + "." + getSupportedFileExt())), + false) + .isValid()); + ASSERT_TRUE(internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(newDir + "." + getSupportedFileExt())), + false) + .isValid()); ASSERT_TRUE(internalCollection() ->addTrack( Track::newTemporary( - TrackFile(testdir, "a." + getSupportedFileExt())), + TrackFile(otherDir + "." + getSupportedFileExt())), false) .isValid()); ASSERT_TRUE(internalCollection() ->addTrack( Track::newTemporary( - TrackFile(testdir, "b." + getSupportedFileExt())), + TrackFile(oldDir.toLower(), "a." + getSupportedFileExt())), false) .isValid()); ASSERT_TRUE(internalCollection() ->addTrack( Track::newTemporary( - TrackFile(test2, "c." + getSupportedFileExt())), + TrackFile(oldDir.toUpper(), "b." + getSupportedFileExt())), false) .isValid()); ASSERT_TRUE(internalCollection() ->addTrack( Track::newTemporary( - TrackFile(test2, "d." + getSupportedFileExt())), + TrackFile(newDir.toLower(), "c." + getSupportedFileExt())), false) .isValid()); + ASSERT_TRUE( + internalCollection() + ->addTrack( + Track::newTemporary( + TrackFile(otherDir.toUpper(), "d." + getSupportedFileExt())), + false) + .isValid()); QList relocatedTracks = - dao.relocateDirectory(testdir, testnew); + dao.relocateDirectory(oldDir, newDir); EXPECT_EQ(2, relocatedTracks.size()); - const QStringList allDirs = dao.getDirs(); - EXPECT_EQ(2, allDirs.size()); - EXPECT_THAT(allDirs, UnorderedElementsAre(test2, testnew)); + const QStringList newDirs = dao.getDirs(); + EXPECT_EQ(2, newDirs.size()); + EXPECT_THAT(newDirs, UnorderedElementsAre(newDir, otherDir)); } From 3a58c6242e532c83208e72afc681b41d5c0bb5ce Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Thu, 29 Jul 2021 15:38:17 +0200 Subject: [PATCH 6/6] DirectoryDAO: Rename "folder" as "directory" --- src/library/dao/directorydao.cpp | 40 ++++++++++++++++---------------- src/library/dao/directorydao.h | 4 ++-- 2 files changed, 22 insertions(+), 22 deletions(-) diff --git a/src/library/dao/directorydao.cpp b/src/library/dao/directorydao.cpp index 79b1454efc5..9525f2aa8ab 100644 --- a/src/library/dao/directorydao.cpp +++ b/src/library/dao/directorydao.cpp @@ -78,40 +78,40 @@ int DirectoryDAO::removeDirectory(const QString& dir) const { } QList DirectoryDAO::relocateDirectory( - const QString& oldFolder, - const QString& newFolder) const { - DEBUG_ASSERT(oldFolder == QDir(oldFolder).absolutePath()); - DEBUG_ASSERT(newFolder == QDir(newFolder).absolutePath()); + const QString& oldDirectory, + const QString& newDirectory) const { + DEBUG_ASSERT(oldDirectory == QDir(oldDirectory).absolutePath()); + DEBUG_ASSERT(newDirectory == QDir(newDirectory).absolutePath()); // TODO(rryan): This method could use error reporting. It can fail in - // mysterious ways for example if a track in the oldFolder also has a zombie - // track location in newFolder then the replace query will fail because the + // mysterious ways for example if a track in the oldDirectory also has a zombie + // track location in newDirectory then the replace query will fail because the // location column becomes non-unique. QSqlQuery query(m_database); query.prepare("UPDATE " % DIRECTORYDAO_TABLE % " SET " % DIRECTORYDAO_DIR % - "=:newFolder WHERE " % DIRECTORYDAO_DIR % "=:oldFolder"); - query.bindValue(":newFolder", newFolder); - query.bindValue(":oldFolder", oldFolder); + "=:newDirectory WHERE " % DIRECTORYDAO_DIR % "=:oldDirectory"); + query.bindValue(":newDirectory", newDirectory); + query.bindValue(":oldDirectory", oldDirectory); if (!query.exec()) { LOG_FAILED_QUERY(query) << "could not relocate directory" - << oldFolder << "to" << newFolder; + << oldDirectory << "to" << newDirectory; return {}; } // Appending '/' is required to disambiguate files from parent // directories, e.g. "a/b.mp3" and "a/b/c.mp3" where "a/b" would // match both instead of only files in the parent directory "a/b/". - DEBUG_ASSERT(!oldFolder.endsWith('/')); - const QString oldFolderPrefix = oldFolder + '/'; - const QString startsWithOldFolder = SqlLikeWildcardEscaper::apply( - oldFolderPrefix, kSqlLikeMatchAll) + + DEBUG_ASSERT(!oldDirectory.endsWith('/')); + const QString oldDirectoryPrefix = oldDirectory + '/'; + const QString startsWithOldDirectory = SqlLikeWildcardEscaper::apply( + oldDirectoryPrefix, kSqlLikeMatchAll) + kSqlLikeMatchAll; query.prepare(QStringLiteral( "SELECT library.id,track_locations.id,track_locations.location " "FROM library INNER JOIN track_locations ON " "track_locations.id=library.location WHERE " - "track_locations.location LIKE :startsWithOldFolder ESCAPE :escape")); - query.bindValue(":startsWithOldFolder", startsWithOldFolder); + "track_locations.location LIKE :startsWithOldDirectory ESCAPE :escape")); + query.bindValue(":startsWithOldDirectory", startsWithOldDirectory); query.bindValue(":escape", kSqlLikeMatchAll); if (!query.exec()) { LOG_FAILED_QUERY(query) << "could not relocate path of tracks"; @@ -122,13 +122,13 @@ QList DirectoryDAO::relocateDirectory( QList relocatedTracks; while (query.next()) { const auto oldLocation = query.value(2).toString(); - const int oldSuffixLen = oldLocation.size() - oldFolder.size(); - QString newLocation = newFolder + oldLocation.right(oldSuffixLen); + const int oldSuffixLen = oldLocation.size() - oldDirectory.size(); + QString newLocation = newDirectory + oldLocation.right(oldSuffixLen); // LIKE is case-insensitive! We cannot decide if the file system // at the old location was case-sensitive or case-insensitive // and must assume that the stored path is at least case-correct. - DEBUG_ASSERT(oldLocation.startsWith(oldFolderPrefix, Qt::CaseInsensitive)); - if (!oldLocation.startsWith(oldFolderPrefix, Qt::CaseSensitive)) { + DEBUG_ASSERT(oldLocation.startsWith(oldDirectoryPrefix, Qt::CaseInsensitive)); + if (!oldLocation.startsWith(oldDirectoryPrefix, Qt::CaseSensitive)) { qDebug() << "Skipping relocation of" << oldLocation << "to" << newLocation; continue; diff --git a/src/library/dao/directorydao.h b/src/library/dao/directorydao.h index 5478cd699d4..907a5a9f061 100644 --- a/src/library/dao/directorydao.h +++ b/src/library/dao/directorydao.h @@ -24,6 +24,6 @@ class DirectoryDAO : public DAO { int removeDirectory(const QString& dir) const; QList relocateDirectory( - const QString& oldFolder, - const QString& newFolder) const; + const QString& oldDirectory, + const QString& newDirectory) const; };