@cryptotaxi247 / netdata-1 / commits / 2eb7d2f9a

Avoid returning uninitialized packet_id on ACLK publish failure (#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

Stelios Fragkakis committed May 19, 2026 at 09:17 UTC 2eb7d2f9a23a0b282a0ba02403a4f6d18c2500c0
2 files changed +23 -5
src/aclk/aclk_tx_msgs.c
+6 -4
@@ -25,7 +25,7 @@ uint16_t aclk_send_bin_message_subtopic_pid(mqtt_wss_client client, char *msg, s
25 #ifndef ACLK_LOG_CONVERSATION_DIR
26 UNUSED(msgname);
27 #endif
28 - uint16_t packet_id;
28 + uint16_t packet_id = 0;
29 const char *topic = aclk_get_topic(subtopic);
30
31 if (unlikely(!topic)) {
@@ -33,21 +33,23 @@ uint16_t aclk_send_bin_message_subtopic_pid(mqtt_wss_client client, char *msg, s
33 return 0;
34 }
35
36 - mqtt_wss_publish5(client, (char *)topic, NULL, msg, &freez_aclk_publish5a, msg_len, MQTT_WSS_PUB_QOS1, &packet_id);
37 -
36 if (aclklog_enabled) {
37 char *json = protomsg_to_json(msg, msg_len, msgname);
38 log_aclk_message_bin(json, strlen(json), 1, topic, msgname);
39 freez(json);
40 }
41
42 + int rc = mqtt_wss_publish5(client, (char *)topic, NULL, msg, &freez_aclk_publish5a, msg_len, MQTT_WSS_PUB_QOS1, &packet_id);
43 + if (rc != MQTT_WSS_OK)
44 + packet_id = 0;
45 +
46 return packet_id;
47 }
48
49 #define V2_BIN_PAYLOAD_SEPARATOR "\x0D\x0A\x0D\x0A"
50 static short aclk_send_message_with_bin_payload(mqtt_wss_client client, json_object *msg, const char *topic, const void *payload, size_t payload_len)
51 {
50 - uint16_t packet_id;
52 + uint16_t packet_id = 0;
53 const char *str;
54 char *full_msg = NULL;
55 size_t len;
src/aclk/mqtt_websockets/mqtt_ng.c
+17 -1
@@ -1300,14 +1300,30 @@ int mqtt_ng_publish(struct mqtt_ng_client *client,
1300
1301 if (client->max_msg_size && PUBLISH_SP_SIZE + mqtt_ng_publish_size(topic, msg_len, topic_id) > client->max_msg_size) {
1302 nd_log(NDLS_DAEMON, NDLP_ERR, "Message too big for server: %zu", msg_len);
1303 + if (packet_id)
1304 + *packet_id = 0;
1305 if (msg_free)
1306 msg_free(msg);
1307 return MQTT_NG_MSGGEN_MSG_TOO_BIG;
1308 }
1309
1310 int rc = TRY_GENERATE_MESSAGE(mqtt_ng_generate_publish, topic, topic_free, msg, msg_free, msg_len, publish_flags, packet_id, topic_id);
1309 - if (rc == MQTT_NG_MSGGEN_OK)
1311 + if (rc == MQTT_NG_MSGGEN_OK) {
1312 add_packet_to_timeout_monitor_list(client, *packet_id);
1313 + } else {
1314 + // generator may have written *packet_id before rolling back; clear it so callers
1315 + // don't observe a stale id on failure
1316 + if (packet_id)
1317 + *packet_id = 0;
1318 + if (msg_free) {
1319 + // mqtt_ng_generate_publish has no fail_rollback path after frag_set_external_data
1320 + // succeeds, so on non-OK return msg was never linked to a fragment and the rollback
1321 + // cannot have freed it. Free here to keep the publish-layer contract (msg freed on
1322 + // every non-OK return) holding for all callers. If a future change adds a fallible
1323 + // step after frag_set_external_data, this branch will double-free and must be revisited.
1324 + msg_free(msg);
1325 + }
1326 + }
1327 return rc;
1328 }
1329