Skip to content

Commit

Permalink
Move FileWatcher into Database class
Browse files Browse the repository at this point in the history 
* Fix keepassxreboot#3506
* Fix keepassxreboot#2389
* Fix keepassxreboot#2536
* Fix keepassxreboot#2230

Every database that has been opened now watch's it's own file. This allows the database class to manage file changes and detect fail conditions during saving. Additionally, all stakeholders of the database can listen for the database file changed notification and respond accordingly.

Performed significant cleanup of the autoreload code within DatabaseWidget. Fixed several issues with handling changes due to merging, not merging, and other scenarios while reloading.

Prevent database saves to the same file if there are changes on disk that have not been merged with the open database.
  • Loading branch information
@droidmonkey
droidmonkey committed Oct 20, 2019
1 parent 6b74691 commit 744b4ab
Show file tree
Hide file tree
Showing 10 changed files with 204 additions and 118 deletions.
74 changes: 51 additions & 23 deletions src/core/Database.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "Database.h"

#include "core/Clock.h"
#include "core/FileWatcher.h"
#include "core/Group.h"
#include "core/Merger.h"
#include "core/Metadata.h"
Expand All @@ -42,6 +43,7 @@ Database::Database()
, m_data()
, m_rootGroup(nullptr)
, m_timer(new QTimer(this))
, m_fileWatcher(new FileWatcher(this))
, m_emitModified(false)
, m_uuid(QUuid::createUuid())
{
Expand All @@ -54,7 +56,9 @@ Database::Database()

connect(m_metadata, SIGNAL(metadataModified()), this, SLOT(markAsModified()));
connect(m_timer, SIGNAL(timeout()), SIGNAL(databaseModified()));
connect(this, SIGNAL(databaseOpened()), SLOT(updateCommonUsernames()));
connect(this, SIGNAL(databaseSaved()), SLOT(updateCommonUsernames()));
connect(m_fileWatcher, SIGNAL(fileChanged()), SIGNAL(databaseFileChanged()));

m_modified = false;
m_emitModified = true;
Expand Down Expand Up @@ -116,6 +120,7 @@ bool Database::open(const QString& filePath, QSharedPointer<const CompositeKey>
emit databaseDiscarded();
}

m_initialized = false;
setEmitModified(false);

QFile dbFile(filePath);
Expand All @@ -138,8 +143,7 @@ bool Database::open(const QString& filePath, QSharedPointer<const CompositeKey>
}

KeePass2Reader reader;
bool ok = reader.readDatabase(&dbFile, std::move(key), this);
if (reader.hasError()) {
if (!reader.readDatabase(&dbFile, std::move(key), this)) {
if (error) {
*error = tr("Error while reading the database: %1").arg(reader.errorString());
}
Expand All @@ -150,22 +154,23 @@ bool Database::open(const QString& filePath, QSharedPointer<const CompositeKey>
setFilePath(filePath);
dbFile.close();

updateCommonUsernames();

setInitialized(ok);
markAsClean();

m_initialized = true;
emit databaseOpened();
m_fileWatcher->start(canonicalFilePath());
setEmitModified(true);
return ok;

return true;
}

/**
* Save the database to the current file path. It is an error to call this function
* if no file path has been defined.
*
* @param error error message in case of failure
* @param atomic Use atomic file transactions
* @param backup Backup the existing database file, if exists
* @param error error message in case of failure
* @return true on success
*/
bool Database::save(QString* error, bool atomic, bool backup)
Expand Down Expand Up @@ -194,27 +199,52 @@ bool Database::save(QString* error, bool atomic, bool backup)
* wrong moment.
*
* @param filePath Absolute path of the file to save
* @param error error message in case of failure
* @param atomic Use atomic file transactions
* @param backup Backup the existing database file, if exists
* @param error error message in case of failure
* @return true on success
*/
bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool backup)
{
// Disallow saving to the same file if read-only
if (m_data.isReadOnly && filePath == m_data.filePath) {
Q_ASSERT_X(false, "Database::saveAs", "Could not save, database file is read-only.");
if (error) {
*error = tr("Could not save, database file is read-only.");
if (filePath == m_data.filePath) {
// Disallow saving to the same file if read-only
if (m_data.isReadOnly) {
Q_ASSERT_X(false, "Database::saveAs", "Could not save, database file is read-only.");
if (error) {
*error = tr("Could not save, database file is read-only.");
}
return false;
}

// Fail-safe check to make sure we don't overwrite underlying file changes
// that have not yet triggered a file reload/merge operation.
if (!m_fileWatcher->hasSameFileChecksum()) {
if (error) {
*error = tr("Database file has unmerged changes.");
}
return false;
}
return false;
}

// Clear read-only flag
setReadOnly(false);
m_fileWatcher->stop();

auto& canonicalFilePath = QFileInfo::exists(filePath) ? QFileInfo(filePath).canonicalFilePath() : filePath;
bool ok = performSave(canonicalFilePath, error, atomic, backup);
if (ok) {
setFilePath(filePath);
m_fileWatcher->start(canonicalFilePath);
} else {
// Saving failed, don't rewatch file since it does not represent our database
markAsModified();
}

return ok;
}

bool Database::performSave(const QString& filePath, QString* error, bool atomic, bool backup)
{
if (atomic) {
QSaveFile saveFile(filePath);
if (saveFile.open(QIODevice::WriteOnly)) {
Expand All @@ -224,12 +254,11 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool
}

if (backup) {
backupDatabase(canonicalFilePath);
backupDatabase(filePath);
}

if (saveFile.commit()) {
// successfully saved database file
setFilePath(filePath);
return true;
}
}
Expand All @@ -248,28 +277,26 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool
tempFile.close(); // flush to disk

if (backup) {
backupDatabase(canonicalFilePath);
backupDatabase(filePath);
}

// Delete the original db and move the temp file in place
QFile::remove(canonicalFilePath);
QFile::remove(filePath);

// Note: call into the QFile rename instead of QTemporaryFile
// due to an undocumented difference in how the function handles
// errors. This prevents errors when saving across file systems.
if (tempFile.QFile::rename(canonicalFilePath)) {
if (tempFile.QFile::rename(filePath)) {
// successfully saved the database
tempFile.setAutoRemove(false);
setFilePath(filePath);
return true;
} else if (!backup || !restoreDatabase(canonicalFilePath)) {
} else if (!backup || !restoreDatabase(filePath)) {
// Failed to copy new database in place, and
// failed to restore from backup or backups disabled
tempFile.setAutoRemove(false);
if (error) {
*error = tr("%1\nBackup database located at %2").arg(tempFile.errorString(), tempFile.fileName());
}
markAsModified();
return false;
}
}
Expand All @@ -280,7 +307,6 @@ bool Database::saveAs(const QString& filePath, QString* error, bool atomic, bool
}

// Saving failed
markAsModified();
return false;
}

Expand Down Expand Up @@ -490,6 +516,8 @@ void Database::setFilePath(const QString& filePath)
if (filePath != m_data.filePath) {
QString oldPath = m_data.filePath;
m_data.filePath = filePath;
// Don't watch for changes until the next open or save operation
m_fileWatcher->stop();
emit filePathChanged(oldPath, filePath);
}
}
Expand Down
5 changes: 5 additions & 0 deletions src/core/Database.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

class Entry;
enum class EntryReferenceType;
class FileWatcher;
class Group;
class Metadata;
class QTimer;
Expand Down Expand Up @@ -144,9 +145,11 @@ public slots:
void groupRemoved();
void groupAboutToMove(Group* group, Group* toGroup, int index);
void groupMoved();
void databaseOpened();
void databaseModified();
void databaseSaved();
void databaseDiscarded();
void databaseFileChanged();

private slots:
void startModifiedTimer();
Expand Down Expand Up @@ -177,12 +180,14 @@ private slots:
bool writeDatabase(QIODevice* device, QString* error = nullptr);
bool backupDatabase(const QString& filePath);
bool restoreDatabase(const QString& filePath);
bool performSave(const QString& filePath, QString* error, bool atomic, bool backup);

Metadata* const m_metadata;
DatabaseData m_data;
Group* m_rootGroup;
QList<DeletedObject> m_deletedObjects;
QPointer<QTimer> m_timer;
QPointer<FileWatcher> m_fileWatcher;
bool m_initialized = false;
bool m_modified = false;
bool m_emitModified;
Expand Down
107 changes: 65 additions & 42 deletions src/core/FileWatcher.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "FileWatcher.h"

#include "core/Clock.h"
#include <QCryptographicHash>
#include <QFileInfo>

#ifdef Q_OS_LINUX
Expand All @@ -27,36 +28,23 @@

namespace
{
const int FileChangeDelay = 500;
const int TimerResolution = 100;
const int FileChangeDelay = 200;
} // namespace

DelayingFileWatcher::DelayingFileWatcher(QObject* parent)
FileWatcher::FileWatcher(QObject* parent)
: QObject(parent)
, m_ignoreFileChange(false)
{
connect(&m_fileWatcher, SIGNAL(fileChanged(QString)), this, SLOT(onWatchedFileChanged()));
connect(&m_fileUnblockTimer, SIGNAL(timeout()), this, SLOT(observeFileChanges()));
connect(&m_fileChangeDelayTimer, SIGNAL(timeout()), this, SIGNAL(fileChanged()));
connect(&m_fileWatcher, SIGNAL(fileChanged(QString)), SLOT(onWatchedFileChanged()));
connect(&m_fileChangeDelayTimer, SIGNAL(timeout()), SIGNAL(fileChanged()));
connect(&m_fileChecksumTimer, SIGNAL(timeout()), SLOT(checkFileChecksum()));
m_fileChangeDelayTimer.setSingleShot(true);
m_fileUnblockTimer.setSingleShot(true);
m_fileIgnoreDelayTimer.setSingleShot(true);
}

void DelayingFileWatcher::restart()
void FileWatcher::start(const QString& filePath, int checksumInterval)
{
m_fileWatcher.addPath(m_filePath);
}

void DelayingFileWatcher::stop()
{
m_fileWatcher.removePath(m_filePath);
}

void DelayingFileWatcher::start(const QString& filePath)
{
if (!m_filePath.isEmpty()) {
m_fileWatcher.removePath(m_filePath);
}
stop();

#if defined(Q_OS_LINUX)
struct statfs statfsBuf;
Expand All @@ -74,45 +62,80 @@ void DelayingFileWatcher::start(const QString& filePath)
#endif

m_fileWatcher.addPath(filePath);
m_filePath = filePath;
m_fileChecksum = calculateChecksum();
m_fileChecksumTimer.start(checksumInterval);
m_ignoreFileChange = false;
}

if (!filePath.isEmpty()) {
m_filePath = filePath;
void FileWatcher::stop()
{
if (!m_filePath.isEmpty()) {
m_fileWatcher.removePath(m_filePath);
}
m_filePath.clear();
m_fileChecksum.clear();
m_fileChangeDelayTimer.stop();
}

void DelayingFileWatcher::ignoreFileChanges()
void FileWatcher::pause()
{
m_ignoreFileChange = true;
m_fileChangeDelayTimer.stop();
}

void DelayingFileWatcher::observeFileChanges(bool delayed)
void FileWatcher::resume()
{
int timeout = 0;
if (delayed) {
timeout = FileChangeDelay;
} else {
m_ignoreFileChange = false;
start(m_filePath);
}
if (timeout > 0 && !m_fileUnblockTimer.isActive()) {
m_fileUnblockTimer.start(timeout);
m_ignoreFileChange = false;
// Add a short delay to start in the next event loop
if (!m_fileIgnoreDelayTimer.isActive()) {
m_fileIgnoreDelayTimer.start(0);
}
}

void DelayingFileWatcher::onWatchedFileChanged()
void FileWatcher::onWatchedFileChanged()
{
if (m_ignoreFileChange) {
// the client forcefully silenced us
// Don't notify if we are ignoring events or already started a notification chain
if (shouldIgnoreChanges()) {
return;
}
if (m_fileChangeDelayTimer.isActive()) {
// we are waiting to fire the delayed fileChanged event, so nothing
// to do here

m_fileChecksum = calculateChecksum();
m_fileChangeDelayTimer.start(0);
}

bool FileWatcher::shouldIgnoreChanges()
{
return m_filePath.isEmpty() || m_ignoreFileChange || m_fileIgnoreDelayTimer.isActive()
|| m_fileChangeDelayTimer.isActive();
}

bool FileWatcher::hasSameFileChecksum()
{
return calculateChecksum() == m_fileChecksum;
}

void FileWatcher::checkFileChecksum()
{
if (shouldIgnoreChanges()) {
return;
}

m_fileChangeDelayTimer.start(FileChangeDelay);
if (!hasSameFileChecksum()) {
onWatchedFileChanged();
}
}

QByteArray FileWatcher::calculateChecksum()
{
QFile file(m_filePath);
if (file.open(QFile::ReadOnly)) {
QCryptographicHash hash(QCryptographicHash::Sha256);
if (hash.addData(&file)) {
return hash.result();
}
}
return {};
}

BulkFileWatcher::BulkFileWatcher(QObject* parent)
Expand Down Expand Up @@ -281,7 +304,7 @@ void BulkFileWatcher::observeFileChanges(bool delayed)
{
int timeout = 0;
if (delayed) {
timeout = TimerResolution;
timeout = FileChangeDelay;
} else {
const QDateTime current = Clock::currentDateTimeUtc();
for (const QString& key : m_watchedFilesIgnored.keys()) {
Expand Down
Loading

0 comments on commit 744b4ab

Please sign in to comment.