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);