Fix MQTT client: reset use_tls flag in connection_end (TLS to plain reconnect)#400
Open
EdouardMALOT wants to merge 1 commit into
Open
Fix MQTT client: reset use_tls flag in connection_end (TLS to plain reconnect)#400EdouardMALOT wants to merge 1 commit into
EdouardMALOT wants to merge 1 commit into
Conversation
ee120f6 to
116330e
Compare
…econnect) _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 afterwards. A subsequent non-secure connect on the same client inherits the stale flag, triggers nx_secure_tls_session_start on a plain TCP socket, and returns NXD_MQTT_CONNECT_FAILURE (0x10005). Clear the flag in _nxd_mqtt_client_connection_end, after the TCP socket is disconnected and unbound: while the socket can still deliver data, a late TLS record (e.g. the peer's close_notify) must keep failing through the TLS receive path rather than being parsed as plaintext MQTT bytes. Clearing the flag before the socket teardown would let such a record be misread as a partial MQTT message and parked forever in nxd_mqtt_client_processing_packet, leaking a packet (caught by netx_mqtt_packet_leak_test). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
116330e to
119b3af
Compare
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The MQTT client cannot make a non-secure connection after a secure one on the same client instance.
_nxd_mqtt_client_secure_connect()setsnxd_mqtt_client_use_tls = 1before 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 subsequentnxd_mqtt_client_connect()(plain) on the same client inherits the stale flag, callsnx_secure_tls_session_start()on the TLS session that was just deleted, and fails withNXD_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_CLIENTinstance.Changes
addons/mqtt/nxd_mqtt_client.c— in_nxd_mqtt_client_connection_end(), clearnxd_mqtt_client_use_tlsafter 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 innxd_mqtt_client_processing_packet, leaking a packet (caught bynetx_mqtt_packet_leak_testin therequire_secure_buildconfiguration). With the flag cleared only afternx_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 throughnxd_mqtt_client_connect()ornxd_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 getNXD_MQTT_CONNECT_FAILURE; it now gets exactly what it requested — a plaintext connection — matching the documented semantics of the two connect APIs.Test plan
netx_mqtt_packet_leak_testin all build configurations)