Skip to content

Fixed the MQTT client use_tls flag not being reset for a plain reconnect #400

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

Merged
fdesbiens merged 4 commits into from
Aug 13, 2026
Merged

Fixed the MQTT client use_tls flag not being reset for a plain reconnect #400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
119b3af
Fix MQTT client: reset use_tls flag in connection_end (TLS to plain r…
EdouardMALOT Jun 26, 2026
8385932
Clean nxd_mqtt_client_use_tls on connect failure
EdouardMALOT Jul 31, 2026
6ed1451
Complete the caller list of _nxd_mqtt_client_connection_end
EdouardMALOT Jul 31, 2026
507583c
Add a regression test for a plain connection after a secure one
EdouardMALOT Jul 31, 2026
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
12 changes: 12 additions & 0 deletions addons/mqtt/nxd_mqtt_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2539,7 +2539,11 @@ UCHAR len[2];
/* CALLED BY */
/* */
/* _nxd_mqtt_client_connect */
/* _nxd_mqtt_client_websocket_connection_status_callback */
/* _nxd_mqtt_process_connack */
/* _nxd_mqtt_process_disconnect */
/* _nxd_mqtt_tcp_establish_process */
/* _nxd_mqtt_tls_establish_process */
/* */
/**************************************************************************/
VOID _nxd_mqtt_client_connection_end(NXD_MQTT_CLIENT *client_ptr, ULONG wait_option)
Expand Down Expand Up @@ -2571,6 +2575,10 @@ 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));

#ifdef NX_SECURE_ENABLE

@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.

We lack a matching regression test, and this one is worth having because — unlike a lot of state-cleanup bugs — it is fully deterministic. It will fail reliably on the unpatched code, which makes it a genuine regression test rather than just added coverage.

test/regression/mqtt_test/netx_mqtt_connect_test.c is already most of the way there. Its loop at :261-294 runs TEST_LOOP iterations on a single client with nxd_mqtt_client_disconnect() between them, doing a plain connect when i == 0 and a secure connect otherwise, and TEST_LOOP is 2 in the NX_SECURE_ENABLE build (:60). So it already exercises plain → disconnect → secure on one instance — which is the direction that works.

Reversing the order is the whole test: secure on i == 0, plain on i == 1. On dev the second iteration returns NXD_MQTT_CONNECT_FAILURE; with your fix it succeeds. Whether that belongs as a tweak to the existing test or a new netx_mqtt_tls_to_plain_reconnect_test.c is your call — a separate file is probably cleaner, since flipping the existing one would lose the plain → secure direction it currently covers.

Whichever you choose, please make it cover both teardown routes once finding 1 is addressed: one case going through a clean nxd_mqtt_client_disconnect(), and one going through a failed secure connect to an unreachable port. The second is the case that is still broken today.

@EdouardMALOT EdouardMALOT Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 507583c as test/regression/mqtt_test/netx_mqtt_tls_to_plain_reconnect_test.c, a separate file so netx_mqtt_connect_test.c keeps covering the plain → secure direction.

It checks that nxd_mqtt_client_use_tls is cleared on both teardown routes, by doing a plain connect on the same client instance after each: after a secure connect that failed at the TCP level, and after a secure connect ended with nxd_mqtt_client_disconnect().

Verified both ways: without the fix the test fails, with the fix it passes. Running it on top of the original commit of this PR fails on the first case only, so your finding 1 reproduces exactly as described.

client_ptr -> nxd_mqtt_client_use_tls = 0;

@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 right change in the right place, but _nxd_mqtt_client_connection_end() is not the only teardown in this file, and the other one has the same bug you are fixing.

In _nxd_mqtt_client_connect(), when the TCP connect itself fails, the error path at :3736-3757 does its own cleanup rather than calling connection_end:

status = nxd_tcp_client_socket_connect(&(client_ptr -> nxd_mqtt_client_socket), server_ip, server_port, wait_option);
if ((status != NX_SUCCESS) && (status != NX_IN_PROGRESS))
{
    ...
    client_ptr -> nxd_mqtt_client_state = NXD_MQTT_CLIENT_STATE_IDLE;
    ...
#ifdef NX_SECURE_ENABLE
    if (client_ptr -> nxd_mqtt_client_use_tls)
    {
        nx_secure_tls_session_delete(&(client_ptr -> nxd_mqtt_tls_session));
    }
#endif
    nx_tcp_client_socket_unbind(&(client_ptr -> nxd_mqtt_client_socket));
    tx_timer_delete(&(client_ptr -> nxd_mqtt_timer));
    return(NXD_MQTT_CONNECT_FAILURE);
}

