_nxd_mqtt_client_disconnect_notify_set does not acquire the mutex and races with _nxd_mqtt_process_disconnect on nxd_mqtt_disconnect_notify

[jespertragardh]

Describe the bug
https://github.com/eclipse-threadx/netxduo 8b6e03a
https://github.com/eclipse-threadx/netxduo/blob/8b6e03ac30ab688bec02c69d42f2304b7f72a202/addons/mqtt/nxd_mqtt_client.c

This code sets NXD_MQTT_CLIENT.nxd_mqtt_disconnect_notify without first acquiring the mutex NXD_MQTT_CLIENT.nxd_mqtt_client_mutex_ptr:

UINT _nxd_mqtt_client_disconnect_notify_set(NXD_MQTT_CLIENT *client_ptr, VOID (*disconnect_notify)(NXD_MQTT_CLIENT *))
{

    client_ptr -> nxd_mqtt_disconnect_notify = disconnect_notify;

    return(NXD_MQTT_SUCCESS);
}

This means it will race with _nxd_mqtt_process_disconnect called by the internal event processing thread in nxd_mqtt_client.c which is created and started by _nxd_mqtt_client_create_internal:

static VOID _nxd_mqtt_process_disconnect(NXD_MQTT_CLIENT *client_ptr)
{
    …

    if (client_ptr -> nxd_mqtt_client_state == NXD_MQTT_CLIENT_STATE_CONNECTED)
    {
        /* State changes from CONNECTED TO IDLE.  Call disconnect notify callback
           if the function is set. */
        disconnect_callback = NX_TRUE;
    }
    else if (client_ptr -> nxd_mqtt_client_state != NXD_MQTT_CLIENT_STATE_CONNECTING)
    {

        /* If state isn't CONNECTED or CONNECTING, just return. */
        return;
    }

    tx_mutex_put(client_ptr -> nxd_mqtt_client_mutex_ptr);

    /* End connection. */
    _nxd_mqtt_client_connection_end(client_ptr, NXD_MQTT_SOCKET_TIMEOUT);

    status = tx_mutex_get(client_ptr -> nxd_mqtt_client_mutex_ptr, TX_WAIT_FOREVER);

    …

    /* If a callback notification is defined, call it now. */
    if ((disconnect_callback == NX_TRUE) && (client_ptr -> nxd_mqtt_disconnect_notify))
    {
        client_ptr -> nxd_mqtt_disconnect_notify(client_ptr);
    }

The call chain is _nxd_mqtt_thread_entry -> _nxd_mqtt_client_event_process -> _nxd_mqtt_packet_receive_process -> _nxd_mqtt_process_disconnect

Expected behavior
Data in NXD_MQTT_CLIENT shared by the internal thread and the threadx threadx configuring it should be protected by the mutex.

Impact
In practice, the outside thread will probably set call nxd_mqtt_disconnect_notify before calling connect and a sane compiler will write the pointer value to memory no later than before returning from _nxd_mqtt_client_disconnect_notify_set. So I guess that on cache-coherent architectures where ThreadX is expected to run, this won't break although it is formally broken.

I don't believe this is a security issue.

[fdesbiens]

Thanks @jespertragardh. I will investigate.