From cf3846c565f201d44935332507579ca5a43fd5e3 Mon Sep 17 00:00:00 2001 From: Olivier Goffart Date: Thu, 31 May 2018 10:56:52 +0200 Subject: [PATCH] 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::List, void, void (OCC::SyncEngine::*)(int)>::call(void (OCC::SyncEngine::*)(int), OCC::SyncEngine*, void**) (qobjectdefs_impl.h:134) ==7230== by 0x5495D92: void QtPrivate::FunctionPointer::call, void>(void (OCC::SyncEngine::*)(int), OCC::SyncEngine*, void**) (qobjectdefs_impl.h:167) ==7230== by 0x5495CB5: QtPrivate::QSlotObject, 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 >), 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== ``` --- src/csync/csync.cpp | 8 +------- src/csync/csync.h | 9 --------- src/csync/csync_private.h | 5 ++++- src/csync/csync_update.cpp | 6 ++---- src/csync/vio/csync_vio.cpp | 6 ------ src/csync/vio/csync_vio.h | 4 ---- src/libsync/discoveryphase.cpp | 2 +- src/libsync/syncengine.cpp | 6 +++--- 8 files changed, 11 insertions(+), 35 deletions(-) diff --git a/src/csync/csync.cpp b/src/csync/csync.cpp index 05fe05217..60c44f2c5 100644 --- 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 50ebfe979..ccc7b1480 100644 --- 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 009c8741b..d64a8c41e 100644 --- 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 cc8f891c7..7799eb01f 100644 --- 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 c5485a9e6..31f168a6c 100644 --- a/src/csync/vio/csync_vio.cpp +++ b/src/csync/vio/csync_vio.cpp @@ -90,9 +90,3 @@ std::unique_ptr 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 926c98ffb..f14c95a70 100644 --- 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_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 112f1caf8..29c70b637 100644 --- 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 1ca1d47a7..54b6d1b13 100644 --- 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) { -- 2.30.2