It deletes the TLS session and unbinds the socket exactly as connection_end would, but leaves nxd_mqtt_client_use_tls set. So the failure mode from your PR description still reproduces after this patch, via a different route: call nxd_mqtt_client_secure_connect() while the broker is unreachable or refusing connections, then call plain nxd_mqtt_client_connect() on the same instance — the stale flag sends it to nx_secure_tls_session_start() on a deleted session and it fails with 0x10005.

That route is arguably the more likely one in the field. A device that cannot reach its broker at all is a far more common starting point for "fall back to a plain connection" than a clean secure session that was disconnected on purpose, which is the scenario your test plan covers.

The minimal fix is the same three lines you added, in that block after nx_tcp_client_socket_unbind():

#ifdef NX_SECURE_ENABLE
    client_ptr -> nxd_mqtt_client_use_tls = 0;
#endif

I would keep it minimal rather than refactoring that block to call connection_end, tempting as that looks. The two are deliberately different: this path skips nx_secure_tls_session_end() and nx_tcp_socket_disconnect() because nothing was ever established, and it deletes the timer unconditionally where connection_end only does so when nxd_mqtt_keepalive is set. Routing it through connection_end would change all three of those behaviours, which is more risk than this fix needs.

@EdouardMALOT EdouardMALOT Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8385932, kept minimal as you suggested.

@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.

Not introduced by you, but you are editing this function, and it is what made the review harder than it needed to be. The header block at :2539-2543 lists two callers:

/*  CALLED BY                                                             */
/*                                                                        */
/*    _nxd_mqtt_client_connect                                            */
/*    _nxd_mqtt_process_disconnect                                        */

There are in fact seven distinct callers: _nxd_mqtt_process_connack (:1113), _nxd_mqtt_process_disconnect (:1893), _nxd_mqtt_tcp_establish_process (:2280, :2314, :2337), _nxd_mqtt_tls_establish_process (:2430), _nxd_mqtt_client_connect (:3789, :3827, :3845, :3860), and _nxd_mqtt_client_websocket_connection_status_callback (:5861).

For a function whose whole job is teardown, and where the correctness of your change depends on every caller being on a terminal path, an accurate caller list has real value to the next reviewer. Worth completing while you are here.

@EdouardMALOT EdouardMALOT Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 6ed1451

#endif

/* Disable timer if timer has been started. */
if (client_ptr -> nxd_mqtt_keepalive)
{
Expand Down Expand Up @@ -3752,6 +3760,10 @@ UINT old_priority;
}
#endif /* NX_SECURE_ENABLE */
nx_tcp_client_socket_unbind(&(client_ptr -> nxd_mqtt_client_socket));

#ifdef NX_SECURE_ENABLE
client_ptr -> nxd_mqtt_client_use_tls = 0;
#endif /* NX_SECURE_ENABLE */
tx_timer_delete(&(client_ptr -> nxd_mqtt_timer));
return(NXD_MQTT_CONNECT_FAILURE);
}
Expand Down
3 changes: 2 additions & 1 deletion test/cmake/mqtt/regression/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ set(mqtt_test_cases
${SOURCE_DIR}/mqtt_test/netx_mqtt_branch_test.c
${SOURCE_DIR}/mqtt_test/netx_mqtt_transmit_queue_depth_test.c
${SOURCE_DIR}/mqtt_test/netx_mqtt_websocket_non_block_test.c
${SOURCE_DIR}/mqtt_test/netx_mqtt_websocket_block_test.c)
${SOURCE_DIR}/mqtt_test/netx_mqtt_websocket_block_test.c
${SOURCE_DIR}/mqtt_test/netx_mqtt_tls_to_plain_reconnect_test.c)

set(test_utility_files
${SOURCE_DIR}/test/nx_ram_network_driver_test_1500.c
Expand Down
2 changes: 2 additions & 0 deletions test/regression/mqtt_test/netx_mqtt_testcontrol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ void netx_mqtt_client_connack_error_application_define(void *);
void netx_mqtt_client_branch_application_define(void *);
void netx_mqtt_websocket_non_block_test_application_define(void *);
void netx_mqtt_websocket_block_test_application_define(void *);
void netx_mqtt_tls_to_plain_reconnect_application_define(void *);
#ifdef CTEST
void test_application_define(void *);
#endif
Expand Down Expand Up @@ -142,6 +143,7 @@ TEST_ENTRY test_control_tests[] =
{netx_mqtt_client_branch_application_define, TEST_TIMEOUT_LOW},
{netx_mqtt_websocket_non_block_test_application_define, TEST_TIMEOUT_LOW},
{netx_mqtt_websocket_block_test_application_define, TEST_TIMEOUT_LOW},
{netx_mqtt_tls_to_plain_reconnect_application_define, TEST_TIMEOUT_LOW},
#endif /* CTEST */

{TX_NULL, TEST_TIMEOUT_LOW},
Expand Down
Loading