Repository navigation
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
🟡 Changes recommended
A critical timeout path still allows self-matching Will delivery to overwrite a partial retained write, and the regression test fails in wildcard-disabled builds because it truncates a SUBACK rather than a PUBLISH.
2 open issues
What changed in this PR
This PR addresses retained-message frame splicing in wolfMQTT’s static-memory broker by treating incomplete writes with bytes already sent as fatal.
Changes:
- Stop retained delivery after a partial write and return a network error for connection teardown.
- Add a short-write-then-timeout mock and a regression test checking that no further bytes follow the partial frame.
| File | Description |
|---|---|
tests/test_broker_connect.c |
Adds timeout injection and a static retained-delivery regression test. |
src/mqtt_broker.c |
Releases packet identifiers and aborts retained delivery after partial writes. |
Review effort: Balanced
Comment on lines
+6382
to
+6383
| deliver_rc = MQTT_CODE_ERROR_NETWORK; | ||
| break; |
| 0x31, 0x08, 0x00, 0x03, 'r', '/', 'b', 'T', 'W', 'O' | ||
| }; | ||
| static const byte subscribe_all[] = { | ||
| 0x82, 0x08, 0x00, 0x01, 0x00, 0x03, 'r', '/', '#', 0x00 |
In WOLFMQTT_STATIC_MEMORY builds BrokerRetained_DeliverToClient encodes each matching retained PUBLISH into bc->tx_buf and writes it directly. When a write stops part way (CONTINUE, or a select() timeout in BrokerPosix_Write), MqttSocket_Write leaves client.write.pos at the bytes already sent. The loop then encodes the next retained message into the same tx_buf, and the next write resumes at that offset inside the new packet. The subscriber receives the head of one PUBLISH joined to the tail of another; with different payload lengths it loses framing for the rest of the connection. Static builds keep nothing to resume a cut frame from. Stop the delivery once part of a frame is on the wire and fail the SUBSCRIBE, so the connection is closed the same way BrokerHandle_Publish handles a short fan-out write. A write that fails before sending any bytes keeps the existing behaviour. Add a mock mode that short-writes and then times out with the socket still open, and a test that fails without this change (15 bytes reach the subscriber instead of 3: the cut PUBLISH, the tail of the next one and the SUBACK).
Marti2203
force-pushed
the
broker-static-partial-write
branch
from
November 2, 2026 11:11
572d976 to
fe10546
Compare
Returning MQTT_CODE_ERROR_NETWORK alone left the socket open until the dispatcher's BrokerClient_AbnormalClose, which publishes the client's Will first. If the client is subscribed to its own Will topic, the Will fan-out wrote into tx_buf at the stale client.write.pos and spliced the frame again. Call BrokerStaticClient_Close before failing the SUBSCRIBE, as the queue-full branch of the same loop does; the closed client is no longer connected, so the Will fan-out skips it. Tests: use exact-topic filters so they also run with --disable-broker-wildcards (r/# was rejected there and the short write cut the SUBACK instead), and add static_retained_short_write_own_will_not_spliced, which fails without this change.
Member
|
Hello @Marti2203 Thanks for contacting wolfSSL. We typically do not accept small fixes. Would you be willing to convert this to a GitHub issue for us to review? Kind regards, |
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
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.


In
WOLFMQTT_STATIC_MEMORYbuilds, retained delivery can splice two PUBLISH frames on the wire when a write to the subscriber stops part way.Problem
BrokerRetained_DeliverToClient(static branch) encodes every matching retained message intobc->tx_bufand callsBrokerClient_WriteDirect. If the write stops after sending some bytes (MQTT_CODE_CONTINUE, orMQTT_CODE_ERROR_TIMEOUTwhen theselect()inBrokerPosix_Writetimes out),MqttSocket_Writeleavesclient.write.posat the number of bytes sent. The loop continues: the next retained PUBLISH is encoded into the sametx_buf, and its write resumes atwrite.posinside the new frame. The subscriber reads the fixed header and first bytes of message A followed by the tail of message B. If the payload lengths differ, it loses framing for the rest of the connection. Nothing is logged.Dynamic builds avoid this with
direct_pending_lenand the queued drain. The other static write paths (BrokerHandle_Publishfan-out,BrokerStaticOrphan_Drain) already close the connection when a write does not complete. The retained loop was the remaining path that kept going.Reproduction
Unmodified broker built with
--enable-broker --disable-tls CFLAGS=-DWOLFMQTT_STATIC_MEMORY, accepted sockets givenSO_SNDBUF=4096(viaLD_PRELOAD, standing in for a small embedded TCP stack). A publisher stores 16 retained 3500-byte messages. A subscriber subscribes with 3 filters (48 deliveries) and pauses before reading:t/0, payload ist/0bytes followed byt/4bytes)A 1.5 s pause is just over
BROKER_TIMEOUT_MS. The dynamic build delivers all 48 with no pause. The results were the same over three runs.Fix
When a retained write did not complete and part of the frame is on the wire (
client.write.pos != 0), release the packet identifier, close the transport withBrokerStaticClient_Close(as the queue-full branch of the same loop does), stop the loop and returnMQTT_CODE_ERROR_NETWORK. The SUBSCRIBE then fails, its SUBACK is not sent, and the dispatcher finishes withBrokerClient_AbnormalClose(Will published, Session kept according to its expiry). Because the transport is closed first, the client's own Will is not written after the cut frame when it is subscribed to its Will topic. A write that fails before any byte is sent keeps the current behaviour, sostatic_retained_releases_packet_id_on_write_failureis unchanged.With the patch, the subscriber in the setup above receives part of one frame and then the connection closes, with no spliced data. A subscriber that keeps up still receives all 48.
Tests
write_limit_then_timeout: a short write, then a timeout with the socket still open.static_retained_short_write_closes_without_splicing. On master it fails: 15 bytes reach the subscriber instead of 3 (the cut PUBLISH, the tail of the next one, and the SUBACK).static_retained_short_write_own_will_not_spliced, where the subscriber is subscribed to its own Will topic. It fails on master and on the first commit of this PR.--disable-broker-wildcards.tests/test_broker_connect, all passing, no new warnings: static, static + nonblock, static without wildcards, static + v5, static + v5 + persist, static + TLS, dynamic, dynamic + nonblock.Contributor agreement
I'll request the contributor agreement from support@wolfssl.com and reference this PR.
This issue was found by an LLM-Enhanced Static Analysis Engine and verified manually.