fix(plc4j/drivers/s7): release direct ByteBufs allocated for alarm-query reassembly#2543
Open
michele-tramonti wants to merge 1 commit intoapache:developfrom
Open
Conversation
…ry reassembly S7ProtocolLogic.decodeEventSubscriptionResponse() allocates two direct ByteBufs (`buffer` and `rxBuffer`) when handling S7PayloadUserDataItemCpuFunctionAlarmQueryResponse, but never releases them. ByteBufUtil.getBytes(rxBuffer) copies the bytes into a Java array but does not release the underlying Netty buffer. On a PLC that emits alarms frequently, this leaks two direct buffers per event, slowly exhausting MaxDirectMemorySize on long-running applications. It is the same family of leak as apache#2248 but in a different code path (alarm subscriptions rather than the multiplex outbound queue). Wrap the existing logic in a try/finally so both buffers are always released, including when an exception propagates out of the parsing block.
Contributor
|
Hi Michele, thanks for the PR ... however I should mention that in the SPI3 PR I'm currently completely rewriting all of our drivers to completely work without Netty and everything that was making our life miserable. Chris |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Second direct-memory leak I noticed while auditing #2248 (and its fix #2542). Same family of bug, different code path.
S7ProtocolLogic.decodeEventSubscriptionResponse()allocates two directByteBufs when handlingS7PayloadUserDataItemCpuFunctionAlarmQueryResponse:```java
ByteBuf buffer = Unpooled.directBuffer(items.getItems().length * 2);
ByteBuf rxBuffer = Unpooled.directBuffer(items.getItems().length * 2);
```
They are populated, then drained via `ByteBufUtil.getBytes(rxBuffer)` (which copies the bytes into a Java array) and the method returns. Neither buffer is ever `release()`d. `ByteBufUtil.getBytes` does not own the buffer and does not release it.
On a PLC that emits alarms regularly, every alarm event leaks two direct buffers. On long-running applications this slowly exhausts `MaxDirectMemorySize` and triggers `OutOfMemoryError: Cannot reserve N bytes of direct buffer memory` — the same symptom as #2248, different root cause.
Fix
Wrap the existing logic in a
try/finallyso both buffers are released on every path, including when the inner parsing block (or any of theloopFuture.get()calls) propagates an exception.No change in behaviour or wire format — the buffers are local to the method and only used for reassembly before the bytes are copied out via
ByteBufUtil.getBytes.Test plan
mvn -pl :plc4j-driver-s7 -P with-java testpasses