MQTTProperties_free coredump due to heap use after free

[wangkevin5626]

When I construct the following scenario：mqtt client connect and disconnect with broker frequently, I got the coredump as below:

Thread 1 (Thread 0xe7fb627ccc00 (LWP 7136)):

0 0x0000e7fbfdc12bd8 in MQTTProperties_free (props=0xe7fb4c01c650) at paho.mqtt.c/src/MQTTProperties.c:406

1 0x0000e7fbfdbfc914 in MQTTClient_emptyMessageQueue (client=client@entry=0xaaaafa9f79b8) at paho.mqtt.c/src/MQTTClient.c:580

2 0x0000e7fbfdbfdf68 in MQTTClient_cleanSession (client=0xaaaafa9f79b8) at paho.mqtt.c/src/MQTTClient.c:1140

3 MQTTClient_closeSession (client=0xaaaafa9f79b8, reason=, props=) at paho.mqtt.c/src/MQTTClient.c:1125

4 MQTTClient_disconnect1 (handle=handle@entry=0xaaaafab8ef18, timeout=, call_connection_lost=call_connection_lost@entry=1, stop=, stop@entry=1, reason=, props=) at paho.mqtt.c/src/MQTTClient.c:1963

5 0x0000e7fbfdbff3f4 in MQTTClient_disconnect_internal (handle=0xaaaafab8ef18, timeout=0) at paho.mqtt.c/src/MQTTClient.c:1986

6 MQTTClient_publish5 (handle=, handle@entry=0xaaaafab8ef18, topicName=, topicName@entry=0xe7fb40016570 "test/A", '0' <repeats 16 times>, payloadlen=, payload=, qos=, retained=, properties=, deliveryToken=, deliveryToken@entry=0xe7fb627cbcb4) at paho.mqtt.c/src/MQTTClient.c:2485

7 0x0000e7fbfdbff77c in MQTTClient_publishMessage5 (handle=, handle@entry=0xaaaafab8ef18, topicName=, topicName@entry=0xe7fb40016570 "test/A", '0' <repeats 16 times>, message=, message@entry=0xe7fb627cbcb8, deliveryToken=, deliveryToken@entry=0xe7fb627cbcb4) at paho.mqtt.c/src/MQTTClient.c:2535

8 0x0000e7fbfdbff850 in MQTTClient_publishMessage (handle=0xaaaafab8ef18, topicName=0xe7fb40016570 "test/A", '0' <repeats 16 times>, message=0xe7fb627cbcb8, deliveryToken=0xe7fb627cbcb4) at paho.mqtt.c/src/MQTTClient.c:2555

(gdb) p props $5 = (MQTTProperties ) 0xe7fb4c01c650 (gdb) p props $6 = {count = 542978613, max_count = 925906779, length = 1936547124, array = 0x6e612064656c646e} (gdb) p props->array[0] Cannot access memory at address 0x6e612064656c646e (gdb)

You can see the props->count is 542978613, which is abnormal.

[wangkevin5626]

I think, the MQTTClient_message may be received by client, and be freed by client (client's MsgArrived function), but paho needs to set it to NULL after client message arrived funciton called in MQTTClient_run:

rc = (*(m->ma))(m->context, qe->topicName, topicLen, qe->msg);

 qe->msg = NULL;    // need to set  qe->msg to NULL

and we need to judge whther qe->msg is NULL in MQTTClient_emptyMessageQueue

static void MQTTClient_emptyMessageQueue(Clients* client) 
{

    FUNC_ENTRY;
    /* empty message queue */
    if (client->messageQueue->count > 0)
    {
        ListElement* current = NULL;
        while (ListNextElement(client->messageQueue, &current))
        {
            qEntry* qe = (qEntry*)(current->content);
            free(qe->topicName);
                        if (qe->msg != NULL) {  // need to judge qe->msg
                            MQTTProperties_free(&qe->msg->properties);
                            free(qe->msg->payload);
                            free(qe->msg);
                            qe->msg = NULL;
                        }
        }
        ListEmpty(client->messageQueue);
    }
    FUNC_EXIT;
}

[icraggs]

If the message is received correctly by the messageArrived callback, then the message is removed from the message queue entirely, so it should never be in a state where the topicName exists and the message does not in the queue entry.

For this to happen correctly, then the message arrived callback should return 1 when the message and topic are freed, as shown in the MQTTClient_subscribe.c sample:

int msgarrvd(void *context, char *topicName, int topicLen, MQTTClient_message *message)
{
    printf("Message arrived\n");
    printf("     topic: %s\n", topicName);
    printf("   message: %.*s\n", message->payloadlen, (char*)message->payload);
    MQTTClient_freeMessage(&message);
    MQTTClient_free(topicName);
    return 1;
}

That looks similar to what you have? What version of the library are you using? A library trace as described in the README might help narrow the situation down, or a test program.

[wangkevin5626]

If the message is received correctly by the messageArrived callback, then the message is removed from the message queue entirely, so it should never be in a state where the topicName exists and the message does not in the queue entry.

For this to happen correctly, then the message arrived callback should return 1 when the message and topic are freed, as shown in the MQTTClient_subscribe.c sample:

int msgarrvd(void *context, char *topicName, int topicLen, MQTTClient_message *message)
{
    printf("Message arrived\n");
    printf("     topic: %s\n", topicName);
    printf("   message: %.*s\n", message->payloadlen, (char*)message->payload);
    MQTTClient_freeMessage(&message);
    MQTTClient_free(topicName);
    return 1;
}

That looks similar to what you have? What version of the library are you using? A library trace as described in the README might help narrow the situation down, or a test program.

The coredump is from my pub thread (not from mqtt run thread with msgarrvd function), and the returncode rc is SOCKET_ERROR, the mqtt version I used is version 1.3.12

[wangkevin5626]

I think, the MQTTClient_message may be received by client, and be freed by client (client's MsgArrived function), but paho needs to set it to NULL after client message arrived funciton called in MQTTClient_run:

rc = (*(m->ma))(m->context, qe->topicName, topicLen, qe->msg);

 qe->msg = NULL;    // need to set  qe->msg to NULL

and we need to judge whther qe->msg is NULL in MQTTClient_emptyMessageQueue

static void MQTTClient_emptyMessageQueue(Clients* client) 
{

  FUNC_ENTRY;
  /* empty message queue */
  if (client->messageQueue->count > 0)
  {
      ListElement* current = NULL;
      while (ListNextElement(client->messageQueue, &current))
      {
          qEntry* qe = (qEntry*)(current->content);
          free(qe->topicName);
                        if (qe->msg != NULL) {  // need to judge qe->msg
                            MQTTProperties_free(&qe->msg->properties);
                            free(qe->msg->payload);
                            free(qe->msg);
                            qe->msg = NULL;
                        }
      }
      ListEmpty(client->messageQueue);
  }
  FUNC_EXIT;
}

This solution can not fix this problem, because mqttclient_mutex is unlocked before mqtt msg arrived funtion in MQTTClient_run thread， and we may have concurrency problem between pub thread and MQTTClient_run thread.

Paho_thread_unlock_mutex(mqttclient_mutex);
rc = (*(m->ma))(m->context, qe->topicName, topicLen, qe->msg);
Paho_thread_lock_mutex(mqttclient_mutex);