Fix MQTT client: reset use_tls flag in connection_end (TLS to plain reconnect)

[EdouardMALOT]

Summary

The MQTT client cannot make a non-secure connection after a secure one on the same client instance.

_nxd_mqtt_client_secure_connect() sets nxd_mqtt_client_use_tls = 1 before delegating to _nxd_mqtt_client_connect(), but the flag is never cleared anywhere — _nxd_mqtt_client_connection_end() ends and deletes the TLS session, yet leaves the flag set. A subsequent nxd_mqtt_client_connect() (plain) on the same client inherits the stale flag, calls nx_secure_tls_session_start() on the TLS session that was just deleted, and fails with NXD_MQTT_CONNECT_FAILURE (0x10005).

Typical scenario: a device provisioned over TLS that must fall back to (or be reconfigured for) a plain connection without recreating the NXD_MQTT_CLIENT instance.

Changes

• addons/mqtt/nxd_mqtt_client.c — in _nxd_mqtt_client_connection_end(), clear nxd_mqtt_client_use_tls after the TCP socket is disconnected and unbound.

The placement matters: clearing the flag before the socket teardown opens a window where a late TLS record (e.g. the peer's close_notify) is read through the plain TCP path and parsed as MQTT bytes; its fixed header bytes decode as a truncated MQTT message that gets parked forever in nxd_mqtt_client_processing_packet, leaking a packet (caught by netx_mqtt_packet_leak_test in the require_secure_build configuration). With the flag cleared only after nx_tcp_client_socket_unbind(), any late TLS data keeps failing through the TLS receive path exactly as it does today.

_nxd_mqtt_client_connection_end() is only reached on terminal paths (connect failure, disconnect processing, timeout, explicit disconnect); every reconnection goes back through nxd_mqtt_client_connect() or nxd_mqtt_client_secure_connect(), and the latter re-runs the TLS setup callback and sets the flag again — so secure reconnection is unaffected.

Behavioral note: an application that (incorrectly) called the plain nxd_mqtt_client_connect() to re-establish a previously secure session used to get NXD_MQTT_CONNECT_FAILURE; it now gets exactly what it requested — a plaintext connection — matching the documented semantics of the two connect APIs.

Test plan

• secure connect → disconnect → plain connect on the same client: previously failed with 0x10005, now succeeds
• secure connect → disconnect → secure connect: unchanged (TLS setup callback re-runs)
• plain-only and secure-only usage: unchanged
• MQTT regression suite (including netx_mqtt_packet_leak_test in all build configurations)

[fdesbiens]

Thank you — the diagnosis is right, and the PR description does a better job of explaining the reasoning than most. Confirming the parts I checked:

The root cause is exactly as described. nxd_mqtt_client_use_tls is assigned in precisely one place, _nxd_mqtt_client_secure_ connect() at nxd_mqtt_client.c:4178, and nothing in the file ever clears it. It is a set-once flag on a reusable client instance, so a secure connect permanently marks the instance as TLS.

The #ifdef NX_SECURE_ENABLE guard is necessary, not just decorative. The field itself is declared inside that guard in nxd_m qtt_client.h:353-354, so an unguarded assignment would not compile with NX_SECURE_ENABLE undefined. Worth stating because it loo ks like belt-and-braces at first glance.

The placement is right, and for the reason you give. The flag has to survive the nx_secure_tls_session_end() / nx_secure_tls _session_delete() pair just above at :2565-2569, since those are gated on it. And clearing it before the socket teardown would leave a window where _nxd_mqtt_packet_receive() (which selects the TLS or plain receive path on this same flag, at :660) could read a late TLS record through the plain path. Placing it after nx_tcp_client_socket_unbind() closes both. I have not reproduced the packet leak you describe, but the mechanism is consistent with the code, so I am taking your word for that part.

No silent-downgrade risk from the change itself. This was my main worry, so I checked all nine _nxd_mqtt_client_connection_end() call sites (:1113, :1893, :2280, :2314, :2337, :2430, :3789, :3827, :3845, :3860). Every one is terminal —
each is immediately followed by an error return or a nxd_mqtt_connect_notify with a failure status. In particular, nothing callsconnection_end and then returns NX_IN_PROGRESS, so there is no path where a non-blocking secure connect could reach _nxd_mqtt_tcp_establish_process() (:2272) with the flag freshly cleared and silently negotiate plaintext. There is also no auto-reconnect feature in this add-on, so your claim that every reconnection goes back through one of the two public connect calls holds.

The gap is finding 1: _nxd_mqtt_client_connect() has a hand-rolled teardown at :3736-3757 that never calls connection_end, so it does not clear the flag. Details inline.

On your behavioural note — an application that called plain nxd_mqtt_client_connect() on a previously secure client used to get NXD_MQTT_CONNECT_FAILURE and now gets a plaintext connection: I agree that is the correct semantics, and that the old failure was accidental rather than a safety feature. But since the accidental behaviour did prevent an unintended cleartext MQTT session, it is worth a line in the release notes so that anyone who was relying on it, knowingly or not, finds out from the changelog rather than from a packet capture. That is a maintainer call, not a change request.

One process item: please add a regression test (finding 2 — there is an existing test that is very close to covering this).

[fdesbiens]

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]

Fixed in 8385932, kept minimal as you suggested.

[fdesbiens]

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]

Done in 6ed1451

[fdesbiens]

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]

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.

[EdouardMALOT]

@fdesbiens All three findings are addressed.