You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "lordgamez (via GitHub)" <gi...@apache.org> on 2026/09/11 12:25:17 UTC
[PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
lordgamez opened a new pull request, #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261
Claude Code found a potential memory leak when allocating node id in OPC processors. In case of `translateBrowsePathsToNodeIdsRequest` calls or any other cases where `UA_NodeId` is allocated, and uses a node id type that is heap allocated like a string nodeid, the owner's responsibility is to free that node id using `UA_NodeId_clear` call when the id is no longer in use. This commit adds a RAII wrapper for the `UA_NodeId` to mitigate this issue. The PR additionally renames the OPC files to align with the filename conventions of the repository.
https://issues.apache.org/jira/browse/MINIFICPP-2900
-----------
Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.
In order to streamline the review of the contribution we ask you to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically main)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file?
- [ ] If applicable, have you updated the NOTICE file?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check GitHub Actions CI results for build issues and submit an update to your PR as soon as possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "fgerlits (via GitHub)" <gi...@apache.org>.
fgerlits merged PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5637600429
> Can you have Claude Code do a thorough review too, before we waste human reviewer time on trivial issues? It's giving better results than Copilot usually does.
>
> The first things I've noticed:
>
> * No gsl::owner annotation when taking ownership of the node id
>
> * Destructor should be noexcept
I ran Claude Code to review the commit, but it did not find any issues that was relevant to the commit and needs to be addressed.
I added noexcept to the destructor in https://github.com/apache/nifi-minifi-cpp/pull/2261/commits/7cbb097173228045e2e26afb43104153e138a5ee
`gsl::owner` is not applicable here as the UA_NodeId struct itself is stack allocated and the heap allocation depends on the actual type in its union member. The ownership logic is implemented in the UA_* types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5640259343
> Ownership of UA_NodeId is transferred to the RAII wrapper in its constructor, so I'd annotate the constructor parameter and the data member as owner.
>
> > Also, the id_ member is of type UA_NodeId, not UA_NodeId*, and gsl::owner can only be applied to a pointer type.
>
> I wasn't sure about this restriction, but if gsl::owner only applies to pointer types, then indeed we can't use it for this, and a comment is the best we can do.
I see now, that was the source of my confusion. Making it at construction would make sense, but when I checked the definition I saw that it can only be applied to a pointer, so I didn't understand where we would use it, that's why I pointed out previously that the `UA_NodeId` member itself is stack allocated. We can add a comment if nothing else.
```
template <class T, std::enable_if_t<std::is_pointer<T>::value, bool> = true>
using owner = T;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5638744736
Since the clear function operates with UA_NodeId, I would treat the internals as a black box for the purpose of cleanup, and always call UA_NodeId_clear on it, regardless of the union values. I'd treat UA_NodeId as a resource handle and UA_NodeId_clear its corresponding resource cleanup function, and resource handles can be owning or non-owning, that's why I think we should use gsl::owner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5640151120
Ownership of UA_NodeId is transferred to the RAII wrapper in its constructor, so I'd annotate the constructor parameter and the data member as owner.
> Also, the id_ member is of type UA_NodeId, not UA_NodeId*, and gsl::owner can only be applied to a pointer type.
I wasn't sure about this restriction, but if gsl::owner only applies to pointer types, then indeed we can't use it for this, and a comment is the best we can do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5636164007
Can you have Claude Code do a thorough review too, before we waste human reviewer time on trivial issues? It's giving better results than Copilot usually does.
The first things I've noticed:
- No gsl::owner annotation when taking ownership of the node id
- Destructor should be noexcept
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#issuecomment-5640119846
> Since the clear function operates with UA_NodeId, I would treat the internals as a black box for the purpose of cleanup, and always call UA_NodeId_clear on it, regardless of the union values. I'd treat UA_NodeId as a resource handle and UA_NodeId_clear its corresponding resource cleanup function, and resource handles can be owning or non-owning, that's why I think we should use gsl::owner.
I agree the internals should be treated as a black box. One of the reasons for this PR is that we had previously not tested node IDs with string-based underlying types, which is why the issue didn't show up in our memchecks. The fix now handles all union type cases the same way, clearing them every time, even though for integer node ID types the clear operation is a no-op.
That said, I still don't understand how gsl::owner could be used here. In the NodeId RAII wrapper, ownership is taken at construction and held until the end of its lifetime, so ownership never changes over the course of this solution. Also, the `id_` member is of type `UA_NodeId`, not `UA_NodeId*`, and `gsl::owner` can only be applied to a pointer type. In every function where we actually return a pointer, we deliberately return a non-owning one, since `NodeId` retains ownership of the `id_` member itself. Maybe I'm missing something, could you elaborate on how you pictured using `gsl::owner` here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "szaszm (via GitHub)" <gi...@apache.org>.
szaszm commented on code in PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#discussion_r4016019988
##########
extensions/opc/include/OPCCommon.h:
##########
@@ -58,6 +58,43 @@ enum class OPCNodeDataType{
String
};
+// RAII owner for a UA_NodeId that calls UA_NodeId_clear to free node id allocation
+class NodeId {
+ public:
+ NodeId() = default;
+ explicit NodeId(UA_NodeId id) noexcept : id_(id) {} // takes ownership of an already-built node id
+ NodeId(const NodeId&) = delete;
+ NodeId& operator=(const NodeId&) = delete;
+ NodeId(NodeId&& other) noexcept : id_(other.id_) { other.id_ = UA_NODEID_NULL; }
+ NodeId& operator=(NodeId&& other) noexcept {
+ if (this != &other) {
+ UA_NodeId_clear(&id_);
+ id_ = other.id_;
+ other.id_ = UA_NODEID_NULL;
+ }
+ return *this;
+ }
+ ~NodeId() noexcept { UA_NodeId_clear(&id_); }
+
+ static NodeId copyOf(const UA_NodeId& id) {
+ NodeId result;
+ UA_NodeId_copy(&id, &result.id_);
+ return result;
+ }
+
+ operator const UA_NodeId&() const noexcept { return id_; } // NOLINT(google-explicit-constructor) implicit passthrough to the C API is intended
+ [[nodiscard]] const UA_NodeId& get() const noexcept { return id_; }
+
+ // Returns a pointer to the (cleared) node id for an open62541 out-parameter to write a freshly created node id into.
+ UA_NodeId* receive() noexcept {
+ UA_NodeId_clear(&id_);
+ return &id_;
+ }
Review Comment:
what if the write fails, do we handle those error cases correctly? wouldn't it be cleaner to keep the out param local to the call, then wrap the result manually in this NodeId wrapper, and move-assign to existing wrapper objects as necessary?
##########
extensions/opc/include/OPCCommon.h:
##########
@@ -58,6 +58,43 @@ enum class OPCNodeDataType{
String
};
+// RAII owner for a UA_NodeId that calls UA_NodeId_clear to free node id allocation
+class NodeId {
+ public:
+ NodeId() = default;
+ explicit NodeId(UA_NodeId id) noexcept : id_(id) {} // takes ownership of an already-built node id
+ NodeId(const NodeId&) = delete;
+ NodeId& operator=(const NodeId&) = delete;
+ NodeId(NodeId&& other) noexcept : id_(other.id_) { other.id_ = UA_NODEID_NULL; }
+ NodeId& operator=(NodeId&& other) noexcept {
+ if (this != &other) {
+ UA_NodeId_clear(&id_);
+ id_ = other.id_;
+ other.id_ = UA_NODEID_NULL;
+ }
+ return *this;
+ }
+ ~NodeId() noexcept { UA_NodeId_clear(&id_); }
Review Comment:
Is this valid for UA_NODEID_NULL or already cleared objects?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#discussion_r4016174813
##########
extensions/opc/include/OPCCommon.h:
##########
@@ -58,6 +58,43 @@ enum class OPCNodeDataType{
String
};
+// RAII owner for a UA_NodeId that calls UA_NodeId_clear to free node id allocation
+class NodeId {
+ public:
+ NodeId() = default;
+ explicit NodeId(UA_NodeId id) noexcept : id_(id) {} // takes ownership of an already-built node id
+ NodeId(const NodeId&) = delete;
+ NodeId& operator=(const NodeId&) = delete;
+ NodeId(NodeId&& other) noexcept : id_(other.id_) { other.id_ = UA_NODEID_NULL; }
+ NodeId& operator=(NodeId&& other) noexcept {
+ if (this != &other) {
+ UA_NodeId_clear(&id_);
+ id_ = other.id_;
+ other.id_ = UA_NODEID_NULL;
+ }
+ return *this;
+ }
+ ~NodeId() noexcept { UA_NodeId_clear(&id_); }
Review Comment:
Yes, UA_NODEID_NULL is a numeric node id with 0 namespace and 0 id, when UA_NodeId_clear is called that is what the id is set to after the free operation. The UA_NodeId_clear operation is no-op for all numeric node ids, so nothing is done when it is called for an already cleared object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINIFICPP-2900 Fix leak of NodeID allocation in OPC processors [nifi-minifi-cpp]
Posted by "lordgamez (via GitHub)" <gi...@apache.org>.
lordgamez commented on code in PR #2261:
URL: https://github.com/apache/nifi-minifi-cpp/pull/2261#discussion_r4016253469
##########
extensions/opc/include/OPCCommon.h:
##########
@@ -58,6 +58,43 @@ enum class OPCNodeDataType{
String
};
+// RAII owner for a UA_NodeId that calls UA_NodeId_clear to free node id allocation
+class NodeId {
+ public:
+ NodeId() = default;
+ explicit NodeId(UA_NodeId id) noexcept : id_(id) {} // takes ownership of an already-built node id
+ NodeId(const NodeId&) = delete;
+ NodeId& operator=(const NodeId&) = delete;
+ NodeId(NodeId&& other) noexcept : id_(other.id_) { other.id_ = UA_NODEID_NULL; }
+ NodeId& operator=(NodeId&& other) noexcept {
+ if (this != &other) {
+ UA_NodeId_clear(&id_);
+ id_ = other.id_;
+ other.id_ = UA_NODEID_NULL;
+ }
+ return *this;
+ }
+ ~NodeId() noexcept { UA_NodeId_clear(&id_); }
+
+ static NodeId copyOf(const UA_NodeId& id) {
+ NodeId result;
+ UA_NodeId_copy(&id, &result.id_);
+ return result;
+ }
+
+ operator const UA_NodeId&() const noexcept { return id_; } // NOLINT(google-explicit-constructor) implicit passthrough to the C API is intended
+ [[nodiscard]] const UA_NodeId& get() const noexcept { return id_; }
+
+ // Returns a pointer to the (cleared) node id for an open62541 out-parameter to write a freshly created node id into.
+ UA_NodeId* receive() noexcept {
+ UA_NodeId_clear(&id_);
+ return &id_;
+ }
Review Comment:
This is already handled in the OPC UA client code in `src/client/ua_client_highlevel.c`. We are using this value as the out parameter of the add node operation, and the add node only touches the out parameter if the internal call succeeds otherwise it remains untouched:
```
/* Move the id of the created node */
retval = response.results[0].statusCode;
if(retval == UA_STATUSCODE_GOOD && outNewNodeId) {
*outNewNodeId = response.results[0].addedNodeId;
UA_NodeId_init(&response.results[0].addedNodeId);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org