SyncEngine: invalid the blacklist entry when the rename destination change

The problem in this case is if we rename the file "xxx" to "invalid\file".
The rename will fail because the new filename constains a slash, and it
will be blacklisted.
But then if the user re-rename the file to "valid_name", then we should
invalidate the blacklist entry and retry to upload. But we did not do
that because renaming don't change the mtime and we did not store the
rename target in the database

IL issue 558

CMakeLists.txt

@@ -1,3 +1,4 @@
+
 cmake_minimum_required(VERSION 2.6)
 cmake_policy(VERSION 2.8.0)
 
@@ -122,6 +123,17 @@ if(OWNCLOUD_5XX_NO_BLACKLIST)
     add_definitions(-DOWNCLOUD_5XX_NO_BLACKLIST=1)
 endif()
 
+# When this option is enabled, a rename that is not allowed will be renamed back
+# do the original as a restoration step.  Withut this option, the restoration will
+# re-download the file instead.
+# The default is off because we don't want to rename the files back behind the user's back
+# Added for IL issue #550
+option(OWNCLOUD_RESTORE_RENAME "OWNCLOUD_RESTORE_RENAME" OFF)
+if(OWNCLOUD_RESTORE_RENAME)
+    add_definitions(-DOWNCLOUD_RESTORE_RENAME=1)
+endif()
+
+
 if(APPLE)
   set( SOCKETAPI_TEAM_IDENTIFIER_PREFIX "" CACHE STRING "SocketApi prefix (including a following dot) that must match the codesign key's TeamIdentifier/Organizational Unit" )
 endif()

csync/src/csync_exclude.c

@@ -47,6 +47,45 @@ int _csync_exclude_add(c_strlist_t **inList, const char *string) {
     return c_strlist_add_grow(inList, string);
 }
 
