csync: refactor csync_s::error_string to avoid valgrind error

The problem here is that we were sometimes allocating the error_string with
qstrdup, which need to be released with delete[] and not free().

Simplify the code by using QString instead.

```
==7230== Mismatched free() / delete / delete []
==7230==    at 0x4C2E10B: free (vg_replace_malloc.c:530)
==7230==    by 0x57C2321: csync_s::reinitialize() (csync.cpp:247)
==7230==    by 0x548130F: OCC::SyncEngine::finalize(bool) (syncengine.cpp:1212)
==7230==    by 0x5481223: OCC::SyncEngine::handleSyncError(csync_s*, char const*) (syncengine.cpp:746)
==7230==    by 0x5483E78: OCC::SyncEngine::slotDiscoveryJobFinished(int) (syncengine.cpp:965)
==7230==    by 0x5495E34: QtPrivate::FunctorCall<QtPrivate::IndexesList<0>, QtPrivate::List<int>, void, void (OCC::SyncEngine::*)(int)>::call(void (OCC::SyncEngine::*)(int), OCC::SyncEngine*, void**) (qobjectdefs_impl.h:134)
==7230==    by 0x5495D92: void QtPrivate::FunctionPointer<void (OCC::SyncEngine::*)(int)>::call<QtPrivate::List<int>, void>(void (OCC::SyncEngine::*)(int), OCC::SyncEngine*, void**) (qobjectdefs_impl.h:167)
==7230==    by 0x5495CB5: QtPrivate::QSlotObject<void (OCC::SyncEngine::*)(int), QtPrivate::List<int>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (qobjectdefs_impl.h:396)
==7230==    by 0xA9BF2E1: QObject::event(QEvent*) (in /usr/lib/libQt5Core.so.5.11.0)
==7230==    by 0x64BE983: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.11.0)
==7230==    by 0x64C625A: QApplication::notify(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.11.0)
==7230==    by 0xA994BC8: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib/libQt5Core.so.5.11.0)
==7230==  Address 0x225b2640 is 0 bytes inside a block of size 50 alloc'd
==7230==    at 0x4C2DC6F: operator new[](unsigned long) (vg_replace_malloc.c:423)
==7230==    by 0xA7E8FC8: qstrdup(char const*) (in /usr/lib/libQt5Core.so.5.11.0)
==7230==    by 0x53F5750: OCC::DiscoveryJob::remote_vio_opendir_hook(char const*, void*) (discoveryphase.cpp:666)
==7230==    by 0x57E1278: csync_vio_opendir(csync_s*, char const*) (csync_vio.cpp:39)
==7230==    by 0x57D718F: csync_ftw(csync_s*, char const*, int (*)(csync_s*, std::unique_ptr<csync_file_stat_s, std::default_delete<csync_file_stat_s> >), unsigned int) (csync_update.cpp:674)
==7230==    by 0x57C1B05: csync_update(csync_s*) (csync.cpp:109)
==7230==    by 0x53F5BCC: OCC::DiscoveryJob::start() (discoveryphase.cpp:718)
==7230==    by 0x54B8F74: OCC::DiscoveryJob::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) (moc_discoveryphase.cpp:494)
==7230==    by 0xA9BF2E1: QObject::event(QEvent*) (in /usr/lib/libQt5Core.so.5.11.0)
==7230==    by 0x64BE983: QApplicationPrivate::notify_helper(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.11.0)
==7230==    by 0x64C625A: QApplication::notify(QObject*, QEvent*) (in /usr/lib/libQt5Widgets.so.5.11.0)
==7230==    by 0xA994BC8: QCoreApplication::notifyInternal2(QObject*, QEvent*) (in /usr/lib/libQt5Core.so.5.11.0)
==7230==
```

