Added reporting of required memory to a call of nxd_mqtt_client_message_get() with insufficient memory

[hwmaier]

If nxd_mqtt_client_message_get() is called with insufficient memory, then currently there is no option to find out how much memory is required to perform a second call as suggested on https://docs.microsoft.com/en-us/azure/rtos/netx-duo/netx-duo-mqtt/chapter3#nxd_mqtt_client_message_get

This little PR adds reporting of the actual memory required in actual_topic_length and actual_message_length.

A caller can then adopt the strategy to first call nxd_mqtt_client_message_get() with both topic_buffer_size and message_buffer_size set to 0 and then use the values reported in actual_topic_length and actual_message_length to allocate the exact amount of memory required to store the message data.

[yuxin-azrtos]

Thanks for the suggestion. We will review this requirement internally and get back to you.

[hwmaier]

I like also like to add to the discussion the fact that a message can only be removed from the queue if sufficient memory has been allocated. So one needs to know how much is needed in advance. I don't see much value in providing this information only after the call has succeeded as per current implementation. In that case I had enough memory and would not care about how much would have been needed, I overallocated already.

The concept of providing zero length buffers to gather the size before allocation is a common practise in the standard C library and in this case adds little overhead.

[hwmaier]

Are there any updates on this review? It is a very simple change and would make mqtt usage much easier and more memory efficient as we can optimise the memory allocations to match what is actually needed rather over allocate all the time and hope we allocated enough.

[TiejunMS]

Sorry there is no updates recently. As for AzureRTOS convention, output parameters are meaningless when the return value is not successful. We will consider to add API to retrieve message length in the future.

[hwmaier]

Ok, I accept that you stay by your convention. Would you accept a PR for such an API call?

[TiejunMS]

Feel free to create a PR. But keep in mind, we can not merge PR in Github. We will merge it into our internal repo when get a chance. If your user application needs to retrieve the length of message, I suggest to access internal functions/data as a workaround.

        packet_ptr = client_ptr -> message_receive_queue_head;
        status = _nxd_mqtt_process_publish_packet(packet_ptr, &topic_offset, &topic_length, &message_offset, &message_length);

[hwmaier]

@TiejunMS Yes, that would be a workaround, thank you for the suggestion. But I rather like to see that in an official API than accessing internal data structured directly. Compared to a lot of other NetX methods, MQTT does not use zero-copy buffers and a user has to allocate a message buffer before retrieving the message. This buffer must be of sufficient size, so you must know how big the message will be BEFORE calling nxd_mqtt_client_message_get(). So a function like nxd_mqtt_client_message_size_get() would be very useful. Otherwise how do you make sure the buffer is large enough?

[TiejunMS]

You are right. MQTT does not support zero-copy. It was designed to send telemetry with small amount of data. While developing Azure IoT Middleware, we noticed JSON is used in some cases. So we have supported zero copy in Azure IoT Middleware. If your devices are connecting to Azure IoTHub, I would suggest you to use Azure IoT Middleware instead of MQTT directly. That is our first priority to simplify the user experience of connecting to Azure IoTHub.
See more details here: https://github.com/azure-rtos/netxduo/tree/master/addons/azure_iot

[hwmaier]

Unfortunately we have to support vanilla MQTT and digest arbitrary sized messages. So sensible and adaptive buffer sizing is important, we are on an embedded device and memory is finite.

I am right now in the process of implementing a sample nxd_mqtt_client_message_size_get() function. Once finished, tested and found workable, I publish it as a PR and will then close this PR hoping that one day it will make it into the official MQTT API.

[hwmaier]

Hello @TiejunMS,

here is a first implementation of a _nxd_mqtt_client_message_length_get() function to retrieve topic length and message length:

UINT _nxd_mqtt_client_message_length_get(NXD_MQTT_CLIENT *client_ptr, UINT *topic_length_ptr, UINT *message_length_ptr)
{

UINT                status;
NX_PACKET          *packet_ptr;
ULONG               topic_offset;
USHORT              topic_length;
ULONG               message_offset;
ULONG               message_length;
UINT                message_receive_queue_depth;

    tx_mutex_get(client_ptr -> nxd_mqtt_client_mutex_ptr, NX_WAIT_FOREVER);
    message_receive_queue_depth = client_ptr -> message_receive_queue_depth;
    packet_ptr = client_ptr -> message_receive_queue_head;

    while (message_receive_queue_depth)
    {
        status = _nxd_mqtt_process_publish_packet(packet_ptr, &topic_offset, &topic_length, &message_offset, &message_length);
        if (status == NXD_MQTT_SUCCESS)
        {
            {
                *topic_length_ptr = topic_length;
                *message_length_ptr = message_length;
                tx_mutex_put(client_ptr -> nxd_mqtt_client_mutex_ptr);
                return(NXD_MQTT_SUCCESS);
            }
        }

        packet_ptr = packet_ptr -> nx_packet_queue_next;
        message_receive_queue_depth--;
    }

    tx_mutex_put(client_ptr -> nxd_mqtt_client_mutex_ptr);
    return(NXD_MQTT_NO_MESSAGE);
}

[TiejunMS]

Hello @hwmaier , your code looks good to me. Thanks for your contribution! We will try to incorporate this feature in the release this year. Before that, could you maintain it in your user application?

[hwmaier]

@TiejunMS Thanks and sure I keep in the mean time my private extension.

While dealing with the memory allocation and the packet queue I also noticed one other potential issue which probably needs attention. Not sure if this is worth a separate issue report, but I'll mention it here for now.

If the application receives a large enough MQTT message which it fails to provision enough memory for, those messages will remain forever in the queue and the device won't be able to process MQTT any more. This leaves a device open to very simple DOS attacks by sending MQTT with large payloads. I believe there should be an official mechanism to clear the queue if _nxd_mqtt_client_message_get returns with NXD_MQTT_INSUFFICIENT_BUFFER_SPACE.

[TiejunMS]

I would not treat large MQTT data as DOS attacks. Same situation applies to all TCP/UDP packets. For network security, we depends on TLS session to provide certain level of protection. But I agree with you that it is convenient to allow user application to skip copying specific MQTT data. One possible solution in the future is, we can allow user application to receive MQTT packet directly instead of copying data.

[hwmaier]

Ordinary TCP/UDP packets can simply be released if they don't comply by content or size. The TLS protects only between device and broker. MQTT data can travel between brokers and sometimes there is no control where it comes from. So the application should always be able to sanitise the data and reject any data it does not accept without deadlocking. Like you suggested, a simple API to reject the packets and release them would suffice.