From 816096311027bac0f391ea97d0b61b4175e9b8f9 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Wed, 14 Jun 2017 12:14:46 +0200 Subject: [PATCH] Compare the hash of files with identical mtime/size #5589 * For conflicts where mtime and size are identical: a) If there's no remote checksum, skip (unchanged) b) If there's a remote checksum that's a useful hash, create a PropagateDownload job and compute the local hash. If the hashes are identical, don't download the file and just update metadata. * Avoid exposing the existence of checksumTypeId beyond the database layer. This makes handling checksums easier in general because they can usually be treated as a single blob. This change was prompted by the difficulty of producing file_stat_t entries uniformly from PROPFINDs and the database. --- csync/src/csync.c | 5 +-- csync/src/csync.h | 11 +++-- csync/src/csync_private.h | 7 ++- csync/src/csync_reconcile.c | 41 +++++++++++++++-- csync/src/csync_statedb.c | 20 +++++---- csync/src/csync_update.c | 35 ++++++++------- csync/src/vio/csync_vio_file_stat.c | 4 ++ src/libsync/checksums.cpp | 48 ++++++++++---------- src/libsync/checksums.h | 14 +++--- src/libsync/discoveryphase.cpp | 30 ++++++++++++- src/libsync/propagatedownload.cpp | 47 ++++++++++++++++++-- src/libsync/propagatedownload.h | 47 +++++++++++++++++++- src/libsync/propagateremotemove.cpp | 3 +- src/libsync/propagateupload.cpp | 20 ++++----- src/libsync/propagateupload.h | 3 +- src/libsync/propagateuploadng.cpp | 5 +-- src/libsync/propagateuploadv1.cpp | 5 +-- src/libsync/propagatorjobs.cpp | 3 +- src/libsync/syncengine.cpp | 10 +++-- src/libsync/syncfileitem.h | 9 +++- src/libsync/syncjournaldb.cpp | 16 +++---- src/libsync/syncjournalfilerecord.cpp | 9 ++-- src/libsync/syncjournalfilerecord.h | 3 +- test/testsyncengine.cpp | 64 +++++++++++++++++++++++++++ test/testsyncjournaldb.cpp | 14 +++--- 25 files changed, 345 insertions(+), 128 deletions(-) diff --git a/csync/src/csync.c b/csync/src/csync.c index abac02efe..9af6f0fcf 100644 --- a/csync/src/csync.c +++ b/csync/src/csync.c @@ -376,8 +376,7 @@ static int _csync_treewalk_visitor(void *obj, void *data) { trav.error_status = cur->error_status; trav.has_ignored_files = cur->has_ignored_files; - trav.checksum = cur->checksum; - trav.checksumTypeId = cur->checksumTypeId; + trav.checksumHeader = cur->checksumHeader; if( other_node ) { csync_file_stat_t *other_stat = (csync_file_stat_t*)other_node->data; @@ -670,7 +669,7 @@ void csync_file_stat_free(csync_file_stat_t *st) SAFE_FREE(st->directDownloadCookies); SAFE_FREE(st->etag); SAFE_FREE(st->destpath); - SAFE_FREE(st->checksum); + SAFE_FREE(st->checksumHeader); SAFE_FREE(st); } } diff --git a/csync/src/csync.h b/csync/src/csync.h index c8664a378..949c9efd2 100644 --- a/csync/src/csync.h +++ b/csync/src/csync.h @@ -223,6 +223,10 @@ struct csync_vio_file_stat_s { enum csync_vio_file_flags_e flags; char *original_name; // only set if locale conversion fails + + // For remote file stats: the highest quality checksum the server provided + // in the "SHA1:324315da2143" form. + char *checksumHeader; }; csync_vio_file_stat_t OCSYNC_EXPORT *csync_vio_file_stat_new(void); @@ -262,8 +266,7 @@ struct csync_tree_walk_file_s { char *directDownloadUrl; char *directDownloadCookies; - const char *checksum; - uint32_t checksumTypeId; + const char *checksumHeader; struct { int64_t size; @@ -304,8 +307,8 @@ typedef int (*csync_vio_stat_hook) (csync_vio_handle_t *dhhandle, void *userdata); /* Compute the checksum of the given \a checksumTypeId for \a path. */ -typedef const char* (*csync_checksum_hook) ( - const char *path, uint32_t checksumTypeId, void *userdata); +typedef const char *(*csync_checksum_hook)( + const char *path, const char *otherChecksumHeader, void *userdata); /** * @brief Allocate a csync context. diff --git a/csync/src/csync_private.h b/csync/src/csync_private.h index a10b09f6f..11775f345 100644 --- a/csync/src/csync_private.h +++ b/csync/src/csync_private.h @@ -191,8 +191,11 @@ struct csync_file_stat_s { char *directDownloadCookies; char remotePerm[REMOTE_PERM_BUF_SIZE+1]; - const char *checksum; - uint32_t checksumTypeId; + // In the local tree, this can hold a checksum and its type if it is + // computed during discovery for some reason. + // In the remote tree, this will have the server checksum, if available. + // In both cases, the format is "SHA1:baff". + const char *checksumHeader; CSYNC_STATUS error_status; diff --git a/csync/src/csync_reconcile.c b/csync/src/csync_reconcile.c index 9d716a80c..59156e800 100644 --- a/csync/src/csync_reconcile.c +++ b/csync/src/csync_reconcile.c @@ -65,6 +65,19 @@ static c_rbnode_t *_csync_check_ignored(c_rbtree_t *tree, const char *path, int } } +/* Returns true if we're reasonably certain that hash equality + * for the header means content equality. + * + * Cryptographic safety is not required - this is mainly + * intended to rule out checksums like Adler32 that are not intended for + * hashing and have high likelihood of collision with particular inputs. + */ +static bool _csync_is_collision_safe_hash(const char *checksum_header) +{ + return strncmp(checksum_header, "SHA1:", 5) == 0 + || strncmp(checksum_header, "MD5:", 4) == 0; +} + /** * The main function in the reconcile pass. * @@ -272,11 +285,31 @@ static int _csync_merge_algorithm_visitor(void *obj, void *data) { // Folders of the same path are always considered equals is_conflict = false; } else { + // If the size or mtime is different, it's definitely a conflict. is_conflict = ((other->size != cur->size) || (other->modtime != cur->modtime)); - // FIXME: do a binary comparision of the file here because of the following - // edge case: - // The files could still have different content, even though the mtime - // and size are the same. + + // It could be a conflict even if size and mtime match! + // + // In older client versions we always treated these cases as a + // non-conflict. This behavior is preserved in case the server + // doesn't provide a suitable content hash. + // + // When it does have one, however, we do create a job, but the job + // will compare hashes and avoid the download if they are equal. + const char *remoteChecksumHeader = + (ctx->current == REMOTE_REPLICA ? cur->checksumHeader : other->checksumHeader); + if (remoteChecksumHeader) { + is_conflict |= _csync_is_collision_safe_hash(remoteChecksumHeader); + } + + // SO: If there is no checksum, we can have !is_conflict here + // even though the files have different content! This is an + // intentional tradeoff. Downloading and comparing files would + // be technically correct in this situation but leads to too + // much waste. + // In particular this kind of NEW/NEW situation with identical + // sizes and mtimes pops up when the local database is lost for + // whatever reason. } if (ctx->current == REMOTE_REPLICA) { // If the files are considered equal, only update the DB with the etag from remote diff --git a/csync/src/csync_statedb.c b/csync/src/csync_statedb.c index 9b87985a8..056ca48dd 100644 --- a/csync/src/csync_statedb.c +++ b/csync/src/csync_statedb.c @@ -225,7 +225,12 @@ int csync_statedb_close(CSYNC *ctx) { return rc; } -#define METADATA_COLUMNS "phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, contentChecksum, contentChecksumTypeId" +#define METADATA_QUERY \ + "phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, " \ + "filesize, ignoredChildrenRemote, " \ + "contentchecksumtype.name || ':' || contentChecksum " \ + "FROM metadata " \ + "LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id" // This funciton parses a line from the metadata table into the given csync_file_stat // structure which it is also allocating. @@ -287,9 +292,8 @@ static int _csync_file_stat_from_metadata_table( csync_file_stat_t **st, sqlite3 if(column_count > 13) { (*st)->has_ignored_files = sqlite3_column_int(stmt, 13); } - if(column_count > 15 && sqlite3_column_int(stmt, 15)) { - (*st)->checksum = c_strdup( (char*) sqlite3_column_text(stmt, 14)); - (*st)->checksumTypeId = sqlite3_column_int(stmt, 15); + if (column_count > 14 && sqlite3_column_text(stmt, 14)) { + (*st)->checksumHeader = c_strdup((char *)sqlite3_column_text(stmt, 14)); } } @@ -313,7 +317,7 @@ csync_file_stat_t *csync_statedb_get_stat_by_hash(CSYNC *ctx, } if( ctx->statedb.by_hash_stmt == NULL ) { - const char *hash_query = "SELECT " METADATA_COLUMNS " FROM metadata WHERE phash=?1"; + const char *hash_query = "SELECT " METADATA_QUERY " WHERE phash=?1"; SQLITE_BUSY_HANDLED(sqlite3_prepare_v2(ctx->statedb.db, hash_query, strlen(hash_query), &ctx->statedb.by_hash_stmt, NULL)); ctx->statedb.lastReturnValue = rc; @@ -356,7 +360,7 @@ csync_file_stat_t *csync_statedb_get_stat_by_file_id(CSYNC *ctx, } if( ctx->statedb.by_fileid_stmt == NULL ) { - const char *query = "SELECT " METADATA_COLUMNS " FROM metadata WHERE fileid=?1"; + const char *query = "SELECT " METADATA_QUERY " WHERE fileid=?1"; SQLITE_BUSY_HANDLED(sqlite3_prepare_v2(ctx->statedb.db, query, strlen(query), &ctx->statedb.by_fileid_stmt, NULL)); ctx->statedb.lastReturnValue = rc; @@ -396,7 +400,7 @@ csync_file_stat_t *csync_statedb_get_stat_by_inode(CSYNC *ctx, } if( ctx->statedb.by_inode_stmt == NULL ) { - const char *inode_query = "SELECT " METADATA_COLUMNS " FROM metadata WHERE inode=?1"; + const char *inode_query = "SELECT " METADATA_QUERY " WHERE inode=?1"; SQLITE_BUSY_HANDLED(sqlite3_prepare_v2(ctx->statedb.db, inode_query, strlen(inode_query), &ctx->statedb.by_inode_stmt, NULL)); ctx->statedb.lastReturnValue = rc; @@ -439,7 +443,7 @@ int csync_statedb_get_below_path( CSYNC *ctx, const char *path ) { * In other words, anything that is between path+'/' and path+'0', * (because '0' follows '/' in ascii) */ - const char *below_path_query = "SELECT " METADATA_COLUMNS " FROM metadata WHERE path > (?||'/') AND path < (?||'0') ORDER BY path||'/' ASC"; + const char *below_path_query = "SELECT " METADATA_QUERY " WHERE path > (?||'/') AND path < (?||'0') ORDER BY path||'/' ASC"; SQLITE_BUSY_HANDLED(sqlite3_prepare_v2(ctx->statedb.db, below_path_query, -1, &stmt, NULL)); ctx->statedb.lastReturnValue = rc; if( rc != SQLITE_OK ) { diff --git a/csync/src/csync_update.c b/csync/src/csync_update.c index 195152912..d7587765c 100644 --- a/csync/src/csync_update.c +++ b/csync/src/csync_update.c @@ -287,16 +287,15 @@ static int _csync_detect_update(CSYNC *ctx, const char *file, // Checksum comparison at this stage is only enabled for .eml files, // check #4754 #4755 bool isEmlFile = csync_fnmatch("*.eml", file, FNM_CASEFOLD) == 0; - if (isEmlFile && fs->size == tmp->size && tmp->checksumTypeId) { + if (isEmlFile && fs->size == tmp->size && tmp->checksumHeader) { if (ctx->callbacks.checksum_hook) { - st->checksum = ctx->callbacks.checksum_hook( - file, tmp->checksumTypeId, - ctx->callbacks.checksum_userdata); + st->checksumHeader = ctx->callbacks.checksum_hook( + file, tmp->checksumHeader, + ctx->callbacks.checksum_userdata); } bool checksumIdentical = false; - if (st->checksum) { - st->checksumTypeId = tmp->checksumTypeId; - checksumIdentical = strncmp(st->checksum, tmp->checksum, 1000) == 0; + if (st->checksumHeader) { + checksumIdentical = strncmp(st->checksumHeader, tmp->checksumHeader, 1000) == 0; } if (checksumIdentical) { CSYNC_LOG(CSYNC_LOG_PRIORITY_TRACE, "NOTE: Checksums are identical, file did not actually change: %s", path); @@ -381,15 +380,14 @@ static int _csync_detect_update(CSYNC *ctx, const char *file, // Verify the checksum where possible - if (isRename && tmp->checksumTypeId && ctx->callbacks.checksum_hook - && fs->type == CSYNC_VIO_FILE_TYPE_REGULAR) { - st->checksum = ctx->callbacks.checksum_hook( - file, tmp->checksumTypeId, - ctx->callbacks.checksum_userdata); - if (st->checksum) { - CSYNC_LOG(CSYNC_LOG_PRIORITY_TRACE, "checking checksum of potential rename %s %s <-> %s", path, st->checksum, tmp->checksum); - st->checksumTypeId = tmp->checksumTypeId; - isRename = strncmp(st->checksum, tmp->checksum, 1000) == 0; + if (isRename && tmp->checksumHeader && ctx->callbacks.checksum_hook + && fs->type == CSYNC_VIO_FILE_TYPE_REGULAR) { + st->checksumHeader = ctx->callbacks.checksum_hook( + file, tmp->checksumHeader, + ctx->callbacks.checksum_userdata); + if (st->checksumHeader) { + CSYNC_LOG(CSYNC_LOG_PRIORITY_TRACE, "checking checksum of potential rename %s %s <-> %s", path, st->checksumHeader, tmp->checksumHeader); + isRename = strncmp(st->checksumHeader, tmp->checksumHeader, 1000) == 0; } } @@ -506,6 +504,11 @@ out: strncpy(st->remotePerm, fs->remotePerm, REMOTE_PERM_BUF_SIZE); } + // For the remote: propagate the discovered checksum + if (fs->checksumHeader && ctx->current == REMOTE_REPLICA) { + st->checksumHeader = c_strdup(fs->checksumHeader); + } + st->phash = h; st->pathlen = len; memcpy(st->path, (len ? path : ""), len + 1); diff --git a/csync/src/vio/csync_vio_file_stat.c b/csync/src/vio/csync_vio_file_stat.c index bc935d375..8ebb64f33 100644 --- a/csync/src/vio/csync_vio_file_stat.c +++ b/csync/src/vio/csync_vio_file_stat.c @@ -40,6 +40,9 @@ csync_vio_file_stat_t* csync_vio_file_stat_copy(csync_vio_file_stat_t *file_stat if (file_stat_cpy->directDownloadUrl) { file_stat_cpy->directDownloadUrl = c_strdup(file_stat_cpy->directDownloadUrl); } + if (file_stat_cpy->checksumHeader) { + file_stat_cpy->checksumHeader = c_strdup(file_stat_cpy->checksumHeader); + } file_stat_cpy->name = c_strdup(file_stat_cpy->name); return file_stat_cpy; } @@ -57,6 +60,7 @@ void csync_vio_file_stat_destroy(csync_vio_file_stat_t *file_stat) { SAFE_FREE(file_stat->directDownloadCookies); SAFE_FREE(file_stat->name); SAFE_FREE(file_stat->original_name); + SAFE_FREE(file_stat->checksumHeader); SAFE_FREE(file_stat); } diff --git a/src/libsync/checksums.cpp b/src/libsync/checksums.cpp index 00818442f..eaf36379c 100644 --- a/src/libsync/checksums.cpp +++ b/src/libsync/checksums.cpp @@ -81,6 +81,8 @@ Q_LOGGING_CATEGORY(lcChecksums, "sync.checksums", QtInfoMsg) QByteArray makeChecksumHeader(const QByteArray &checksumType, const QByteArray &checksum) { + if (checksumType.isEmpty() || checksum.isEmpty()) + return QByteArray(); QByteArray header = checksumType; header.append(':'); header.append(checksum); @@ -105,6 +107,16 @@ bool parseChecksumHeader(const QByteArray &header, QByteArray *type, QByteArray return true; } + +QByteArray parseChecksumHeaderType(const QByteArray &header) +{ + const auto idx = header.indexOf(':'); + if (idx < 0) { + return QByteArray(); + } + return header.left(idx); +} + bool uploadChecksumEnabled() { static bool enabled = qgetenv("OWNCLOUD_DISABLE_CHECKSUM_UPLOAD").isEmpty(); @@ -214,39 +226,27 @@ void ValidateChecksumHeader::slotChecksumCalculated(const QByteArray &checksumTy emit validated(checksumType, checksum); } -CSyncChecksumHook::CSyncChecksumHook(SyncJournalDb *journal) - : _journal(journal) +CSyncChecksumHook::CSyncChecksumHook() { } -const char *CSyncChecksumHook::hook(const char *path, uint32_t checksumTypeId, void *this_obj) +const char *CSyncChecksumHook::hook(const char *path, const char *otherChecksumHeader, void * /*this_obj*/) { - CSyncChecksumHook *checksumHook = static_cast(this_obj); - QByteArray checksum = checksumHook->compute(QString::fromUtf8(path), checksumTypeId); + QByteArray type = parseChecksumHeaderType(QByteArray(otherChecksumHeader)); + if (type.isEmpty()) + return NULL; + + QByteArray checksum = ComputeChecksum::computeNow(path, type); if (checksum.isNull()) { + qCWarning(lcChecksums) << "Failed to compute checksum" << type << "for" << path; return NULL; } - char *result = (char *)malloc(checksum.size() + 1); - memcpy(result, checksum.constData(), checksum.size()); - result[checksum.size()] = 0; + QByteArray checksumHeader = makeChecksumHeader(type, checksum); + char *result = (char *)malloc(checksumHeader.size() + 1); + memcpy(result, checksumHeader.constData(), checksumHeader.size()); + result[checksumHeader.size()] = 0; return result; } -QByteArray CSyncChecksumHook::compute(const QString &path, int checksumTypeId) -{ - QByteArray checksumType = _journal->getChecksumType(checksumTypeId); - if (checksumType.isEmpty()) { - qCWarning(lcChecksums) << "Checksum type" << checksumTypeId << "not found"; - return QByteArray(); - } - - QByteArray checksum = ComputeChecksum::computeNow(path, checksumType); - if (checksum.isNull()) { - qCWarning(lcChecksums) << "Failed to compute checksum" << checksumType << "for" << path; - return QByteArray(); - } - - return checksum; -} } diff --git a/src/libsync/checksums.h b/src/libsync/checksums.h index 5d88b0bcb..a7259f7ec 100644 --- a/src/libsync/checksums.h +++ b/src/libsync/checksums.h @@ -31,6 +31,9 @@ QByteArray makeChecksumHeader(const QByteArray &checksumType, const QByteArray & /// Parses a checksum header bool parseChecksumHeader(const QByteArray &header, QByteArray *type, QByteArray *checksum); +/// Convenience for getting the type from a checksum header, null if none +QByteArray parseChecksumHeaderType(const QByteArray &header); + /// Checks OWNCLOUD_DISABLE_CHECKSUM_UPLOAD bool uploadChecksumEnabled(); @@ -119,20 +122,15 @@ class OWNCLOUDSYNC_EXPORT CSyncChecksumHook : public QObject { Q_OBJECT public: - explicit CSyncChecksumHook(SyncJournalDb *journal); + explicit CSyncChecksumHook(); /** - * Returns the checksum value for \a path for the given \a checksumTypeId. + * Returns the checksum value for \a path that is comparable to \a otherChecksum. * * Called from csync, where a instance of CSyncChecksumHook has * to be set as userdata. * The return value will be owned by csync. */ - static const char *hook(const char *path, uint32_t checksumTypeId, void *this_obj); - - QByteArray compute(const QString &path, int checksumTypeId); - -private: - SyncJournalDb *_journal; + static const char *hook(const char *path, const char *otherChecksumHeader, void *this_obj); }; } diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index c8f13784b..16b98faeb 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -272,7 +272,8 @@ void DiscoverySingleDirectoryJob::start() << "http://owncloud.org/ns:id" << "http://owncloud.org/ns:downloadURL" << "http://owncloud.org/ns:dDC" - << "http://owncloud.org/ns:permissions"; + << "http://owncloud.org/ns:permissions" + << "http://owncloud.org/ns:checksums"; if (_isRootPath) props << "http://owncloud.org/ns:data-fingerprint"; @@ -294,6 +295,28 @@ void DiscoverySingleDirectoryJob::abort() } } +/** + * Returns the highest-quality checksum in a 'checksums' + * property retrieved from the server. + * + * Example: "ADLER32:1231 SHA1:ab124124 MD5:2131affa21" + * -> "SHA1:ab124124" + */ +static QByteArray findBestChecksum(const QByteArray &checksums) +{ + int i = 0; + // The order of the searches here defines the preference ordering. + if (-1 != (i = checksums.indexOf("SHA1:")) + || -1 != (i = checksums.indexOf("MD5:")) + || -1 != (i = checksums.indexOf("Adler32:"))) { + // Now i is the start of the best checksum + // Grab it until the next space or end of string. + auto checksum = checksums.mid(i); + return checksum.mid(0, checksum.indexOf(" ")); + } + return QByteArray(); +} + static csync_vio_file_stat_t *propertyMapToFileStat(const QMap &map) { csync_vio_file_stat_t *file_stat = csync_vio_file_stat_new(); @@ -343,6 +366,11 @@ static csync_vio_file_stat_t *propertyMapToFileStat(const QMap } else { qCWarning(lcDiscovery) << "permissions too large" << v; } + } else if (property == "checksums") { + QByteArray checksum = findBestChecksum(value.toUtf8()); + if (!checksum.isEmpty()) { + file_stat->checksumHeader = strdup(checksum.constData()); + } } } diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index e66936908..8cc87ccac 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -344,6 +344,41 @@ void PropagateDownloadFile::start() } } + // If we have a conflict where size and mtime are identical, + // compare the remote checksum to the local one. + // Maybe it's not a real conflict and no download is necessary! + if (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT + && _item->_size == _item->log._other_size + && _item->_modtime == _item->log._other_modtime + && !_item->_checksumHeader.isEmpty()) { + qCDebug(lcPropagateDownload) << _item->_file << "may not need download, computing checksum"; + auto computeChecksum = new ComputeChecksum(this); + computeChecksum->setChecksumType(parseChecksumHeaderType(_item->_checksumHeader)); + connect(computeChecksum, SIGNAL(done(QByteArray, QByteArray)), + SLOT(conflictChecksumComputed(QByteArray, QByteArray))); + computeChecksum->start(propagator()->getFilePath(_item->_file)); + return; + } + + startDownload(); +} + +void PropagateDownloadFile::conflictChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum) +{ + if (makeChecksumHeader(checksumType, checksum) == _item->_checksumHeader) { + qCDebug(lcPropagateDownload) << _item->_file << "remote and local checksum match"; + // No download necessary, just update metadata + updateMetadata(/*isConflict=*/false); + return; + } + startDownload(); +} + +void PropagateDownloadFile::startDownload() +{ + if (propagator()->_abortRequested.fetchAndAddRelaxed(0)) + return; + // do a klaas' case clash check. if (propagator()->localFileNameClash(_item->_file)) { done(SyncFileItem::NormalError, tr("File %1 can not be downloaded because of a local file name clash!").arg(QDir::toNativeSeparators(_item->_file))); @@ -672,7 +707,7 @@ namespace { // Anonymous namespace for the recall feature continue; } - qCInfo(lcPropagateDownload) << "Recalling" << localRecalledFile << "Checksum:" << record._contentChecksumType << record._contentChecksum; + qCInfo(lcPropagateDownload) << "Recalling" << localRecalledFile << "Checksum:" << record._checksumHeader; QString targetPath = makeRecallFileName(recalledFile); @@ -717,8 +752,7 @@ void PropagateDownloadFile::transmissionChecksumValidated(const QByteArray &chec void PropagateDownloadFile::contentChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum) { - _item->_contentChecksum = checksum; - _item->_contentChecksumType = checksumType; + _item->_checksumHeader = makeChecksumHeader(checksumType, checksum); downloadFinished(); } @@ -827,6 +861,13 @@ void PropagateDownloadFile::downloadFinished() // Get up to date information for the journal. _item->_size = FileSystem::getSize(fn); + updateMetadata(isConflict); +} + +void PropagateDownloadFile::updateMetadata(bool isConflict) +{ + QString fn = propagator()->getFilePath(_item->_file); + if (!propagator()->_journal->setFileRecord(SyncJournalFileRecord(*_item, fn))) { done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); return; diff --git a/src/libsync/propagatedownload.h b/src/libsync/propagatedownload.h index 69962131b..26361541c 100644 --- a/src/libsync/propagatedownload.h +++ b/src/libsync/propagatedownload.h @@ -106,6 +106,40 @@ private slots: /** * @brief The PropagateDownloadFile class * @ingroup libsync + * + * This is the flow: + +\code{.unparsed} + start() + | + | deleteExistingFolder() if enabled + | + +--> mtime and size identical? + | then compute the local checksum + | done?-> conflictChecksumComputed() + | | + | checksum differs? | + +-> startDownload() <--------------------------+ + | | + +-> run a GETFileJob | checksum identical? + | + done?-> slotGetFinished() | + | | + +-> validate checksum header | + | + done?-> transmissionChecksumValidated() | + | | + +-> compute the content checksum | + | + done?-> contentChecksumComputed() | + | | + +-> downloadFinished() | + | | + +------------------+ | + | | + +-> updateMetadata() <-------------------------+ + +\endcode */ class PropagateDownloadFile : public PropagateItemJob { @@ -136,11 +170,22 @@ public: void setDeleteExistingFolder(bool enabled); private slots: + /// Called when ComputeChecksum on the local file finishes, + /// maybe the local and remote checksums are identical? + void conflictChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum); + /// Called to start downloading the remote file + void startDownload(); + /// Called when the GETFileJob finishes void slotGetFinished(); - void abort() Q_DECL_OVERRIDE; + /// Called when the download's checksum header was validated void transmissionChecksumValidated(const QByteArray &checksumType, const QByteArray &checksum); + /// Called when the download's checksum computation is done void contentChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum); void downloadFinished(); + /// Called when it's time to update the db metadata + void updateMetadata(bool isConflict); + + void abort() Q_DECL_OVERRIDE; void slotDownloadProgress(qint64, qint64); void slotChecksumFail(const QString &errMsg); diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index 7f4f96c33..69979b1a9 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -173,8 +173,7 @@ void PropagateRemoteMove::finalize() SyncJournalFileRecord record(*_item, propagator()->getFilePath(_item->_renameTarget)); record._path = _item->_renameTarget; if (oldRecord.isValid()) { - record._contentChecksum = oldRecord._contentChecksum; - record._contentChecksumType = oldRecord._contentChecksumType; + record._checksumHeader = oldRecord._checksumHeader; if (record._fileSize != oldRecord._fileSize) { qCWarning(lcPropagateRemoteMove) << "File sizes differ on server vs sync journal: " << record._fileSize << oldRecord._fileSize; diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 93849dff7..f3b0e8336 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -231,9 +231,10 @@ void PropagateUploadFileCommon::slotComputeContentChecksum() QByteArray checksumType = contentChecksumType(); // Maybe the discovery already computed the checksum? - if (_item->_contentChecksumType == checksumType - && !_item->_contentChecksum.isEmpty()) { - slotComputeTransmissionChecksum(checksumType, _item->_contentChecksum); + QByteArray existingChecksumType, existingChecksum; + parseChecksumHeader(_item->_checksumHeader, &existingChecksumType, &existingChecksum); + if (existingChecksumType == checksumType) { + slotComputeTransmissionChecksum(checksumType, existingChecksum); return; } @@ -250,8 +251,7 @@ void PropagateUploadFileCommon::slotComputeContentChecksum() void PropagateUploadFileCommon::slotComputeTransmissionChecksum(const QByteArray &contentChecksumType, const QByteArray &contentChecksum) { - _item->_contentChecksum = contentChecksum; - _item->_contentChecksumType = contentChecksumType; + _item->_checksumHeader = makeChecksumHeader(contentChecksumType, contentChecksum); #ifdef WITH_TESTING _stopWatch.addLapTime(QLatin1String("ContentChecksum")); @@ -288,13 +288,11 @@ void PropagateUploadFileCommon::slotStartUpload(const QByteArray &transmissionCh // When we start chunks, we will add it again, once for every chunks. propagator()->_activeJobList.removeOne(this); - _transmissionChecksum = transmissionChecksum; - _transmissionChecksumType = transmissionChecksumType; + _transmissionChecksumHeader = makeChecksumHeader(transmissionChecksumType, transmissionChecksum); - if (_item->_contentChecksum.isEmpty() && _item->_contentChecksumType.isEmpty()) { - // If the _contentChecksum was not set, reuse the transmission checksum as the content checksum. - _item->_contentChecksum = transmissionChecksum; - _item->_contentChecksumType = transmissionChecksumType; + // If no checksum header was not set, reuse the transmission checksum as the content checksum. + if (_item->_checksumHeader.isEmpty()) { + _item->_checksumHeader = _transmissionChecksumHeader; } const QString fullFilePath = propagator()->getFilePath(_item->_file); diff --git a/src/libsync/propagateupload.h b/src/libsync/propagateupload.h index e9130d934..5e16b5ff9 100644 --- a/src/libsync/propagateupload.h +++ b/src/libsync/propagateupload.h @@ -226,8 +226,7 @@ protected: Utility::StopWatch _stopWatch; #endif - QByteArray _transmissionChecksum; - QByteArray _transmissionChecksumType; + QByteArray _transmissionChecksumHeader; public: PropagateUploadFileCommon(OwncloudPropagator *propagator, const SyncFileItemPtr &item) diff --git a/src/libsync/propagateuploadng.cpp b/src/libsync/propagateuploadng.cpp index d6c213389..45768102d 100644 --- a/src/libsync/propagateuploadng.cpp +++ b/src/libsync/propagateuploadng.cpp @@ -283,9 +283,8 @@ void PropagateUploadFileNG::startNextChunk() if (!ifMatch.isEmpty()) { headers["If"] = "<" + destination.toUtf8() + "> ([" + ifMatch + "])"; } - if (!_transmissionChecksumType.isEmpty()) { - headers[checkSumHeaderC] = makeChecksumHeader( - _transmissionChecksumType, _transmissionChecksum); + if (!_transmissionChecksumHeader.isEmpty()) { + headers[checkSumHeaderC] = _transmissionChecksumHeader; } headers["OC-Total-Length"] = QByteArray::number(fileSize); diff --git a/src/libsync/propagateuploadv1.cpp b/src/libsync/propagateuploadv1.cpp index f8a88020d..b48b433ab 100644 --- a/src/libsync/propagateuploadv1.cpp +++ b/src/libsync/propagateuploadv1.cpp @@ -103,9 +103,8 @@ void PropagateUploadFileV1::startNextChunk() } qCDebug(lcPropagateUpload) << _chunkCount << isFinalChunk << chunkStart << currentChunkSize; - if (isFinalChunk && !_transmissionChecksumType.isEmpty()) { - headers[checkSumHeaderC] = makeChecksumHeader( - _transmissionChecksumType, _transmissionChecksum); + if (isFinalChunk && !_transmissionChecksumHeader.isEmpty()) { + headers[checkSumHeaderC] = _transmissionChecksumHeader; } const QString fileName = propagator()->getFilePath(_item->_file); diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index bda94fa11..5cf17b26a 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -241,8 +241,7 @@ void PropagateLocalRename::start() SyncJournalFileRecord record(*_item, targetFile); record._path = _item->_renameTarget; if (oldRecord.isValid()) { - record._contentChecksum = oldRecord._contentChecksum; - record._contentChecksumType = oldRecord._contentChecksumType; + record._checksumHeader = oldRecord._checksumHeader; } if (!_item->_isDirectory) { // Directories are saved at the end diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index 00a795976..d65646ff8 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -73,7 +73,6 @@ SyncEngine::SyncEngine(AccountPtr account, const QString &localPath, , _backInTimeFiles(0) , _uploadLimit(0) , _downloadLimit(0) - , _checksum_hook(journal) , _anotherSyncNeeded(NoFollowUpSync) { qRegisterMetaType("SyncFileItem"); @@ -426,9 +425,12 @@ int SyncEngine::treewalkFile(TREE_WALK_FILE *file, bool remote) } // Sometimes the discovery computes checksums for local files - if (!remote && file->checksum && file->checksumTypeId) { - item->_contentChecksum = QByteArray(file->checksum); - item->_contentChecksumType = _journal->getChecksumType(file->checksumTypeId); + if (!remote && file->checksumHeader) { + item->_checksumHeader = QByteArray(file->checksumHeader); + } + // For conflicts, store the remote checksum there + if (remote && item->_instruction == CSYNC_INSTRUCTION_CONFLICT && file->checksumHeader) { + item->_checksumHeader = QByteArray(file->checksumHeader); } // record the seen files to be able to clean the journal later diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h index 6273dd5c9..3eefac5ce 100644 --- a/src/libsync/syncfileitem.h +++ b/src/libsync/syncfileitem.h @@ -190,8 +190,13 @@ public: quint64 _inode; QByteArray _fileId; QByteArray _remotePerm; - QByteArray _contentChecksum; - QByteArray _contentChecksumType; + + // When is this set, and is it the local or the remote checksum? + // - if mtime or size changed locally for *.eml files (local checksum) + // - for potential renames of local files (local checksum) + // - for conflicts (remote checksum) (what about eval_rename/new reconcile?) + QByteArray _checksumHeader; + QString _directDownloadUrl; QString _directDownloadCookies; diff --git a/src/libsync/syncjournaldb.cpp b/src/libsync/syncjournaldb.cpp index c6b67fd06..3406c7e35 100644 --- a/src/libsync/syncjournaldb.cpp +++ b/src/libsync/syncjournaldb.cpp @@ -28,6 +28,7 @@ #include "version.h" #include "filesystem.h" #include "asserts.h" +#include "checksums.h" #include "../../csync/src/std/c_jhash.h" @@ -448,7 +449,7 @@ bool SyncJournalDb::checkConnect() _getFileRecordQuery.reset(new SqlQuery(_db)); if (_getFileRecordQuery->prepare( "SELECT path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize," - " ignoredChildrenRemote, contentChecksum, contentchecksumtype.name" + " ignoredChildrenRemote, contentchecksumtype.name || ':' || contentChecksum" " FROM metadata" " LEFT JOIN checksumtype as contentchecksumtype ON metadata.contentChecksumTypeId == contentchecksumtype.id" " WHERE phash=?1")) { @@ -832,7 +833,7 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) qCInfo(lcDb) << "Updating file record for path:" << record._path << "inode:" << record._inode << "modtime:" << record._modtime << "type:" << record._type << "etag:" << record._etag << "fileId:" << record._fileId << "remotePerm:" << record._remotePerm - << "fileSize:" << record._fileSize << "checksum:" << record._contentChecksum << record._contentChecksumType; + << "fileSize:" << record._fileSize << "checksum:" << record._checksumHeader; qlonglong phash = getPHash(record._path); if (checkConnect()) { @@ -848,7 +849,9 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) QString remotePerm(record._remotePerm); if (remotePerm.isEmpty()) remotePerm = QString(); // have NULL in DB (vs empty) - int contentChecksumTypeId = mapChecksumType(record._contentChecksumType); + QByteArray checksumType, checksum; + parseChecksumHeader(record._checksumHeader, &checksumType, &checksum); + int contentChecksumTypeId = mapChecksumType(checksumType); _setFileRecordQuery->reset_and_clear_bindings(); _setFileRecordQuery->bindValue(1, QString::number(phash)); _setFileRecordQuery->bindValue(2, plen); @@ -864,7 +867,7 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) _setFileRecordQuery->bindValue(12, remotePerm); _setFileRecordQuery->bindValue(13, record._fileSize); _setFileRecordQuery->bindValue(14, record._serverHasIgnoredFiles ? 1 : 0); - _setFileRecordQuery->bindValue(15, record._contentChecksum); + _setFileRecordQuery->bindValue(15, checksum); _setFileRecordQuery->bindValue(16, contentChecksumTypeId); if (!_setFileRecordQuery->exec()) { @@ -943,10 +946,7 @@ SyncJournalFileRecord SyncJournalDb::getFileRecord(const QString &filename) rec._remotePerm = _getFileRecordQuery->baValue(9); rec._fileSize = _getFileRecordQuery->int64Value(10); rec._serverHasIgnoredFiles = (_getFileRecordQuery->intValue(11) > 0); - rec._contentChecksum = _getFileRecordQuery->baValue(12); - if (!_getFileRecordQuery->nullValue(13)) { - rec._contentChecksumType = _getFileRecordQuery->baValue(13); - } + rec._checksumHeader = _getFileRecordQuery->baValue(12); _getFileRecordQuery->reset_and_clear_bindings(); } else { int errId = _getFileRecordQuery->errorId(); diff --git a/src/libsync/syncjournalfilerecord.cpp b/src/libsync/syncjournalfilerecord.cpp index 693778914..b2c65931d 100644 --- a/src/libsync/syncjournalfilerecord.cpp +++ b/src/libsync/syncjournalfilerecord.cpp @@ -47,8 +47,7 @@ SyncJournalFileRecord::SyncJournalFileRecord(const SyncFileItem &item, const QSt , _fileSize(item._size) , _remotePerm(item._remotePerm) , _serverHasIgnoredFiles(item._serverHasIgnoredFiles) - , _contentChecksum(item._contentChecksum) - , _contentChecksumType(item._contentChecksumType) + , _checksumHeader(item._checksumHeader) { // use the "old" inode coming with the item for the case where the // filesystem stat fails. That can happen if the the file was removed @@ -106,8 +105,7 @@ SyncFileItem SyncJournalFileRecord::toSyncFileItem() item._size = _fileSize; item._remotePerm = _remotePerm; item._serverHasIgnoredFiles = _serverHasIgnoredFiles; - item._contentChecksum = _contentChecksum; - item._contentChecksumType = _contentChecksumType; + item._checksumHeader = _checksumHeader; return item; } @@ -130,7 +128,6 @@ bool operator==(const SyncJournalFileRecord &lhs, && lhs._fileSize == rhs._fileSize && lhs._remotePerm == rhs._remotePerm && lhs._serverHasIgnoredFiles == rhs._serverHasIgnoredFiles - && lhs._contentChecksum == rhs._contentChecksum - && lhs._contentChecksumType == rhs._contentChecksumType; + && lhs._checksumHeader == rhs._checksumHeader; } } diff --git a/src/libsync/syncjournalfilerecord.h b/src/libsync/syncjournalfilerecord.h index f96e85b3b..c7579683b 100644 --- a/src/libsync/syncjournalfilerecord.h +++ b/src/libsync/syncjournalfilerecord.h @@ -57,8 +57,7 @@ public: qint64 _fileSize; QByteArray _remotePerm; bool _serverHasIgnoredFiles; - QByteArray _contentChecksum; - QByteArray _contentChecksumType; + QByteArray _checksumHeader; }; bool OWNCLOUDSYNC_EXPORT diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index 4200d5e1b..1f48931da 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -321,6 +321,70 @@ private slots: } } + void testFakeConflict() + { + FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; + + int nGET = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &) { + if (op == QNetworkAccessManager::GetOperation) + ++nGET; + return nullptr; + }); + + // For directly editing the remote checksum + FileInfo &remoteInfo = dynamic_cast(fakeFolder.remoteModifier()); + + // Base mtime with no ms content (filesystem is seconds only) + auto mtime = QDateTime::currentDateTime().addDays(-4); + mtime.setMSecsSinceEpoch(mtime.toMSecsSinceEpoch() / 1000 * 1000); + + // Conflict: Same content, mtime, but no server checksum + // -> ignored in reconcile + fakeFolder.localModifier().setContents("A/a1", 'C'); + fakeFolder.localModifier().setModTime("A/a1", mtime); + fakeFolder.remoteModifier().setContents("A/a1", 'C'); + fakeFolder.remoteModifier().setModTime("A/a1", mtime); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nGET, 0); + + // Conflict: Same content, mtime, but weak server checksum + // -> ignored in reconcile + mtime = mtime.addDays(1); + fakeFolder.localModifier().setContents("A/a1", 'D'); + fakeFolder.localModifier().setModTime("A/a1", mtime); + fakeFolder.remoteModifier().setContents("A/a1", 'D'); + fakeFolder.remoteModifier().setModTime("A/a1", mtime); + remoteInfo.find("A/a1")->checksums = "Adler32:bad"; + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nGET, 0); + + // Conflict: Same content, mtime, but server checksum differs + // -> downloaded + mtime = mtime.addDays(1); + fakeFolder.localModifier().setContents("A/a1", 'W'); + fakeFolder.localModifier().setModTime("A/a1", mtime); + fakeFolder.remoteModifier().setContents("A/a1", 'W'); + fakeFolder.remoteModifier().setModTime("A/a1", mtime); + remoteInfo.find("A/a1")->checksums = "SHA1:bad"; + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nGET, 1); + + // Conflict: Same content, mtime, matching checksums + // -> PropagateDownload, but it skips the download + mtime = mtime.addDays(1); + fakeFolder.localModifier().setContents("A/a1", 'C'); + fakeFolder.localModifier().setModTime("A/a1", mtime); + fakeFolder.remoteModifier().setContents("A/a1", 'C'); + fakeFolder.remoteModifier().setModTime("A/a1", mtime); + remoteInfo.find("A/a1")->checksums = "SHA1:56900fb1d337cf7237ff766276b9c1e8ce507427"; + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nGET, 1); + + // Extra sync reads from db, no difference + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(nGET, 1); + } }; QTEST_GUILESS_MAIN(TestSyncEngine) diff --git a/test/testsyncjournaldb.cpp b/test/testsyncjournaldb.cpp index fa6021acc..ec673077d 100644 --- a/test/testsyncjournaldb.cpp +++ b/test/testsyncjournaldb.cpp @@ -53,17 +53,15 @@ private slots: record._fileId = "abcd"; record._remotePerm = "744"; record._fileSize = 213089055; - record._contentChecksum = "mychecksum"; - record._contentChecksumType = "MD5"; + record._checksumHeader = "MD5:mychecksum"; QVERIFY(_db.setFileRecord(record)); SyncJournalFileRecord storedRecord = _db.getFileRecord("foo"); QVERIFY(storedRecord == record); // Update checksum - record._contentChecksum = "newchecksum"; - record._contentChecksumType = "Adler32"; - _db.updateFileRecordChecksum("foo", record._contentChecksum, record._contentChecksumType); + record._checksumHeader = "Adler32:newchecksum"; + _db.updateFileRecordChecksum("foo", "newchecksum", "Adler32"); storedRecord = _db.getFileRecord("foo"); QVERIFY(storedRecord == record); @@ -91,16 +89,14 @@ private slots: SyncJournalFileRecord record; record._path = "foo-checksum"; record._remotePerm = "744"; - record._contentChecksum = "mychecksum"; - record._contentChecksumType = "MD5"; + record._checksumHeader = "MD5:mychecksum"; record._modtime = QDateTime::currentDateTimeUtc(); QVERIFY(_db.setFileRecord(record)); SyncJournalFileRecord storedRecord = _db.getFileRecord("foo-checksum"); QVERIFY(storedRecord._path == record._path); QVERIFY(storedRecord._remotePerm == record._remotePerm); - QVERIFY(storedRecord._contentChecksum == record._contentChecksum); - QVERIFY(storedRecord._contentChecksumType == record._contentChecksumType); + QVERIFY(storedRecord._checksumHeader == record._checksumHeader); // qDebug()<< "OOOOO " << storedRecord._modtime.toTime_t() << record._modtime.toTime_t(); -- 2.30.2