Avoid returning uninitialized packet_id on ACLK publish failure by stelfrag · Pull Request #22504 · netdata/netdata

stelfrag · 2026-05-18T09:48:46Z

Summary

aclk_send_bin_message_subtopic_pid was returning an uninitialized packet_id whenever the MQTT publish failed, since mqtt_wss_publish5 only writes the out-param on success.
That garbage value gets stored as the expected shutdown ack id in aclk_graceful_disconnect, and the PUBACK handler compares incoming ids against it — so a random match can falsely
declare the shutdown ack'd and exit the agent early.
Fix: initialize packet_id to 0 (the existing "no message" sentinel) and force it back to 0 on any non-OK publish return; same defensive init applied to
aclk_send_message_with_bin_payload.

Summary by cubic

Fixes a bug where failed publishes could return an uninitialized or stale packet_id, and hardens the publish path to prevent leaks and use-after-free. packet_id is now 0 on error (including oversize), logging happens before send, and messages are freed on generator failures.

Bug Fixes
- aclk_send_bin_message_subtopic_pid: initialize packet_id=0; reset to 0 when mqtt_wss_publish5 != MQTT_WSS_OK.
- aclk_send_message_with_bin_payload: initialize packet_id=0; publish after logging to avoid use-after-free.
- mqtt_ng_publish: on oversized messages or non-OK from mqtt_ng_generate_publish, null-check and set *packet_id=0; call msg_free(msg) on failure; only add to timeout list on success.
- Prevents false shutdown ACK detection and early exit.

^{Written for commit 33bdf6a. Summary will update on new commits. Review in cubic}

aclk_send_bin_message_subtopic_pid returned an uninitialized packet_id when mqtt_wss_publish5 failed. aclk_graceful_disconnect stores that value in mqtt_shutdown_msg_id and the PUBACK handler matches incoming packet_ids against it, so a garbage match could falsely declare the shutdown ack'd and trigger an early graceful exit. Initialize packet_id to 0 (the existing "no message" sentinel matching mqtt_shutdown_msg_id's -1 init and >0 check) and force it back to 0 on any non-OK return from mqtt_wss_publish5. Defensive init also added to aclk_send_message_with_bin_payload for the same pattern.

cubic-dev-ai

No issues found across 1 file

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

Copilot

Pull request overview

This PR fixes an ACLK/MQTT shutdown edge case where a failed publish could return an uninitialized packet_id, which could then be mistakenly treated as an expected PUBACK id and cause premature agent shutdown.

Changes:

Initialize packet_id to 0 in aclk_send_bin_message_subtopic_pid() and force it back to 0 when mqtt_wss_publish5() returns a non-OK status.
Defensively initialize packet_id to 0 in aclk_send_message_with_bin_payload().

Comments suppressed due to low confidence (1)

src/aclk/aclk_tx_msgs.c:43

mqtt_wss_publish5() may free msg immediately on publish failure (e.g., not connected/disconnecting or message too big) via the msg_free callback, but the code still calls protomsg_to_json(msg, ...) for conversation logging afterwards. This can dereference freed memory when aclklog_enabled is on. To avoid a UAF, generate the JSON/log payload before calling mqtt_wss_publish5(), or only log when rc == MQTT_WSS_OK (and ensure the log path never touches msg after ownership has been handed off).

    int rc = mqtt_wss_publish5(client, (char *)topic, NULL, msg, &freez_aclk_publish5a, msg_len, MQTT_WSS_PUB_QOS1, &packet_id);
    if (rc != MQTT_WSS_OK)
        packet_id = 0;

    if (aclklog_enabled) {
        char *json = protomsg_to_json(msg, msg_len, msgname);
        log_aclk_message_bin(json, strlen(json), 1, topic, msgname);
        freez(json);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

…nerator

cubic-dev-ai

1 issue found across 1 file (changes from recent commits).

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

…rom mqtt_ng_generate_publish

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

…OK returns

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

cubic-dev-ai

1 issue found across 1 file (changes from recent commits).

_{Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic}

sonarqubecloud · 2026-05-18T17:09:50Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

thiagoftsm

No issues were identified on cloud when running this PR. LGTM!

…ata#22504) * aclk: return packet_id=0 on publish failure aclk_send_bin_message_subtopic_pid returned an uninitialized packet_id when mqtt_wss_publish5 failed. aclk_graceful_disconnect stores that value in mqtt_shutdown_msg_id and the PUBACK handler matches incoming packet_ids against it, so a garbage match could falsely declare the shutdown ack'd and trigger an early graceful exit. Initialize packet_id to 0 (the existing "no message" sentinel matching mqtt_shutdown_msg_id's -1 init and >0 check) and force it back to 0 on any non-OK return from mqtt_wss_publish5. Defensive init also added to aclk_send_message_with_bin_payload for the same pattern. * aclk: move mqtt_wss_publish5 call after logging to avoid use after free * aclk: ensure msg_free contract holds on non-OK return from message generator * aclk: clear packet_id and ensure msg_free contract on non-OK return from mqtt_ng_generate_publish * aclk: clarify msg_free logic to ensure publish-layer contract on non-OK returns * aclk: add null check for packet_id before assignment on failure rollback * aclk: reset packet_id to 0 on oversized message error path

* aclk: return packet_id=0 on publish failure aclk_send_bin_message_subtopic_pid returned an uninitialized packet_id when mqtt_wss_publish5 failed. aclk_graceful_disconnect stores that value in mqtt_shutdown_msg_id and the PUBACK handler matches incoming packet_ids against it, so a garbage match could falsely declare the shutdown ack'd and trigger an early graceful exit. Initialize packet_id to 0 (the existing "no message" sentinel matching mqtt_shutdown_msg_id's -1 init and >0 check) and force it back to 0 on any non-OK return from mqtt_wss_publish5. Defensive init also added to aclk_send_message_with_bin_payload for the same pattern. * aclk: move mqtt_wss_publish5 call after logging to avoid use after free * aclk: ensure msg_free contract holds on non-OK return from message generator * aclk: clear packet_id and ensure msg_free contract on non-OK return from mqtt_ng_generate_publish * aclk: clarify msg_free logic to ensure publish-layer contract on non-OK returns * aclk: add null check for packet_id before assignment on failure rollback * aclk: reset packet_id to 0 on oversized message error path

