diff --git a/src/csync/csync_reconcile.cpp b/src/csync/csync_reconcile.cpp index 92aa563fe09..b2283ad2ca0 100644 --- a/src/csync/csync_reconcile.cpp +++ b/src/csync/csync_reconcile.cpp @@ -63,18 +63,6 @@ static csync_file_stat_t *_csync_check_ignored(csync_s::FileMap *tree, const Byt } } -/* 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. @@ -297,14 +285,14 @@ static int _csync_merge_algorithm_visitor(csync_file_stat_t *cur, CSYNC * ctx) { // // 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. + // doesn't provide a content checksum. // // 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 = + // will compare hashes and avoid the download if possible. + QByteArray remoteChecksumHeader = (ctx->current == REMOTE_REPLICA ? cur->checksumHeader : other->checksumHeader); - if (remoteChecksumHeader) { - is_conflict |= _csync_is_collision_safe_hash(remoteChecksumHeader); + if (!remoteChecksumHeader.isEmpty()) { + is_conflict = true; } // SO: If there is no checksum, we can have !is_conflict here diff --git a/src/csync/csync_util.cpp b/src/csync/csync_util.cpp index afafafa5b41..b05228efeb1 100644 --- a/src/csync/csync_util.cpp +++ b/src/csync/csync_util.cpp @@ -211,3 +211,10 @@ time_t oc_httpdate_parse( const char *date ) { result = timegm(&gmt); return result; } + + +bool csync_is_collision_safe_hash(const QByteArray &checksum_header) +{ + return checksum_header.startsWith("SHA1:") + || checksum_header.startsWith("MD5:"); +} diff --git a/src/csync/csync_util.h b/src/csync/csync_util.h index f65ada5924b..57a5f12a052 100644 --- a/src/csync/csync_util.h +++ b/src/csync/csync_util.h @@ -31,4 +31,14 @@ const char OCSYNC_EXPORT *csync_instruction_str(enum csync_instructions_e instr) void OCSYNC_EXPORT csync_memstat_check(void); bool OCSYNC_EXPORT csync_file_locked_or_open( const char *dir, const char *fname); + +/* 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. + */ +bool OCSYNC_EXPORT csync_is_collision_safe_hash(const QByteArray &checksum_header); + #endif /* _CSYNC_UTIL_H */ diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 79c4893b6e4..3b8452505cb 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -357,13 +357,16 @@ void PropagateDownloadFile::start() } } - // If we have a conflict where size and mtime are identical, + // If we have a conflict where size of the file is unchanged, // compare the remote checksum to the local one. // Maybe it's not a real conflict and no download is necessary! + // If the hashes are collision safe and identical, we assume the content is too. + // For weak checksums, we only do that if the mtimes are also identical. if (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT && _item->_size == _item->_previousSize - && _item->_modtime == _item->_previousModtime - && !_item->_checksumHeader.isEmpty()) { + && !_item->_checksumHeader.isEmpty() + && (csync_is_collision_safe_hash(_item->_checksumHeader) + || _item->_modtime == _item->_previousModtime)) { qCDebug(lcPropagateDownload) << _item->_file << "may not need download, computing checksum"; auto computeChecksum = new ComputeChecksum(this); computeChecksum->setChecksumType(parseChecksumHeaderType(_item->_checksumHeader)); @@ -379,8 +382,17 @@ void PropagateDownloadFile::start() void PropagateDownloadFile::conflictChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum) { if (makeChecksumHeader(checksumType, checksum) == _item->_checksumHeader) { + // No download necessary, just update fs and journal metadata qCDebug(lcPropagateDownload) << _item->_file << "remote and local checksum match"; - // No download necessary, just update metadata + + // Apply the server mtime locally if necessary, ensuring the journal + // and local mtimes end up identical + auto fn = propagator()->getFilePath(_item->_file); + if (_item->_modtime != _item->_previousModtime) { + FileSystem::setModTime(fn, _item->_modtime); + emit propagator()->touchedFile(fn); + } + _item->_modtime = FileSystem::getModTime(fn); updateMetadata(/*isConflict=*/false); return; } diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp index c3db53ab683..5f38ff7cd3d 100644 --- a/test/testsyncengine.cpp +++ b/test/testsyncengine.cpp @@ -311,6 +311,22 @@ private slots: QVERIFY(fakeFolder.syncOnce()); QCOMPARE(nGET, 1); + // Conflict: Same content, different 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.addDays(1)); + remoteInfo.find("A/a1")->checksums = "SHA1:56900fb1d337cf7237ff766276b9c1e8ce507427"; + QVERIFY(fakeFolder.syncOnce()); + // check that mtime in journal and filesystem agree + QString a1path = fakeFolder.localPath() + "A/a1"; + SyncJournalFileRecord a1record; + fakeFolder.syncJournal().getFileRecord(QByteArray("A/a1"), &a1record); + QCOMPARE(a1record._modtime, (qint64)FileSystem::getModTime(a1path)); + QCOMPARE(nGET, 1); + // Extra sync reads from db, no difference QVERIFY(fakeFolder.syncOnce()); QCOMPARE(nGET, 1);