Skip to content

Fix the handling of the protocol requirements for MQTT V5 topic aliases maximum #1507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alvin1221 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix the handling of the protocol requirements for MQTT V5 topic aliases maximum #1507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/nng/nng.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,7 @@ NNG_DECL uint8_t conn_param_get_will_qos(conn_param *cparam);
NNG_DECL uint8_t conn_param_get_will_retain(conn_param *cparam);
NNG_DECL uint8_t conn_param_get_protover(conn_param *cparam);
NNG_DECL uint16_t conn_param_get_keepalive(conn_param *cparam);
NNG_DECL uint16_t conn_param_get_topic_alias_max_out(conn_param *cparam);
NNG_DECL uint32_t conn_param_get_clientid_len(conn_param *cparam);
NNG_DECL void *conn_param_get_qos_db(conn_param *cparam);
NNG_DECL char *conn_param_get_ip_addr_v4(conn_param *cparam);
Expand Down
1 change: 1 addition & 0 deletions src/core/message.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ struct conn_param {
uint16_t rx_max;
uint16_t keepalive_mqtt;
uint16_t topic_alias_max;
uint16_t topic_alias_max_out;
uint8_t pro_ver;
uint8_t con_flag;
uint8_t clean_start;
Expand Down
6 changes: 6 additions & 0 deletions src/nng.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2518,6 +2518,12 @@ conn_param_get_keepalive(conn_param *cparam)
return cparam->keepalive_mqtt;
}

uint16_t
conn_param_get_topic_alias_max_out(conn_param *cparam)
{
return cparam->topic_alias_max_out;
}

uint8_t
conn_param_get_protover(conn_param *cparam)
{
Expand Down
1 change: 1 addition & 0 deletions src/sp/protocol/mqtt/mqtt_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,7 @@ conn_param_init(conn_param *cparam)
cparam->rx_max = 65535;
cparam->max_packet_size = 0;
cparam->topic_alias_max = 0;
cparam->topic_alias_max_out = 0;
cparam->req_resp_info = 0;
cparam->req_problem_info = 1;
cparam->auth_method = NULL;
Expand Down
46 changes: 35 additions & 11 deletions src/sp/transport/mqtt/broker_tcp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,23 +424,47 @@ tcptran_pipe_nego_cb(void *arg)
nni_list_append(&ep->waitpipes, p);
// Match happens before accept_cb. Make pipe id ready
tcptran_ep_match(ep);
if (p->tcp_cparam->max_packet_size == 0) {
conn_param *cparam = p->tcp_cparam;
property *props = cparam->properties;
if (props != NULL) {
// CONNECT's Topic Alias Maximum is client
// capability and must not be reused as
// broker's CONNACK capability.
property_remove(props, TOPIC_ALIAS_MAXIMUM);
}
if (cparam->max_packet_size == 0) {
// set default max packet size for client
p->tcp_cparam->max_packet_size =
p->conf == NULL
cparam->max_packet_size = p->conf == NULL
? NANO_MAX_PACKET_SIZE
: p->conf->client_max_packet_size;
if (p->tcp_cparam->properties != NULL) {
property_remove(p->tcp_cparam->properties,
MAXIMUM_PACKET_SIZE);
property_append(p->tcp_cparam->properties,
property_set_value_u32(MAXIMUM_PACKET_SIZE,
p->tcp_cparam->max_packet_size));
if (props != NULL) {
property_remove(
props, MAXIMUM_PACKET_SIZE);
property_append(props,
property_set_value_u32(
MAXIMUM_PACKET_SIZE,
cparam->max_packet_size));
}
}
// Advertise broker's Topic Alias Maximum in CONNACK
// for MQTT v5. Default to 65535 to allow clients to
// use Topic Aliases.
if (p->pro_ver == MQTT_PROTOCOL_VERSION_v5) {
if (props == NULL) {
props = property_alloc();
cparam->properties = props;
}
if (props != NULL) {
cparam->topic_alias_max_out = 65535;
property_append(props,
property_set_value_u16(
TOPIC_ALIAS_MAXIMUM,
cparam->topic_alias_max_out));
}
Comment on lines +427 to 463
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Build CONNACK properties from a dedicated list instead of mutating CONNECT properties

At Line 428, props is sourced from cparam->properties (CONNECT-side data) and then edited for CONNACK. Removing only TOPIC_ALIAS_MAXIMUM is partial; other CONNECT-only properties can still leak into CONNACK, and original CONNECT metadata is mutated in place.

Suggested direction 
- conn_param *cparam = p->tcp_cparam;
- property   *props  = cparam->properties;
+ conn_param *cparam = p->tcp_cparam;
+ property   *conn_props = cparam->properties;   // keep CONNECT properties
+ property   *props      = property_alloc();      // build CONNACK properties only

+ if (props == NULL) {
+     rv   = NNG_ENOMEM;
+     code = SERVER_UNAVAILABLE;
+     goto error;
+ }

+ // Use `props` only for CONNACK-legal properties (e.g. MAXIMUM_PACKET_SIZE,
+ // TOPIC_ALIAS_MAXIMUM), and keep CONNECT properties separate.
+ // (If your encoder currently reads cparam->properties for CONNACK,
+ // introduce/route a dedicated CONNACK property container.)

}
log_debug("max_packet_size of %.*s is %d",
p->tcp_cparam->clientid.len, p->tcp_cparam->clientid.body,
p->tcp_cparam->max_packet_size);
cparam->clientid.len, cparam->clientid.body,
cparam->max_packet_size);
nni_mtx_unlock(&ep->mtx);
return;
} else {
Expand Down
5 changes: 1 addition & 4 deletions src/supplemental/mqtt/mqtt_codec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4326,10 +4326,7 @@ encode_properties(nni_msg *msg, property *prop, uint8_t cmd)
nni_mqtt_msg_append_u8(msg, p->data.p_value.u8);
break;
case U16:
if (p->id == TOPIC_ALIAS_MAXIMUM)
nni_mqtt_msg_append_u16(msg, 0xFFFF);
else
nni_mqtt_msg_append_u16(msg, p->data.p_value.u16);
nni_mqtt_msg_append_u16(msg, p->data.p_value.u16);
break;
case U32:
nni_mqtt_msg_append_u32(msg, p->data.p_value.u32);
Expand Down
1 change: 1 addition & 0 deletions src/supplemental/mqtt/mqtt_msg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ nni_get_conn_param_from_msg(nni_msg *msg)
conn_ctx->rx_max = 65535;
conn_ctx->max_packet_size = 0;
conn_ctx->topic_alias_max = 0;
conn_ctx->topic_alias_max_out = 0;
conn_ctx->req_resp_info = 0;
conn_ctx->req_problem_info = 1;
conn_ctx->auth_method = NULL;
Expand Down
Loading