github-actions Bot added the area/aclk label May 18, 2026

cubic-dev-ai Bot reviewed May 18, 2026

View reviewed changes

stelfrag marked this pull request as ready for review May 18, 2026 09:58

Copilot AI review requested due to automatic review settings May 18, 2026 09:58

stelfrag marked this pull request as draft May 18, 2026 09:58

stelfrag requested a review from thiagoftsm May 18, 2026 09:58

Copilot started reviewing on behalf of stelfrag May 18, 2026 10:01 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

aclk: move mqtt_wss_publish5 call after logging to avoid use after free

e2bf6ac

stelfrag marked this pull request as ready for review May 18, 2026 10:31

stelfrag requested a review from Copilot May 18, 2026 10:31

Copilot started reviewing on behalf of stelfrag May 18, 2026 10:33 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/aclk_tx_msgs.c

aclk: ensure msg_free contract holds on non-OK return from message ge…

13f5609

…nerator

cubic-dev-ai Bot reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c Outdated

aclk: clear packet_id and ensure msg_free contract on non-OK return f…

238ab8b

…rom mqtt_ng_generate_publish

stelfrag requested a review from Copilot May 18, 2026 10:54

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c Outdated

Copilot started reviewing on behalf of stelfrag May 18, 2026 11:02 View session

aclk: clarify msg_free logic to ensure publish-layer contract on non-…

7786747

…OK returns

stelfrag requested a review from Copilot May 18, 2026 12:31

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c

Copilot started reviewing on behalf of stelfrag May 18, 2026 12:41 View session

stelfrag requested a review from Copilot May 18, 2026 15:44

Copilot started reviewing on behalf of stelfrag May 18, 2026 15:45 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c Outdated

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c Outdated

aclk: add null check for packet_id before assignment on failure rollback

07b6034

cubic-dev-ai Bot reviewed May 18, 2026

View reviewed changes

Comment thread src/aclk/mqtt_websockets/mqtt_ng.c

aclk: reset packet_id to 0 on oversized message error path

33bdf6a

stelfrag requested a review from Copilot May 18, 2026 17:08

Copilot started reviewing on behalf of stelfrag May 18, 2026 17:09 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

thiagoftsm approved these changes May 18, 2026

View reviewed changes

stelfrag merged commit 2eb7d2f into netdata:master May 19, 2026
218 of 222 checks passed

stelfrag deleted the aclk_fix_packet_id_uninit branch May 19, 2026 06:17

stelfrag mentioned this pull request Jul 12, 2026

Patch release 2.10.4 #22825

Merged

Uh oh!

Conversation

stelfrag commented May 18, 2026 • edited by cubic-dev-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Summary by cubic

Uh oh!

cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

cubic-dev-ai Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sonarqubecloud Bot commented May 18, 2026

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

thiagoftsm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stelfrag commented May 18, 2026 •

edited by cubic-dev-ai Bot

Loading

cubic-dev-ai Bot left a comment •

edited

Loading

cubic-dev-ai Bot left a comment •

edited

Loading