diff --git a/src/csync/csync.cpp b/src/csync/csync.cpp
index 05fe052172caec2f197bab93d75efc91f85064c6..60c44f2c50567b995356f9267047377ac37f27ee 100644 (file)
--- a/src/csync/csync.cpp
+++ b/src/csync/csync.cpp
@@ -244,7 +244,7 @@ int csync_s::reinitialize() {
   renames.folder_renamed_to.clear();
 
   status = CSYNC_STATUS_INIT;
-  SAFE_FREE(error_string);
+  error_string.clear();
 
   rc = 0;
   return rc;
@@ -252,7 +252,6 @@ int csync_s::reinitialize() {
 
 csync_s::~csync_s() {
   SAFE_FREE(local.uri);
-  SAFE_FREE(error_string);
 }
 
 void *csync_get_userdata(CSYNC *ctx) {
@@ -298,11 +297,6 @@ CSYNC_STATUS csync_get_status(CSYNC *ctx) {
   return ctx->status_code;
 }
 
-const char *csync_get_status_string(CSYNC *ctx)
-{
-  return csync_vio_get_status_string(ctx);
-}
-
 void csync_request_abort(CSYNC *ctx)
 {
   if (ctx) {

diff --git a/src/csync/csync.h b/src/csync/csync.h
index 50ebfe9799a6a1d0a621f2da57387b5fccf5b3ca..ccc7b1480afc076a43106106fce89489f332f437 100644 (file)
--- a/src/csync/csync.h
+++ b/src/csync/csync.h
@@ -294,15 +294,6 @@ int OCSYNC_EXPORT csync_walk_local_tree(CSYNC *ctx, const csync_treewalk_visit_f
  */
 int OCSYNC_EXPORT csync_walk_remote_tree(CSYNC *ctx, const csync_treewalk_visit_func &visitor);
 
-/**
- * @brief Get the csync status string.
- *
- * @param ctx            The csync context.
- *
- * @return               A const pointer to a string with more precise status info.
- */
-const char OCSYNC_EXPORT *csync_get_status_string(CSYNC *ctx);
-
 /**
  * @brief Aborts the current sync run as soon as possible. Can be called from another thread.
  *

diff --git a/src/csync/csync_private.h b/src/csync/csync_private.h
index 009c8741b64a5e233fc78f7cdcb48b91de08e30d..d64a8c41e5799e3f7bb56bb268852da2e89b9840 100644 (file)
--- a/src/csync/csync_private.h
+++ b/src/csync/csync_private.h
@@ -191,7 +191,10 @@ struct OCSYNC_EXPORT csync_s {
   /* csync error code */
   enum CSYNC_STATUS status_code = CSYNC_STATUS_OK;
 
-  char *error_string = nullptr;
+  /* Some additional string information which is added to the error message corresponding to the error code in errno.
+   * Usually a filename
+   */
+  QString error_string;
 
   int status = CSYNC_STATUS_INIT;
   volatile bool abort = false;

diff --git a/src/csync/csync_update.cpp b/src/csync/csync_update.cpp
index cc8f891c769dca9bfe9876263bcc1badd0658839..7799eb01f29292d81d98f8f5ed453bd488ed98c6 100644 (file)
--- a/src/csync/csync_update.cpp
+++ b/src/csync/csync_update.cpp
@@ -707,7 +707,6 @@ int csync_ftw(CSYNC *ctx, const char *uri, csync_walker_fn fn,
           ctx->status_code = CSYNC_STATUS_ABORTED;
           goto error;
       }
-      int asp = 0;
       /* permission denied */
       ctx->status_code = csync_errno_to_status(errno, CSYNC_STATUS_OPENDIR_ERROR);
       if (errno == EACCES) {
@@ -716,8 +715,7 @@ int csync_ftw(CSYNC *ctx, const char *uri, csync_walker_fn fn,
               return 0;
           }
       } else if(errno == ENOENT) {
-          asp = asprintf( &ctx->error_string, "%s", uri);
-          ASSERT(asp >= 0);
+          ctx->error_string = QString::fromUtf8(uri);
       }
       // 403 Forbidden can be sent by the server if the file firewall is active.
       // A file or directory should be ignored and sync must continue. See #3490
@@ -762,7 +760,7 @@ int csync_ftw(CSYNC *ctx, const char *uri, csync_walker_fn fn,
     /* Conversion error */
     if (dirent->path.isEmpty() && !dirent->original_path.isEmpty()) {
         ctx->status_code = CSYNC_STATUS_INVALID_CHARACTERS;
-        ctx->error_string = c_strdup(dirent->original_path);
+        ctx->error_string = QString::fromUtf8(dirent->original_path);
         dirent->original_path.clear();
         goto error;
     }

diff --git a/src/csync/vio/csync_vio.cpp b/src/csync/vio/csync_vio.cpp
index c5485a9e65b5b78fe76dfa93f0b8e4a4a6884e4b..31f168a6c3683b7fa6802ac544088eb56c6afe7d 100644 (file)
--- a/src/csync/vio/csync_vio.cpp
+++ b/src/csync/vio/csync_vio.cpp
@@ -90,9 +90,3 @@ std::unique_ptr<csync_file_stat_t> csync_vio_readdir(CSYNC *ctx, csync_vio_handl
   return nullptr;
 }
 
-char *csync_vio_get_status_string(CSYNC *ctx) {
-  if(ctx->error_string) {
-    return ctx->error_string;
-  }
-  return nullptr;
-}

diff --git a/src/csync/vio/csync_vio.h b/src/csync/vio/csync_vio.h
index 926c98ffb8059e19749f81eed4969212b833cb8e..f14c95a70bc0e3f0be689d2ba19e5f5d7e5b89bf 100644 (file)
--- a/src/csync/vio/csync_vio.h
+++ b/src/csync/vio/csync_vio.h
@@ -35,8 +35,4 @@ struct fhandle_t {
 csync_vio_handle_t *csync_vio_opendir(CSYNC *ctx, const char *name);
 int csync_vio_closedir(CSYNC *ctx, csync_vio_handle_t *dhandle);
 std::unique_ptr<csync_file_stat_t> csync_vio_readdir(CSYNC *ctx, csync_vio_handle_t *dhandle);
-
-char *csync_vio_get_status_string(CSYNC *ctx);
-
-
 #endif /* _CSYNC_VIO_H */

diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp
index 112f1caf8c1b73f64092c0dac06384c54d693bdd..29c70b63732e8e67f856314a12461deb472255ce 100644 (file)
--- a/src/libsync/discoveryphase.cpp
+++ b/src/libsync/discoveryphase.cpp
@@ -671,7 +671,7 @@ csync_vio_handle_t *DiscoveryJob::remote_vio_opendir_hook(const char *url,
             qCDebug(lcDiscovery) << directoryResult->code << "when opening" << url << "msg=" << directoryResult->msg;
             errno = directoryResult->code;
             // save the error string to the context
-            discoveryJob->_csync_ctx->error_string = qstrdup(directoryResult->msg.toUtf8().constData());
+            discoveryJob->_csync_ctx->error_string = directoryResult->msg;
             return nullptr;
         }
 

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index 1ca1d47a748fdc4dd7ae12b544dbac6a70cd1198..54b6d1b13b81d914a6904435ef88d3b34880ef62 100644 (file)
--- a/src/libsync/syncengine.cpp
+++ b/src/libsync/syncengine.cpp
@@ -722,13 +722,13 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other,
 void SyncEngine::handleSyncError(CSYNC *ctx, const char *state)
 {
     CSYNC_STATUS err = csync_get_status(ctx);
-    const char *errMsg = csync_get_status_string(ctx);
+    QString errMsg = ctx->error_string;
     QString errStr = csyncErrorToString(err);
-    if (errMsg) {
+    if (!errMsg.isEmpty()) {
         if (!errStr.endsWith(" ")) {
             errStr.append(" ");
         }
-        errStr += QString::fromUtf8(errMsg);
+        errStr += errMsg;
     }
     // Special handling CSYNC_STATUS_INVALID_CHARACTERS
     if (err == CSYNC_STATUS_INVALID_CHARACTERS) {