From 563eae914268ab5c107e9843d395df7062d47b78 Mon Sep 17 00:00:00 2001 From: Christian Kamm Date: Thu, 16 Nov 2017 10:13:21 +0100 Subject: [PATCH] PropagateDownload: Adjustments to skipping downloads #6153 Previously we required matching mtimes but that's actually unnecessary when the question is about whether to skip the download. We will still update the file's metadata. Also, adjust behavior when the checksum is weak (Adler32): in these cases we still depend on equal mtimes. --- src/csync/csync_reconcile.cpp | 22 +++++----------------- src/csync/csync_util.cpp | 7 +++++++ src/csync/csync_util.h | 10 ++++++++++ src/libsync/propagatedownload.cpp | 20 ++++++++++++++++---- test/testsyncengine.cpp | 16 ++++++++++++++++ 5 files changed, 54 insertions(+), 21 deletions(-) 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);