This is an automated email from the ASF dual-hosted git repository.
martinzink pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/nifi-minifi-cpp.git
The following commit(s) were added to refs/heads/main by this push:
new a6d147bbb MINIFICPP-2877 Clean up memcheck (#2243)
a6d147bbb is described below
commit a6d147bbbdd09014e96911234e5a64872efa0005
Author: Ferenc Gerlits <[email protected]>
AuthorDate: Fri Aug 14 16:35:26 2026 +0200
MINIFICPP-2877 Clean up memcheck (#2243)
- fix a small leak in opc.cpp: the default stdout logger allocated by
UA_Client_new was not freed when we replaced it with our own logger
(and it was also cached elsewhere inside the client)
- fix a test code leak in PutOpcProcessorTests
- suppress "still reachable" valgrind logs: these come from static objects,
and are not a problem
- suppress some "possible loss" valgrind logs: these also come from
long-lived objects which are not really leaked
---
.github/workflows/memcheck_ci.yml | 19 ++++
CMakeLists.txt | 1 +
cmake/valgrind.supp | 121 ++++++++++++++++++++++++++
extensions/opc/include/opc.h | 2 +-
extensions/opc/src/opc.cpp | 39 +++++----
extensions/opc/tests/PutOpcProcessorTests.cpp | 5 ++
6 files changed, 171 insertions(+), 16 deletions(-)
diff --git a/.github/workflows/memcheck_ci.yml
b/.github/workflows/memcheck_ci.yml
index 6a94b89f1..3b8d0f71a 100644
--- a/.github/workflows/memcheck_ci.yml
+++ b/.github/workflows/memcheck_ci.yml
@@ -55,3 +55,22 @@ jobs:
ulimit -c unlimited
ctest -j$(nproc) -L memchecked -T memcheck
working-directory: build
+ - name: check valgrind output
+ id: check
+ run: |
+ nonempty_valgrind_outputs=$(find Testing/Temporary -maxdepth 1 -name
'MemoryChecker.*.log' -size +0 -print)
+ if [ -n "$nonempty_valgrind_outputs" ]; then
+ echo "::error::Valgrind reported issues in one or more tests."
+ exit 1
+ fi
+ echo "Valgrind report is clean."
+ working-directory: build
+ - name: upload memcheck logs
+ if: ${{ failure() && steps.check.conclusion == 'failure' }}
+ uses: actions/upload-artifact@v4
+ with:
+ name: memcheck-logs
+ path: |
+ build/Testing/Temporary/MemoryChecker.*.log
+ build/Testing/Temporary/LastDynamicAnalysis_*.log
+ if-no-files-found: ignore
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 88b28c970..2d0903c2e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -795,6 +795,7 @@ if (NOT SKIP_TESTS)
enable_testing()
set(BUILD_TESTING ON)
set(CTEST_NEW_FORMAT true)
+ set(MEMORYCHECK_SUPPRESSIONS_FILE
"${CMAKE_CURRENT_SOURCE_DIR}/cmake/valgrind.supp" CACHE FILEPATH "Valgrind
suppression file")
include(CTest)
include(BuildTests)
diff --git a/cmake/valgrind.supp b/cmake/valgrind.supp
new file mode 100644
index 000000000..3a4ce186c
--- /dev/null
+++ b/cmake/valgrind.supp
@@ -0,0 +1,121 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Valgrind suppressions for MiNiFi C++ test runs.
+# Wired into CTest via the MEMORYCHECK_SUPPRESSIONS_FILE cache variable
+# in the top-level CMakeLists.txt.
+# Or invoke valgrind directly with --suppressions=cmake/valgrind.supp
+
+# Every "still reachable" record in the MiNiFi test binaries stems from a
+# process-lifetime singleton or a static initializer registering plugin
+# factories: rocksdb::ObjectLibrary, spdlog pattern-formatter handlers,
+# LoggerConfiguration, extension dlopen()s, zlib globals, etc. Valgrind
+# still holds a live pointer to all of these at exit, so by definition
+# they are not leaked; they are intentionally kept for the process's
+# lifetime and freed by the OS on exit. `fun:*` matches any top frame
+# (valgrind rejects suppressions whose only entry is `...`).
+{
+ still_reachable_process_lifetime
+ Memcheck:Leak
+ match-leak-kinds: reachable
+ fun:*
+}
+
+# glibc allocates a Thread-Local-Storage / DTV block via calloc inside
+# pthread_create. For long-lived worker threads (e.g. the log-compressor)
+# the pointer is still reachable at exit and valgrind flags it "possibly
+# lost". Not a real leak.
+{
+ pthread_create_tls_dtv_LogCompressorSink
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:calloc
+ ...
+ fun:pthread_create*
+ ...
+ fun:*LogCompressorSink*
+}
+
+# Every extension which uses protobuf calls AddDescriptorsImpl at static-init
time,
+# which populates the process-wide DescriptorPool / MessageFactory singletons.
+# Valgrind flags these as "possibly lost" because they're reached via interior
abseil-hash pointers.
+# The memory is freed by the OS at exit, not leaked.
+{
+ possibly_lost_extension_static_init
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:_Znwm
+ ...
+ fun:*AddDescriptorsImpl*
+ ...
+ fun:*Extension*load*
+ ...
+}
+# PushGrafanaLokiGrpcTest and PushGrafanaLokiRESTTest statically link protobuf,
+# so a second copy of the DescriptorPool singleton is initialized from inside
the
+# test binary itself at process startup.
+{
+ possibly_lost_grafana_loki_static_init_dl_init
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:_Znwm
+ ...
+ fun:*AddDescriptorsImpl*
+ ...
+ fun:_dl_init
+ obj:*ld-linux*
+}
+{
+ possibly_lost_grafana_loki_static_init_libc_start_main
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:_Znwm
+ ...
+ fun:*AddDescriptorsImpl*
+ ...
+ fun:__libc_start_main*
+ ...
+}
+
+# glibc grows the Thread-Local-Storage descriptor vector (DTV) via malloc the
+# first time spdlog::details::os::thread_id() runs on a new thread. The block
+# is kept for the thread's lifetime and freed by the OS at exit. Not a leak.
+{
+ possibly_lost_tls_dtv_spdlog_thread_id
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:malloc
+ ...
+ fun:_dl_resize_dtv
+ ...
+ fun:*spdlog*thread_id*
+ ...
+}
+
+# grpc_core::ChannelInit::BuildStackConfig populates the process-lifetime
+# CoreConfiguration singleton (channelz::PropertyTable and friends) the first
+# time grpc_server_create is called. Held via interior abseil-hash pointers,
+# freed by the OS at exit. Not a leak.
+{
+ possibly_lost_grpc_core_configuration_singleton
+ Memcheck:Leak
+ match-leak-kinds: possible
+ fun:_Znwm
+ ...
+ fun:*ChannelInit*BuildStackConfig*
+ ...
+}
diff --git a/extensions/opc/include/opc.h b/extensions/opc/include/opc.h
index 163bb2c9e..8a4973515 100644
--- a/extensions/opc/include/opc.h
+++ b/extensions/opc/include/opc.h
@@ -91,7 +91,7 @@ class Client {
const std::vector<char>& cert_buffer, const std::vector<char>&
key_buffer,
const std::vector<std::vector<char>>& trust_buffers);
- UA_Client *client_;
+ UA_Client *client_{nullptr};
std::shared_ptr<core::logging::Logger> logger_;
UA_Logger minifi_ua_logger_{};
bool use_encryption_{false};
diff --git a/extensions/opc/src/opc.cpp b/extensions/opc/src/opc.cpp
index ab04c4bbc..b66664529 100644
--- a/extensions/opc/src/opc.cpp
+++ b/extensions/opc/src/opc.cpp
@@ -114,13 +114,20 @@ core::logging::LOG_LEVEL MapOPCLogLevel(UA_LogLevel
ualvl) {
Client::Client(const std::shared_ptr<core::logging::Logger>& logger, const
std::string& application_uri,
const std::vector<char>& cert_buffer, const std::vector<char>&
key_buffer,
const std::vector<std::vector<char>>& trust_buffers)
- : client_(UA_Client_new()),
- use_encryption_(!cert_buffer.empty()) {
+ : use_encryption_(!cert_buffer.empty()) {
+ minifi_ua_logger_ = {logFunc, logger.get(), [](UA_Logger*){}};
+
+ // Build the config with our logger pre-installed so that open62541 doesn't
allocate a default stdout logger (as it would with UA_Client_new).
+ UA_ClientConfig config{};
+ config.logging = &minifi_ua_logger_;
+
if (!use_encryption_) {
- UA_ClientConfig_setDefault(UA_Client_getConfig(client_));
+ if (UA_StatusCode sc = UA_ClientConfig_setDefault(&config); sc !=
UA_STATUSCODE_GOOD) {
+ UA_ClientConfig_clear(&config);
+ throw OPCException(GENERAL_EXCEPTION, std::string("Failed to configure
the OPC UA client: ") + UA_StatusCode_name(sc));
+ }
} else {
- UA_ClientConfig *cc = UA_Client_getConfig(client_);
- cc->securityMode = UA_MESSAGESECURITYMODE_SIGNANDENCRYPT;
+ config.securityMode = UA_MESSAGESECURITYMODE_SIGNANDENCRYPT;
// Certificate
UA_ByteString cert_byte_string = UA_STRING_NULL;
@@ -143,7 +150,7 @@ Client::Client(const
std::shared_ptr<core::logging::Logger>& logger, const std::
trust_list[i].data =
reinterpret_cast<UA_Byte*>(UA_malloc(trust_list[i].length * sizeof(UA_Byte)));
// NOLINT(cppcoreguidelines-owning-memory)
memcpy(trust_list[i].data, trust_buffers[i].data(),
trust_list[i].length);
}
- UA_StatusCode sc = UA_ClientConfig_setDefaultEncryption(cc,
cert_byte_string, key_byte_string,
+ UA_StatusCode sc = UA_ClientConfig_setDefaultEncryption(&config,
cert_byte_string, key_byte_string,
trust_list.data(),
trust_buffers.size(),
nullptr, 0);
UA_ByteString_clear(&cert_byte_string);
@@ -153,20 +160,22 @@ Client::Client(const
std::shared_ptr<core::logging::Logger>& logger, const std::
}
if (sc != UA_STATUSCODE_GOOD) {
logger->log_error("Configuring the client for encryption failed: {}",
UA_StatusCode_name(sc));
- UA_Client_delete(client_);
- throw OPCException(GENERAL_EXCEPTION, std::string("Failed to created
client with the provided encryption settings: ") + UA_StatusCode_name(sc));
+ UA_ClientConfig_clear(&config);
+ throw OPCException(GENERAL_EXCEPTION, std::string("Failed to create the
OPC UA client with the provided encryption settings: ") +
UA_StatusCode_name(sc));
}
}
- minifi_ua_logger_ = {logFunc, logger.get(), [](UA_Logger*){}};
-
- UA_ClientConfig *config_ptr = UA_Client_getConfig(client_);
- config_ptr->logging = &minifi_ua_logger_;
- config_ptr->allowNonePolicyPassword = true;
+ config.allowNonePolicyPassword = true;
if (!application_uri.empty()) {
- UA_String_clear(&config_ptr->clientDescription.applicationUri);
- config_ptr->clientDescription.applicationUri =
UA_STRING_ALLOC(application_uri.c_str());
+ UA_String_clear(&config.clientDescription.applicationUri);
+ config.clientDescription.applicationUri =
UA_STRING_ALLOC(application_uri.c_str());
+ }
+
+ client_ = UA_Client_newWithConfig(&config);
+ if (client_ == nullptr) {
+ UA_ClientConfig_clear(&config);
+ throw OPCException(GENERAL_EXCEPTION, "Failed to allocate the OPC UA
client");
}
logger_ = logger;
diff --git a/extensions/opc/tests/PutOpcProcessorTests.cpp
b/extensions/opc/tests/PutOpcProcessorTests.cpp
index e031fe736..583dc0cf4 100644
--- a/extensions/opc/tests/PutOpcProcessorTests.cpp
+++ b/extensions/opc/tests/PutOpcProcessorTests.cpp
@@ -22,6 +22,7 @@
#include "include/putopc.h"
#include "utils/StringUtils.h"
#include "unit/TestUtils.h"
+#include "minifi-cpp/utils/gsl.h"
namespace org::apache::nifi::minifi::test {
@@ -69,6 +70,10 @@ void verifyCreatedNode(const NodeData& expected_node,
SingleProcessorTestControl
ref_desc.nodeId.nodeId = found_node_ids[0];
ref_desc.browseName = UA_QUALIFIEDNAME_ALLOC(expected_node.namespace_index,
expected_node.browse_name.c_str());
ref_desc.displayName = UA_LOCALIZEDTEXT_ALLOC("en-US",
expected_node.browse_name.c_str());
+ const auto ref_desc_guard = gsl::finally([&ref_desc] {
+ UA_LocalizedText_clear(&ref_desc.displayName);
+ UA_QualifiedName_clear(&ref_desc.browseName);
+ });
ref_desc.nodeClass = UA_NODECLASS_VARIABLE;
ref_desc.typeDefinition.nodeId = UA_NODEID_NUMERIC(0, UA_NODEIDTYPE_NUMERIC);
auto data = client->getNodeData(&ref_desc, expected_node.path);
