Skip to content

Commit

Permalink
OwnSql: Distinguish no-data from error #6677
Browse files Browse the repository at this point in the history 
This could fix a problem where the client incorrectly decides to delete
local data.

Previously any sqlite3_step() return value that wasn't SQLITE_ROW would
be interpreted as "there's no more data here". Thus an sqlite error at a
bad time could cause the remote discovery to fail to read an unchanged
subtree from the database. These files would then be deleted locally.

With this change sqlite errors from sqlite3_step are detected and
logged. For the particular case of SyncJournalDb::getFilesBelowPath()
the error will now be propagated and the sync run will fail instead of
performing spurious deletes.

Note that many other database functions still don't distinguish
not-found from error cases. Most of them won't have as severe effects on
affected sync runs though.
  • Loading branch information
@ckamm
ckamm committed Mar 4, 2019
1 parent c859f87 commit 7eeb86f
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 43 deletions.
27 changes: 24 additions & 3 deletions src/common/ownsql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,31 @@ bool SqlQuery::exec()
return true;
}

bool SqlQuery::next()
auto SqlQuery::next() -> NextResult
{
SQLITE_DO(sqlite3_step(_stmt));
return _errId == SQLITE_ROW;
const bool firstStep = !sqlite3_stmt_busy(_stmt);

int n = 0;
forever {
_errId = sqlite3_step(_stmt);
if (n < SQLITE_REPEAT_COUNT && firstStep && (_errId == SQLITE_LOCKED || _errId == SQLITE_BUSY)) {
sqlite3_reset(_stmt); // not necessary after sqlite version 3.6.23.1
n++;
OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
} else {
break;
}
}

NextResult result;
result.ok = _errId == SQLITE_ROW || _errId == SQLITE_DONE;
result.hasData = _errId == SQLITE_ROW;
if (!result.ok) {
_error = QString::fromUtf8(sqlite3_errmsg(_db));
qCWarning(lcSql) << "Sqlite step statement error:" << _errId << _error << "in" << _sql;
}

return result;
}

void SqlQuery::bindValue(int pos, const QVariant &value)
Expand Down
9 changes: 8 additions & 1 deletion src/common/ownsql.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,14 @@ class OCSYNC_EXPORT SqlQuery
bool isSelect();
bool isPragma();
bool exec();
bool next();

struct NextResult
{
bool ok = false;
bool hasData = false;
};
NextResult next();

void bindValue(int pos, const QVariant &value);
QString lastQuery() const;
int numRowsAffected();
Expand Down
95 changes: 61 additions & 34 deletions src/common/syncjournaldb.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ bool SyncJournalDb::checkConnect()
bool forceRemoteDiscovery = false;

