Parts of the payload is sent twice for MQTT buffer length < data-length

[mh-ba-dg]

Hi there,

If the tx-buffer length for the MQTT client is smaller then the data sent, the function MqttClient_Publish_WritePayload() will send the start of the payload twice. Once, since the payload is set in MqttEncode_Publish for the remainder of the tx-buffer. Second, when MqttClient_Publish_WritePayload is called, since it does not take into account the values of intBuf_pos and intBuf_len.

I could fix it more or less by using this:

---
 third_party/tcpip/wolfmqtt/src/mqtt_client.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/third_party/tcpip/wolfmqtt/src/mqtt_client.c b/third_party/tcpip/wolfmqtt/src/mqtt_client.c
index e8c3324a..8458989b 100644
--- a/third_party/tcpip/wolfmqtt/src/mqtt_client.c
+++ b/third_party/tcpip/wolfmqtt/src/mqtt_client.c
@@ -1219,18 +1219,14 @@ static int MqttClient_Publish_WritePayload(MqttClient *client,
         } while (publish->buffer_pos < publish->total_len);
     }
     else if (publish->buffer_pos < publish->total_len) {
-        if (publish->buffer_pos > 0) {
-            XMEMCPY(client->tx_buf, publish->buffer,
-                client->write.len);
-        }
+//        if (publish->buffer_pos > 0) {
+//            XMEMCPY(client->tx_buf, publish->buffer,
+//                client->write.len);
+//        }

         /* Send packet and payload */
         do {
-            rc = MqttPacket_Write(client, client->tx_buf,
-                    client->write.len);
-            if (rc < 0) {
-                return rc;
-            }
+

             publish->intBuf_pos += publish->intBuf_len;
             publish->intBuf_len = 0;
@@ -1249,6 +1245,12 @@ static int MqttClient_Publish_WritePayload(MqttClient *client,
             publish->intBuf_len = client->write.len;
             XMEMCPY(client->tx_buf, &publish->buffer[publish->intBuf_pos],
                 client->write.len);
+            
+            rc = MqttPacket_Write(client, client->tx_buf,
+                    client->write.len);
+            if (rc < 0) {
+                return rc;
+            }

         #ifdef WOLFMQTT_NONBLOCK
             return MQTT_CODE_CONTINUE;
-- 

But I am not sure if this is the way to go...

Environment: Microchip Harmony v2.6 OS: FreeRTOS Defines:

define WOLFSSL_BASE64_ENCODE

define WOLFMQTT_CUSTOM_MALLOC

define WOLFMQTT_MALLOC(s) pvPortMalloc((s))

define WOLFMQTT_FREE(p) vPortFree((p))

define ENABLE_MQTT_TLS

define WOLFMQTT_NONBLOCK

define MICROCHIP_MPLAB_HARMONY

define MICROCHIP_TCPIP

define MICROCHIP_PIC32

define MICROCHIP_PIC32_RNG

[embhorn]

Hi @Daniel-Gruber

Could you please attach a snippet showing the wolfMQTT API used to reproduce this error?

Thanks, Eric Blankenhorn wolfSSL Support

[mh-ba-dg]

Im glad to. Basically my code is based on the azure-example from MQTT v0.10. I just upgraded to 1.4. This is not the actual code, but might serve as well:

#define NUI16_IO_BUFFER_SIZE_RX 512U
#define NUI16_IO_BUFFER_SIZE_TX 512U
#define NUI16_COMMAND_TIMEOUT_MSEC 30000

tUINT8 pui8_RxBuffer[NUI16_IO_BUFFER_SIZE_RX]; 
tUINT8 pui8_TxBuffer[NUI16_IO_BUFFER_SIZE_TX];

static void Publish(char *ps_Payload)
{
    char response_topic[100];
    wms_snprintf(response_topic, 100, "$iothub/methods/res/%d/?$rid=%s", 200, sz_ResponseID);

    memset(&publish, 0, sizeof (MqttPublish));
    publish.retain = 0;
    publish.qos = DEFAULT_QOS;
    publish.duplicate = 0;
    publish.topic_name = response_topic;
    publish.packet_id = GetPacketId();
    publish.buffer = (byte*)ps_Payload;
    publish.total_len = 0;
    if (NULL != ps_Payload)
    {
        publish.total_len = strlen(ps_Payload) + 1;

        while (MQTT_CODE_CONTINUE == i16_Result)
        {
          i16_Result = MqttClient_Publish(&k_MqttClient, &publish);
          if (MQTT_CODE_CONTINUE == i16_Result)
          {      
             taskYIELD();
          }
        }
    }
}

void Init()
{
/// More init code
    i16_Result = MqttClient_Init(&k_MqttClient,
                                 &k_MqttNet,
                                 MessageCallback,
                                 pui8_TxBuffer,
                                 NUI16_IO_BUFFER_SIZE_TX,
                                 pui8_RxBuffer,
                                 NUI16_IO_BUFFER_SIZE_RX,
                                 NUI16_COMMAND_TIMEOUT_MSEC);
// even more init code
}

If you publish something like

char payload[4069]; // Filled with a string that is longer than NUI16_IO_BUFFER_SIZE_TX
Publish(payload);

parts of the payload will be sent twice.

[mh-ba-dg]

I made a full project for a PIC Processor for a different issue here: https://www.wolfssl.com/forums/topic1520-connection-losses-due-to-segmentation-errors.html

To reproduce this bug, just set

#define MAX_BUFFER_SIZE         1024    /* Maximum size for network read/write callbacks */

in mqtt_azure_segmentation\firmware\src\app.c and start the python script.

If you want, I can open a forum-thread to provide a wireshark trace.

[dgarske]

Hi @Daniel-Gruber ,

Following up to see how you are doing. Did you end up figuring out what was the issue with MAX_BUFFER_SIZE 1024 or is this still outstanding and something we need to look into?

Thanks, David Garske, wolfSSL

[embhorn]

This should be resolved by #162

[embhorn]

It looks like this was resolved by #162. Please feel free to open if there are other comments.