Skip to content

Fix MQTT client: clear credentials and will on connection teardown #402

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

Closed
EdouardMALOT wants to merge 1 commit into from
Closed

Fix MQTT client: clear credentials and will on connection teardown #402

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
25 changes: 18 additions & 7 deletions addons/mqtt/nxd_mqtt_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1950,13 +1950,6 @@ UCHAR fixed_header;
client_ptr -> nxd_mqtt_ping_sent_time = 0;
}

/* Clean up the information when disconnecting. */
client_ptr -> nxd_mqtt_client_username = NX_NULL;
client_ptr -> nxd_mqtt_client_password = NX_NULL;
client_ptr -> nxd_mqtt_client_will_topic = NX_NULL;
client_ptr -> nxd_mqtt_client_will_message = NX_NULL;
client_ptr -> nxd_mqtt_client_will_qos_retain = 0;

/* Release current processing packet. */
if (client_ptr -> nxd_mqtt_client_processing_packet)
{
Expand Down Expand Up @@ -2571,6 +2564,14 @@ VOID _nxd_mqtt_client_connection_end(NXD_MQTT_CLIENT *client_ptr, ULONG wait_opt
nx_tcp_socket_disconnect(&(client_ptr -> nxd_mqtt_client_socket), wait_option);
nx_tcp_client_socket_unbind(&(client_ptr -> nxd_mqtt_client_socket));

/* Clean up per-connection information so a failed or ended connection
never leaks credentials or will settings into the next CONNECT. */
client_ptr -> nxd_mqtt_client_username = NX_NULL;

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a side effect of the move that is not mentioned in the PR description, and it is observable by applications.

In _nxd_mqtt_process_disconnect(), connection_end() is called at :1893, and the callbacks run afterwards at :1917-1927 — nxd_mqtt_disconnect_notify() and, on the connecting path, nxd_mqtt_connect_notify() with NXD_MQTT_CONNECT_FAILURE. The cleanup you are moving sat at :1953, i.e. after those callbacks.

So today a disconnect or connect-failure callback can still read client_ptr -> nxd_mqtt_client_username, nxd_mqtt_client_will_topic and friends. After this change they are all NULL by the time either callback fires. An application that logs which identity failed, or that inspects the will configuration to decide whether to re-arm, changes behaviour silently.

Please either preserve the ordering — keep the clear at the end of _nxd_mqtt_process_disconnect() and add it separately to the paths that need it — or state the new ordering explicitly in the documentation for both callbacks, so implementers know those fields are no longer valid inside them.

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To be clear up front: this is not a bug today. As noted in the summary, every CONNECT flag is driven off the pointer rather than the length, so nxd_mqtt_client_username_length, nxd_mqtt_client_password_length, nxd_mqtt_client_will_topic_length and nxd_mqtt_client_will_message_length are never read while their pointers are NULL.

But leaving them holding values that describe buffers the client no longer points at is a trap for whoever next touches _nxd_mqtt_client_connect_packet_send(). If anyone ever changes a flag test from if (pointer) to if (length) — a plausible tidy-up — the result is a CONNECT with the flag set and a NULL pointer handed to _nxd_mqtt_client_append_message().

Since you are already writing five assignments, zeroing the four lengths alongside them costs nothing and removes the trap. nxd_mqtt_client_will_qos_retain is already being cleared, so the block would then leave no partial state at all.

client_ptr -> nxd_mqtt_client_password = NX_NULL;
client_ptr -> nxd_mqtt_client_will_topic = NX_NULL;

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the part of your change I would keep regardless of how finding 1 is resolved, and it is a stronger argument than the one in the PR description.

The credentials and the will are not symmetric. _nxde_mqtt_client_login_set() accepts a NULL username with zero length, so an application can clear credentials through the public API. _nxde_mqtt_client_will_message_set() does the opposite — it rejects will_topic == NX_NULL outright with NXD_MQTT_INVALID_PARAMETER, and there is no will_message_clear() anywhere in the add-on.

So once an application sets a will, it has no supported way to unset it on that client instance. Its only options are to rely on the library's implicit clear, or to delete and recreate the client. That is a genuine API gap, and it is a much better justification for library-side clearing than the credential case is.

Two ways to close it properly, either of which I would welcome as a follow-up or as a reshaping of this PR:

  1. Relax _nxde_mqtt_client_will_message_set() to accept will_topic == NX_NULL with will_topic_length == 0, mirroring what login_set() already allows. Small, symmetric, and gives applications explicit control.
  2. Add an explicit clear entry point, if allowing NULL through the setter is considered too subtle.

Either would make the will case an application decision rather than a library side effect, which is the same footing the credentials are already on.

client_ptr -> nxd_mqtt_client_will_message = NX_NULL;
client_ptr -> nxd_mqtt_client_will_qos_retain = 0;

Comment on lines +2567 to +2574

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

_nxd_mqtt_client_connection_end() is reached from every terminal path, including the ones that today leave credentials intact: the CONNACK-refused path via _nxd_mqtt_process_connack() (:1113), the TCP and TLS establish failures (:2280, :2314, :2337, :2430), and the four error exits inside _nxd_mqtt_client_connect() (:3789, :3827, :3845, :3860).

None of those go through _nxd_mqtt_process_disconnect(), so none of them clear credentials today. After this change all of them do.

The consequence for an application that calls login_set() once at startup and then retries on failure — which is the normal shape of an embedded MQTT client — is that the first transient failure strips its credentials and every retry afterwards is anonymous and refused. A broker restart, an expired-then-renewed certificate, or a CONNACK "server unavailable" would each brick the connection until something re-sets the credentials.

Note the asymmetry that makes this easy to miss: a clean disconnect already clears them today, so an application that disconnects and reconnects is already forced to re-set. An application that only ever retries after failure is not. This change closes that gap, which is defensible — but it is a change in the documented contract of nxd_mqtt_client_login_set(), not a defect being repaired, and it should be called out as such in the changelog so that anyone relying on the current failure-path behaviour finds out from release notes rather than from a field failure.

If you would rather keep the blast radius small, an alternative that fixes your scenario without touching the retry case: clear on the paths that represent a completed connection ending, and leave the pre-CONNACK connect failures alone. That is a narrower rule and matches "the connection existed and is now over" rather than "an attempt failed".

@fdesbiens fdesbiens Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Worth recording the relationship explicitly so neither PR is merged thinking it is complete on its own.

This line is exactly the gap I raised on #400: that PR clears nxd_mqtt_client_use_tls in _nxd_mqtt_client_connection_end(), which misses this early TCP-connect-failure branch because it does its own hand-rolled teardown rather than calling connection_end. Your line here covers that branch. So #400 plus #402 together fix the stale-TLS-flag bug completely; either one alone leaves half of it.

The placement is right — inside the NX_SECURE_ENABLE guard, since the field only exists there, and after the conditional nx_secure_tls_session_delete() that reads it.

Two requests. First, since the two PRs are independent hunks, please make sure the commit message here mentions that it completes #400 rather than duplicating it, so the history is readable. Second, given how easy it was for a teardown path to be missed twice, it would be worth a follow-up that either routes this early-failure branch through connection_end() or documents why it deliberately does not — as I noted on #400, the two teardowns differ in three ways (no nx_secure_tls_session_end(), no nx_tcp_socket_disconnect(), unconditional timer delete), so a straight substitution is not safe today.

/* Disable timer if timer has been started. */
if (client_ptr -> nxd_mqtt_keepalive)
{
Expand Down Expand Up @@ -3750,9 +3751,19 @@ UINT old_priority;
{
nx_secure_tls_session_delete(&(client_ptr -> nxd_mqtt_tls_session));
}
client_ptr -> nxd_mqtt_client_use_tls = 0;
#endif /* NX_SECURE_ENABLE */
nx_tcp_client_socket_unbind(&(client_ptr -> nxd_mqtt_client_socket));
tx_timer_delete(&(client_ptr -> nxd_mqtt_timer));

/* Same per-connection cleanup as _nxd_mqtt_client_connection_end —
this early-failure path is the only teardown that does not go
through it. */
client_ptr -> nxd_mqtt_client_username = NX_NULL;
client_ptr -> nxd_mqtt_client_password = NX_NULL;
client_ptr -> nxd_mqtt_client_will_topic = NX_NULL;
client_ptr -> nxd_mqtt_client_will_message = NX_NULL;
client_ptr -> nxd_mqtt_client_will_qos_retain = 0;
return(NXD_MQTT_CONNECT_FAILURE);
}

Expand Down
Loading