From 8f1b0bda2244223d898138cf3c82d34cc0e12983 Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Wed, 24 Oct 2018 00:06:09 +0200 Subject: [PATCH 1/2] Fix saving to files that don't exist Previous commit broke it. I'll add unit tests in a minute --- part.cpp | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/part.cpp b/part.cpp index 78d851f68..8ade20364 100644 --- a/part.cpp +++ b/part.cpp @@ -2493,6 +2493,19 @@ bool Part::saveAs(const QUrl & saveUrl) return saveAs( saveUrl, isDocumentArchive ? SaveAsOkularArchive : NoSaveAsFlags ); } +static QUrl resolveSymlinksIfFileExists( const QUrl &saveUrl ) +{ + if ( saveUrl.isLocalFile() ) + { + const QFileInfo fi( saveUrl.toLocalFile() ); + return fi.exists() ? QUrl::fromLocalFile( fi.canonicalFilePath() ) : saveUrl; + } + else + { + return saveUrl; + } +} + bool Part::saveAs( const QUrl & saveUrl, SaveAsFlags flags ) { // TODO When we get different saving backends we need to query the backend @@ -2537,8 +2550,7 @@ bool Part::saveAs( const QUrl & saveUrl, SaveAsFlags flags ) tf.close(); // Figure out the real save url, for symlinks we don't want to copy over the symlink but over the target file - const QUrl realSaveUrl = saveUrl.isLocalFile() ? QUrl::fromLocalFile( QFileInfo( saveUrl.toLocalFile() ).canonicalFilePath() ) - : saveUrl; + const QUrl realSaveUrl = resolveSymlinksIfFileExists( saveUrl ); QScopedPointer tempFile; KIO::Job *copyJob = nullptr; // this will be filled with the job that writes to saveUrl From 05462e2670988c2e83161f3fa1bd5e034cf50283 Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Wed, 24 Oct 2018 00:30:46 +0200 Subject: [PATCH 2/2] Add three autotests for part saving * saving as on a non existing file works * saving as on a symlink doesn't destroy the symlink * saving on the symlink used to open the file doesn't destroy the symlink --- autotests/parttest.cpp | 89 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/autotests/parttest.cpp b/autotests/parttest.cpp index 4c7e907a8..871a07ed7 100644 --- a/autotests/parttest.cpp +++ b/autotests/parttest.cpp @@ -96,6 +96,9 @@ class PartTest void test388288(); void testSaveAs(); void testSaveAs_data(); + void testSaveAsToNonExistingPath(); + void testSaveAsToSymlink(); + void testSaveIsSymlink(); void testSidebarItemAfterSaving(); void testSaveAsUndoStackAnnotations(); void testSaveAsUndoStackAnnotations_data(); @@ -809,6 +812,92 @@ void PartTest::simulateMouseSelection(double startX, double startY, double endX, events.simulate(target); } +void PartTest::testSaveAsToNonExistingPath() +{ + Okular::Part part(nullptr, nullptr, QVariantList()); + part.openDocument( KDESRCDIR "data/file1.pdf" ); + + QString saveFilePath; + { + QTemporaryFile saveFile( QString( "%1/okrXXXXXX.pdf" ).arg( QDir::tempPath() ) ); + saveFile.open(); + saveFilePath = saveFile.fileName(); + // QTemporaryFile is destroyed and the file it created is gone, this is a TOCTOU but who cares + } + + QVERIFY( !QFileInfo::exists( saveFilePath ) ); + + QVERIFY( part.saveAs( QUrl::fromLocalFile( saveFilePath ), Part::NoSaveAsFlags ) ); + + QFile::remove( saveFilePath ); +} + +void PartTest::testSaveAsToSymlink() +{ +#ifdef Q_OS_UNIX + Okular::Part part(nullptr, nullptr, QVariantList()); + part.openDocument( KDESRCDIR "data/file1.pdf" ); + + QTemporaryFile newFile( QString( "%1/okrXXXXXX.pdf" ).arg( QDir::tempPath() ) ); + newFile.open(); + + QString linkFilePath; + { + QTemporaryFile linkFile( QString( "%1/okrXXXXXX.pdf" ).arg( QDir::tempPath() ) ); + linkFile.open(); + linkFilePath = linkFile.fileName(); + // QTemporaryFile is destroyed and the file it created is gone, this is a TOCTOU but who cares + } + + QFile::link( newFile.fileName(), linkFilePath ); + + QVERIFY( QFileInfo( linkFilePath ).isSymLink() ); + + QVERIFY( part.saveAs( QUrl::fromLocalFile( linkFilePath ), Part::NoSaveAsFlags ) ); + + QVERIFY( QFileInfo( linkFilePath ).isSymLink() ); + + QFile::remove( linkFilePath ); +#endif +} + +void PartTest::testSaveIsSymlink() +{ +#ifdef Q_OS_UNIX + Okular::Part part(nullptr, nullptr, QVariantList()); + + QString newFilePath; + { + QTemporaryFile newFile( QString( "%1/okrXXXXXX.pdf" ).arg( QDir::tempPath() ) ); + newFile.open(); + newFilePath = newFile.fileName(); + // QTemporaryFile is destroyed and the file it created is gone, this is a TOCTOU but who cares + } + + QFile::copy( KDESRCDIR "data/file1.pdf", newFilePath ); + + QString linkFilePath; + { + QTemporaryFile linkFile( QString( "%1/okrXXXXXX.pdf" ).arg( QDir::tempPath() ) ); + linkFile.open(); + linkFilePath = linkFile.fileName(); + // QTemporaryFile is destroyed and the file it created is gone, this is a TOCTOU but who cares + } + + QFile::link( newFilePath, linkFilePath ); + + QVERIFY( QFileInfo( linkFilePath ).isSymLink() ); + + part.openDocument( linkFilePath ); + QVERIFY( part.saveAs( QUrl::fromLocalFile( linkFilePath ), Part::NoSaveAsFlags ) ); + + QVERIFY( QFileInfo( linkFilePath ).isSymLink() ); + + QFile::remove( newFilePath ); + QFile::remove( linkFilePath ); +#endif +} + void PartTest::testSaveAs() { QFETCH(QString, file);