+/** Expands C-like escape sequences.
+ *
+ * The returned string is heap-allocated and owned by the caller.
+ */
+static const char *csync_exclude_expand_escapes(const char * input)
+{
+    size_t i_len = strlen(input) + 1;
+    char *out = c_malloc(i_len); // out can only be shorter
+
+    size_t i = 0;
+    size_t o = 0;
+    for (; i < i_len; ++i) {
+        if (input[i] == '\\') {
+            // at worst input[i+1] is \0
+            switch (input[i+1]) {
+            case '\'': out[o++] = '\''; break;
+            case '"': out[o++] = '"'; break;
+            case '?': out[o++] = '?'; break;
+            case '\\': out[o++] = '\\'; break;
+            case 'a': out[o++] = '\a'; break;
+            case 'b': out[o++] = '\b'; break;
+            case 'f': out[o++] = '\f'; break;
+            case 'n': out[o++] = '\n'; break;
+            case 'r': out[o++] = '\r'; break;
+            case 't': out[o++] = '\t'; break;
+            case 'v': out[o++] = '\v'; break;
+            default:
+                out[o++] = input[i];
+                out[o++] = input[i+1];
+                break;
+            }
+            ++i;
+        } else {
+            out[o++] = input[i];
+        }
+    }
+    return out;
+}
+
 int csync_exclude_load(const char *fname, c_strlist_t **list) {
   int fd = -1;
   int i = 0;
@@ -99,8 +138,10 @@ int csync_exclude_load(const char *fname, c_strlist_t **list) {
       if (entry != buf + i) {
         buf[i] = '\0';
         if (*entry != '#') {
-          CSYNC_LOG(CSYNC_LOG_PRIORITY_TRACE, "Adding entry: %s", entry);
-          rc = _csync_exclude_add(list, entry);
+          const char *unescaped = csync_exclude_expand_escapes(entry);
+          CSYNC_LOG(CSYNC_LOG_PRIORITY_TRACE, "Adding entry: %s", unescaped);
+          rc = _csync_exclude_add(list, unescaped);
+          SAFE_FREE(unescaped);
           if (rc < 0) {
               goto out;
           }

csync/src/csync_update.c

@@ -849,8 +849,6 @@ int csync_ftw(CSYNC *ctx, const char *uri, csync_walker_fn fn,
       flag = CSYNC_FTW_FLAG_NSTAT;
     }
 
-    CSYNC_LOG(CSYNC_LOG_PRIORITY_DEBUG, "Flag for %s: %d", filename, flag);
-
     previous_fs = ctx->current_fs;
 
     /* Call walker function for each file */

csync/src/vio/csync_vio_local_win.c

@@ -137,48 +137,45 @@ static time_t FileTimeToUnixTime(FILETIME *filetime, DWORD *remainder)
 
 csync_vio_file_stat_t *csync_vio_local_readdir(csync_vio_handle_t *dhandle) {
 
-    dhandle_t *handle = NULL;
-    csync_vio_file_stat_t *file_stat = NULL;
-    ULARGE_INTEGER FileIndex;
-    DWORD rem;
+  dhandle_t *handle = NULL;
+  csync_vio_file_stat_t *file_stat = NULL;
+  ULARGE_INTEGER FileIndex;
+  DWORD rem;
 
-    handle = (dhandle_t *) dhandle;
+  handle = (dhandle_t *) dhandle;
 
-    errno = 0;
-    file_stat = csync_vio_file_stat_new();
-    if (file_stat == NULL) {
-        errno = ENOMEM;
-        goto err;
-    }
-    file_stat->fields = CSYNC_VIO_FILE_STAT_FIELDS_NONE;
+  errno = 0;
+  file_stat = csync_vio_file_stat_new();
+  if (file_stat == NULL) {
+      errno = ENOMEM;
+      goto err;
+  }
+  file_stat->fields = CSYNC_VIO_FILE_STAT_FIELDS_NONE;
 
-    // the win32 functions get the first valid entry with the opendir
-    // thus we must not jump to next entry if it was the first find.
-    if( handle->firstFind ) {
-        handle->firstFind = 0;
-    } else {
-        if( FindNextFile(handle->hFind, &(handle->ffd)) == 0 ) {
-            // might be error, check!
-            int dwError = GetLastError();
-            if (dwError != ERROR_NO_MORE_FILES) {
-                errno = EACCES; // no more files is fine. Otherwise EACCESS
-            }
-            goto err;
-        }
-    }
-    file_stat->name = c_utf8_from_locale(handle->ffd.cFileName);
+  // the win32 functions get the first valid entry with the opendir
+  // thus we must not jump to next entry if it was the first find.
+  if( handle->firstFind ) {
+      handle->firstFind = 0;
+  } else {
+      if( FindNextFile(handle->hFind, &(handle->ffd)) == 0 ) {
+          // might be error, check!
+          int dwError = GetLastError();
+          if (dwError != ERROR_NO_MORE_FILES) {
+              errno = EACCES; // no more files is fine. Otherwise EACCESS
+          }
+          goto err;
+      }
+  }
+  file_stat->name = c_utf8_from_locale(handle->ffd.cFileName);
 
-    file_stat->fields |= CSYNC_VIO_FILE_STAT_FIELDS_TYPE;
-    CSYNC_LOG(CSYNC_LOG_PRIORITY_DEBUG, "WinStat info: %ld - %ld %s", handle->ffd.dwFileAttributes, handle->ffd.dwReserved0, file_stat->name);
-    if ( (handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)
-         && (handle->ffd.dwReserved0 & IO_REPARSE_TAG_SYMLINK)
-         && (! (handle->ffd.dwReserved0 & IO_REPARSE_TAG_SIS))
-         && (! (handle->ffd.dwReserved0 & IO_REPARSE_TAG_DEDUP)) ) { // The SIS flag means MS Deduplication. It is not a normal symlink that we want to ignore.
+  file_stat->fields |= CSYNC_VIO_FILE_STAT_FIELDS_TYPE;
+  if (handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT
+            && handle->ffd.dwReserved0 & IO_REPARSE_TAG_SYMLINK) {
         file_stat->flags = CSYNC_VIO_FILE_FLAGS_SYMLINK;
         file_stat->type = CSYNC_VIO_FILE_TYPE_SYMBOLIC_LINK;
     } else if (handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_DEVICE
-               || handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_OFFLINE
-               || handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_TEMPORARY) {
+                || handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_OFFLINE
+                || handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_TEMPORARY) {
         file_stat->type = CSYNC_VIO_FILE_TYPE_UNKNOWN;
     } else if (handle->ffd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
         file_stat->type = CSYNC_VIO_FILE_TYPE_DIRECTORY;
@@ -202,7 +199,7 @@ csync_vio_file_stat_t *csync_vio_local_readdir(csync_vio_handle_t *dhandle) {
     file_stat->fields |= CSYNC_VIO_FILE_STAT_FIELDS_ATIME;
 
     file_stat->mtime = FileTimeToUnixTime(&handle->ffd.ftLastWriteTime, &rem);
-    /* CSYNC_LOG(CSYNC_LOG_PRIORITY_DEBUG, "Local File MTime: %llu", (unsigned long long) buf->mtime ); */
+      /* CSYNC_LOG(CSYNC_LOG_PRIORITY_DEBUG, "Local File MTime: %llu", (unsigned long long) buf->mtime ); */
     file_stat->fields |= CSYNC_VIO_FILE_STAT_FIELDS_MTIME;
 
     file_stat->ctime = FileTimeToUnixTime(&handle->ffd.ftCreationTime, &rem);
@@ -211,9 +208,9 @@ csync_vio_file_stat_t *csync_vio_local_readdir(csync_vio_handle_t *dhandle) {
     return file_stat;
 
 err:
-    SAFE_FREE(file_stat);
+  SAFE_FREE(file_stat);
 
-    return NULL;
+  return NULL;
 }
 
 
@@ -232,15 +229,15 @@ int csync_vio_local_stat(const char *uri, csync_vio_file_stat_t *buf) {
     h = CreateFileW( wuri, 0, FILE_SHARE_READ, NULL, OPEN_EXISTING,
                      FILE_ATTRIBUTE_NORMAL+FILE_FLAG_BACKUP_SEMANTICS+FILE_FLAG_OPEN_REPARSE_POINT, NULL );
     if( h == INVALID_HANDLE_VALUE ) {
+        CSYNC_LOG(CSYNC_LOG_PRIORITY_CRIT, "CreateFileW failed on %s", uri );
         errno = GetLastError();
-        CSYNC_LOG(CSYNC_LOG_PRIORITY_CRIT, "CreateFileW failed on %s, error code %ld", uri, errno);
         c_free_locale_string(wuri);
         return -1;
     }
 
     if(!GetFileInformationByHandle( h, &fileInfo ) ) {
+        CSYNC_LOG(CSYNC_LOG_PRIORITY_CRIT, "GetFileInformationByHandle failed on %s", uri );
         errno = GetLastError();
-        CSYNC_LOG(CSYNC_LOG_PRIORITY_CRIT, "GetFileInformationByHandle failed on %s, error code %ld", uri, errno );
         c_free_locale_string(wuri);
         CloseHandle(h);
         return -1;

csync/tests/csync_tests/check_csync_exclude.c

@@ -346,6 +346,25 @@ static void check_csync_excluded_performance(void **state)
     }
 }
 
+static void check_csync_exclude_expand_escapes(void **state)
+{
+    (void)state;
+
+    const char *str = csync_exclude_expand_escapes(
+            "keep \\' \\\" \\? \\\\ \\a \\b \\f \\n \\r \\t \\v \\z");
+    assert_true(0 == strcmp(
+            str, "keep ' \" ? \\ \a \b \f \n \r \t \v \\z"));
+    SAFE_FREE(str);
+
+    str = csync_exclude_expand_escapes("");
+    assert_true(0 == strcmp(str, ""));
+    SAFE_FREE(str);
+
+    str = csync_exclude_expand_escapes("\\");
+    assert_true(0 == strcmp(str, "\\"));
+    SAFE_FREE(str);
+}
+
 int torture_run_tests(void)
 {
     const UnitTest tests[] = {
@@ -356,6 +375,7 @@ int torture_run_tests(void)
         unit_test_setup_teardown(check_csync_pathes, setup_init, teardown),
         unit_test_setup_teardown(check_csync_is_windows_reserved_word, setup_init, teardown),
         unit_test_setup_teardown(check_csync_excluded_performance, setup_init, teardown),
+        unit_test(check_csync_exclude_expand_escapes),
     };
 
     return run_tests(tests);

src/libsync/propagatedownload.cpp

@@ -543,12 +543,20 @@ void PropagateDownloadFileQNAM::slotGetFinished()
         return;
     }
 
+    if (_tmpFile.size() == 0 && _item->_size > 0) {
+        FileSystem::remove(_tmpFile.fileName());
+        done(SyncFileItem::NormalError,
+             tr("The downloaded file is empty despite the server announced it should have been %1.")
+                .arg(Utility::octetsToString(_item->_size)));
+        return;
+    }
+
     // Do checksum validation for the download. If there is no checksum header, the validator
-    // will also emit the validated() signal to continue the flow in slot downloadFinished()
+    // will also emit the validated() signal to continue the flow in slot transmissionChecksumValidated()
     // as this is (still) also correct.
     ValidateChecksumHeader *validator = new ValidateChecksumHeader(this);
     connect(validator, SIGNAL(validated(QByteArray,QByteArray)),
-            SLOT(downloadFinished()));
+            SLOT(transmissionChecksumValidated(QByteArray,QByteArray)));
     connect(validator, SIGNAL(validationFailed(QString)),
             SLOT(slotChecksumFail(QString)));
     auto checksumHeader = job->reply()->rawHeader(checkSumHeaderC);
@@ -636,6 +644,35 @@ static void handleRecallFile(const QString &fn)
 }
 } // end namespace
 
+void PropagateDownloadFileQNAM::transmissionChecksumValidated(const QByteArray &checksumType, const QByteArray &checksum)
+{
+    const auto theContentChecksumType = QByteArray("SHA1");
+
+    // Reuse transmission checksum as content checksum.
+    //
+    // We could do this more aggressively and accept both MD5 and SHA1
+    // instead of insisting on the exactly correct checksum type.
+    if (theContentChecksumType == checksumType || theContentChecksumType.isEmpty()) {
+        return contentChecksumComputed(checksumType, checksum);
+    }
+
+    // Compute the content checksum.
+    auto computeChecksum = new ComputeChecksum(this);
+    computeChecksum->setChecksumType(theContentChecksumType);
+
+    connect(computeChecksum, SIGNAL(done(QByteArray,QByteArray)),
+            SLOT(contentChecksumComputed(QByteArray,QByteArray)));
+    computeChecksum->start(_tmpFile.fileName());
+}
+
+void PropagateDownloadFileQNAM::contentChecksumComputed(const QByteArray &checksumType, const QByteArray &checksum)
+{
+    _item->_contentChecksum = checksum;
+    _item->_contentChecksumType = checksumType;
+
+    downloadFinished();
+}
+
 void PropagateDownloadFileQNAM::downloadFinished()
 {
     QString fn = _propagator->getFilePath(_item->_file);

src/libsync/propagatedownload.h

@@ -128,6 +128,8 @@ public:
 private slots:
     void slotGetFinished();
     void abort() Q_DECL_OVERRIDE;
+    void transmissionChecksumValidated(const QByteArray& checksumType, const QByteArray& checksum);
+    void contentChecksumComputed(const QByteArray& checksumType, const QByteArray& checksum);
     void downloadFinished();
     void slotDownloadProgress(qint64,qint64);
     void slotChecksumFail( const QString& errMsg );

src/libsync/propagateupload.cpp

@@ -222,8 +222,9 @@ void PropagateUploadFileQNAM::slotComputeContentChecksum()
     QByteArray contentChecksumType;
     // We currently only do content checksums for the particular .eml case
     // This should be done more generally in the future!
-    if (filePath.endsWith(QLatin1String(".eml"), Qt::CaseInsensitive)) {
-        contentChecksumType = "MD5";
+    if (filePath.endsWith(QLatin1String(".eml"), Qt::CaseInsensitive)
+            || filePath.endsWith(QLatin1String(".msg"), Qt::CaseInsensitive)) {
+        contentChecksumType = "SHA1";
     }
 
     // Maybe the discovery already computed the checksum?

src/libsync/syncengine.cpp

@@ -183,6 +183,12 @@ QString SyncEngine::csyncErrorToString(CSYNC_STATUS err)
 
 }
 
+/**
+ * Check if the item is in the blacklist.
+ * If it should not be sync'ed because of the blacklist, update the item with the error instruction
+ * and proper error message, and return true.
+ * If the item is not in the blacklist, or the blacklist is stale, return false.
+ */
 bool SyncEngine::checkErrorBlacklisting( SyncFileItem &item )
 {
     if( !_journal ) {
@@ -214,6 +220,9 @@ bool SyncEngine::checkErrorBlacklisting( SyncFileItem &item )
         } else if( item._modtime != entry._lastTryModtime ) {
             qDebug() << item._file << " is blacklisted, but has changed mtime!";
             return false;
+        } else if( item._renameTarget != entry._renameTarget) {
+            qDebug() << item._file << " is blacklisted, but rename target changed from" << entry._renameTarget;
+            return false;
         }
     } else if( item._direction == SyncFileItem::Down ) {
         // download, check the etag.
@@ -525,6 +534,7 @@ int SyncEngine::treewalkFile( TREE_WALK_FILE *file, bool remote )
                 item->_isDirectory = isDirectory;
                 _syncItemMap.insert(key, item);
             }
+            item->_isDirectory = isDirectory;
             emit syncItemDiscovered(*item);
             return re;
         }
@@ -1182,15 +1192,18 @@ void SyncEngine::checkForPermission()
                     }
                 }
 
-#if 0 /* We don't like the idea of renaming behind user's back, as the user may be working with the files */
-
-                if (!sourceOK && !destinationOK) {
+#ifdef OWNCLOUD_RESTORE_RENAME /* We don't like the idea of renaming behind user's back, as the user may be working with the files */
+                if (!sourceOK && (!destinationOK || isRename)
+                        // (not for directory because that's more complicated with the contents that needs to be adjusted)
+                        && !(*it)->_isDirectory) {
                     // Both the source and the destination won't allow move.  Move back to the original
                     std::swap((*it)->_file, (*it)->_renameTarget);
                     (*it)->_direction = SyncFileItem::Down;
                     (*it)->_errorString = tr("Move not allowed, item restored");
                     (*it)->_isRestoration = true;
                     qDebug() << "checkForPermission: MOVING BACK" << (*it)->_file;
+                    // in case something does wrong, we will not do it next time
+                    _journal->avoidRenamesOnNextSync((*it)->_file);
                 } else
 #endif
                 if (!sourceOK || !destinationOK) {

src/libsync/syncjournaldb.cpp

@@ -409,7 +409,7 @@ bool SyncJournalDb::checkConnect()
     _deleteFileRecordRecursively.reset(new SqlQuery(_db));
     _deleteFileRecordRecursively->prepare("DELETE FROM metadata WHERE path LIKE(?||'/%')");
 
-    QString sql( "SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration "
+    QString sql( "SELECT lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget "
                  "FROM blacklist WHERE path=?1");
     if( Utility::fsCasePreserving() ) {
         // if the file system is case preserving we have to check the blacklist
@@ -421,8 +421,8 @@ bool SyncJournalDb::checkConnect()
 
     _setErrorBlacklistQuery.reset(new SqlQuery(_db));
     _setErrorBlacklistQuery->prepare("INSERT OR REPLACE INTO blacklist "
-                                "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration) "
-                                "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7)");
+                                "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget) "
+                                "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8)");
 
     _getSelectiveSyncListQuery.reset(new SqlQuery(_db));
     _getSelectiveSyncListQuery->prepare("SELECT path FROM selectivesync WHERE type=?1");
@@ -612,6 +612,15 @@ bool SyncJournalDb::updateErrorBlacklistTableStructure()
         }
         commitInternal("update database structure: add lastTryTime, ignoreDuration cols");
     }
+    if( columns.indexOf(QLatin1String("renameTarget")) == -1 ) {
+        SqlQuery query(_db);
+        query.prepare("ALTER TABLE blacklist ADD COLUMN renameTarget VARCHAR(4096);");
+        if( !query.exec() ) {
+            sqlFail("updateBlacklistTableStructure: Add renameTarget", query);
+            re = false;
+        }
+        commitInternal("update database structure: add lastTryTime, ignoreDuration cols");
+    }
 
     SqlQuery query(_db);
     query.prepare("CREATE INDEX IF NOT EXISTS blacklist_index ON blacklist(path collate nocase);");
@@ -1213,6 +1222,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry( const QStrin
                 entry._errorString    = _getErrorBlacklistQuery->stringValue(3);
                 entry._lastTryTime    = _getErrorBlacklistQuery->int64Value(4);
                 entry._ignoreDuration = _getErrorBlacklistQuery->int64Value(5);
+                entry._renameTarget   = _getErrorBlacklistQuery->stringValue(6);
                 entry._file           = file;
             }
             _getErrorBlacklistQuery->reset_and_clear_bindings();
@@ -1324,13 +1334,14 @@ void SyncJournalDb::updateErrorBlacklistEntry( const SyncJournalErrorBlacklistRe
     _setErrorBlacklistQuery->bindValue(5, item._errorString);
     _setErrorBlacklistQuery->bindValue(6, QString::number(item._lastTryTime));
     _setErrorBlacklistQuery->bindValue(7, QString::number(item._ignoreDuration));
+    _setErrorBlacklistQuery->bindValue(8, item._renameTarget);
     if( !_setErrorBlacklistQuery->exec() ) {
         QString bug = _setErrorBlacklistQuery->error();
         qDebug() << "SQL exec blacklistitem insert or replace failed: "<< bug;
     }
     qDebug() << "set blacklist entry for " << item._file << item._retryCount
              << item._errorString << item._lastTryTime << item._ignoreDuration
-             << item._lastTryModtime << item._lastTryEtag;
+             << item._lastTryModtime << item._lastTryEtag << item._renameTarget ;
     _setErrorBlacklistQuery->reset_and_clear_bindings();
 
 }

src/libsync/syncjournalfilerecord.cpp

@@ -151,6 +151,7 @@ SyncJournalErrorBlacklistRecord SyncJournalErrorBlacklistRecord::update(
     // The factor of 5 feels natural: 25s, 2 min, 10 min, ~1h, ~5h, ~24h
     entry._ignoreDuration = old._ignoreDuration * 5;
     entry._file = item._file;
+    entry._renameTarget = item._renameTarget;
 
     if( item._httpErrorCode == 403 ) {
         qDebug() << "Probably firewall error: " << item._httpErrorCode << ", blacklisting up to 1h only";

src/libsync/syncjournalfilerecord.h

@@ -89,6 +89,7 @@ public:
     time_t     _ignoreDuration;
 
     QString    _file;
+    QString    _renameTarget;
 
     bool isValid() const;