SqlQuery versionQuery("SELECT major, minor, patch FROM version;", _db);
if (!versionQuery.next()) {
if (!versionQuery.next().hasData) {
// If there was no entry in the table, it means we are likely upgrading from 1.5
qCInfo(lcDb) << "possibleUpgradeFromMirall_1_5 detected!";
forceRemoteDiscovery = true;
Expand Down Expand Up @@ -829,7 +829,7 @@ QVector<QByteArray> SyncJournalDb::tableColumns(const QByteArray &table)
if (!query.exec()) {
return columns;
}
while (query.next()) {
while (query.next().hasData) {
columns.append(query.baValue(1));
}
qCDebug(lcDb) << "Columns in the current journal: " << columns;
Expand Down Expand Up @@ -984,15 +984,15 @@ bool SyncJournalDb::getFileRecord(const QByteArray &filename, SyncJournalFileRec
return false;
}

if (_getFileRecordQuery.next()) {
auto next = _getFileRecordQuery.next();
if (!next.ok) {
QString err = _getFileRecordQuery.error();
qCWarning(lcDb) << "No journal entry found for " << filename << "Error: " << err;
close();
return false;
}
if (next.hasData) {
fillFileRecordFromGetQuery(*rec, _getFileRecordQuery);
} else {
int errId = _getFileRecordQuery.errorId();
if (errId != SQLITE_DONE) { // only do this if the problem is different from SQLITE_DONE
QString err = _getFileRecordQuery.error();
qCWarning(lcDb) << "No journal entry found for " << filename << "Error: " << err;
close();
}
}
}
return true;
Expand Down Expand Up @@ -1021,7 +1021,10 @@ bool SyncJournalDb::getFileRecordByInode(quint64 inode, SyncJournalFileRecord *r
if (!_getFileRecordQueryByInode.exec())
return false;

if (_getFileRecordQueryByInode.next())
auto next = _getFileRecordQueryByInode.next();
if (!next.ok)
return false;
if (next.hasData)
fillFileRecordFromGetQuery(*rec, _getFileRecordQueryByInode);

return true;
Expand All @@ -1045,7 +1048,13 @@ bool SyncJournalDb::getFileRecordsByFileId(const QByteArray &fileId, const std::
if (!_getFileRecordQueryByFileId.exec())
return false;

while (_getFileRecordQueryByFileId.next()) {
forever {
auto next = _getFileRecordQueryByFileId.next();
if (!next.ok)
return false;
if (!next.hasData)
break;

SyncJournalFileRecord rec;
fillFileRecordFromGetQuery(rec, _getFileRecordQueryByFileId);
rowCallback(rec);
Expand Down Expand Up @@ -1097,7 +1106,13 @@ bool SyncJournalDb::getFilesBelowPath(const QByteArray &path, const std::functio
return false;
}

while (query->next()) {
forever {
auto next = query->next();
if (!next.ok)
return false;
if (!next.hasData)
break;

SyncJournalFileRecord rec;
fillFileRecordFromGetQuery(rec, *query);
rowCallback(rec);
Expand All @@ -1124,7 +1139,13 @@ bool SyncJournalDb::postSyncCleanup(const QSet<QString> &filepathsToKeep,

QByteArrayList superfluousItems;

while (query.next()) {
forever {
auto next = query.next();
if (!next.ok)
return false;
if (!next.hasData)
break;

const QString file = query.baValue(1);
bool keep = filepathsToKeep.contains(file);
if (!keep) {
Expand Down Expand Up @@ -1167,7 +1188,7 @@ int SyncJournalDb::getFileRecordCount()
return -1;
}

if (query.next()) {
if (query.next().hasData) {
int count = query.intValue(0);
return count;
}
Expand Down Expand Up @@ -1301,10 +1322,8 @@ SyncJournalDb::DownloadInfo SyncJournalDb::getDownloadInfo(const QString &file)
return res;
}

if (_getDownloadInfoQuery.next()) {
if (_getDownloadInfoQuery.next().hasData) {
toDownloadInfo(_getDownloadInfoQuery, &res);
} else {
res._valid = false;
}
}
return res;
Expand Down Expand Up @@ -1358,7 +1377,7 @@ QVector<SyncJournalDb::DownloadInfo> SyncJournalDb::getAndDeleteStaleDownloadInf
QStringList superfluousPaths;
QVector<SyncJournalDb::DownloadInfo> deleted_entries;

while (query.next()) {
while (query.next().hasData) {
const QString file = query.stringValue(3); // path
if (!keep.contains(file)) {
superfluousPaths.append(file);
Expand All @@ -1385,7 +1404,7 @@ int SyncJournalDb::downloadInfoCount()
if (!query.exec()) {
sqlFail("Count number of downloadinfo entries failed", query);
}
if (query.next()) {
if (query.next().hasData) {
re = query.intValue(0);
}
}
Expand All @@ -1410,7 +1429,7 @@ SyncJournalDb::UploadInfo SyncJournalDb::getUploadInfo(const QString &file)
return res;
}

if (_getUploadInfoQuery.next()) {
if (_getUploadInfoQuery.next().hasData) {
bool ok = true;
res._chunk = _getUploadInfoQuery.intValue(0);
res._transferid = _getUploadInfoQuery.intValue(1);
Expand Down Expand Up @@ -1479,7 +1498,7 @@ QVector<uint> SyncJournalDb::deleteStaleUploadInfos(const QSet<QString> &keep)

QStringList superfluousPaths;

while (query.next()) {
while (query.next().hasData) {
const QString file = query.stringValue(0);
if (!keep.contains(file)) {
superfluousPaths.append(file);
Expand All @@ -1503,7 +1522,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry(const QString
_getErrorBlacklistQuery.reset_and_clear_bindings();
_getErrorBlacklistQuery.bindValue(1, file);
if (_getErrorBlacklistQuery.exec()) {
if (_getErrorBlacklistQuery.next()) {
if (_getErrorBlacklistQuery.next().hasData) {
entry._lastTryEtag = _getErrorBlacklistQuery.baValue(0);
entry._lastTryModtime = _getErrorBlacklistQuery.int64Value(1);
entry._retryCount = _getErrorBlacklistQuery.intValue(2);
Expand Down Expand Up @@ -1539,7 +1558,7 @@ bool SyncJournalDb::deleteStaleErrorBlacklistEntries(const QSet<QString> &keep)

QStringList superfluousPaths;

while (query.next()) {
while (query.next().hasData) {
const QString file = query.stringValue(0);
if (!keep.contains(file)) {
superfluousPaths.append(file);
Expand All @@ -1562,7 +1581,7 @@ int SyncJournalDb::errorBlackListEntryCount()
if (!query.exec()) {
sqlFail("Count number of blacklist entries failed", query);
}
if (query.next()) {
if (query.next().hasData) {
re = query.intValue(0);
}
}
Expand Down Expand Up @@ -1666,7 +1685,7 @@ QVector<SyncJournalDb::PollInfo> SyncJournalDb::getPollInfos()
return res;
}

while (query.next()) {
while (query.next().hasData) {
PollInfo info;
info._file = query.stringValue(0);
info._modtime = query.int64Value(1);
Expand Down Expand Up @@ -1720,7 +1739,15 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList
*ok = false;
return result;
}
while (_getSelectiveSyncListQuery.next()) {
forever {
auto next = _getSelectiveSyncListQuery.next();
if (!next.ok) {
*ok = false;
return result;
}
if (!next.hasData)
break;

auto entry = _getSelectiveSyncListQuery.stringValue(0);
if (!entry.endsWith(QLatin1Char('/'))) {
entry.append(QLatin1Char('/'));
Expand Down Expand Up @@ -1843,12 +1870,12 @@ QByteArray SyncJournalDb::getChecksumType(int checksumTypeId)
return {};
query.bindValue(1, checksumTypeId);
if (!query.exec()) {
return 0;
return QByteArray();
}

if (!query.next()) {
if (!query.next().hasData) {
qCWarning(lcDb) << "No checksum type mapping found for" << checksumTypeId;
return 0;
return QByteArray();
}
return query.baValue(0);
}
Expand All @@ -1875,7 +1902,7 @@ int SyncJournalDb::mapChecksumType(const QByteArray &checksumType)
return 0;
}

if (!_getChecksumTypeIdQuery.next()) {
if (!_getChecksumTypeIdQuery.next().hasData) {
qCWarning(lcDb) << "No checksum type mapping found for" << checksumType;
return 0;
}
Expand All @@ -1896,7 +1923,7 @@ QByteArray SyncJournalDb::dataFingerprint()
return QByteArray();
}

if (!_getDataFingerprintQuery.next()) {
if (!_getDataFingerprintQuery.next().hasData) {
return QByteArray();
}
return _getDataFingerprintQuery.baValue(0);
Expand Down Expand Up @@ -1951,7 +1978,7 @@ ConflictRecord SyncJournalDb::conflictRecord(const QByteArray &path)
ASSERT(query.initOrReset(QByteArrayLiteral("SELECT baseFileId, baseModtime, baseEtag, basePath FROM conflicts WHERE path=?1;"), _db));
query.bindValue(1, path);
ASSERT(query.exec());
if (!query.next())
if (!query.next().hasData)
return entry;

entry.path = path;
Expand Down Expand Up @@ -1984,7 +2011,7 @@ QByteArrayList SyncJournalDb::conflictRecordPaths()
ASSERT(query.exec());

QByteArrayList paths;
while (query.next())
while (query.next().hasData)
paths.append(query.baValue(0));

return paths;
Expand Down
8 changes: 4 additions & 4 deletions test/testownsql.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ private slots:
q.prepare(sql);

q.exec();
while( q.next() ) {
while( q.next().hasData ) {
qDebug() << "Name: " << q.stringValue(1);
qDebug() << "Address: " << q.stringValue(2);
}
Expand All @@ -83,7 +83,7 @@ private slots:
q.prepare(sql);
q.bindValue(1, 2);
q.exec();
if( q.next() ) {
if( q.next().hasData ) {
qDebug() << "Name:" << q.stringValue(1);
qDebug() << "Address:" << q.stringValue(2);
}
Expand All @@ -96,7 +96,7 @@ private slots:
int rc = q.prepare(sql);
qDebug() << "Pragma:" << rc;
q.exec();
if( q.next() ) {
if( q.next().hasData ) {
qDebug() << "P:" << q.stringValue(1);
}
}
Expand All @@ -118,7 +118,7 @@ private slots:
SqlQuery q(_db);
q.prepare(sql);

if(q.next()) {
if(q.next().hasData) {
QString name = q.stringValue(1);
QString address = q.stringValue(2);
QVERIFY( name == QString::fromUtf8("пятницы") );
Expand Down
2 changes: 1 addition & 1 deletion test/testpermissions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ static void assertCsyncJournalOk(SyncJournalDb &journal)
QVERIFY(db.openReadOnly(journal.databaseFilePath()));
SqlQuery q("SELECT count(*) from metadata where length(fileId) == 0", db);
QVERIFY(q.exec());
QVERIFY(q.next());
QVERIFY(q.next().hasData);
QCOMPARE(q.intValue(0), 0);
#if defined(Q_OS_WIN) // Make sure the file does not appear in the FileInfo
FileSystem::setFileHidden(journal.databaseFilePath() + "-shm", true);
Expand Down

0 comments on commit 7eeb86f

Please sign in to comment.