free(outbound_message->data) wrong?

[martin-ger]

As far as I understand lines 220 ff. in mqtt.c:

  if (mqttClient->mqtt_state.outbound_message != NULL) {
    if (mqttClient->mqtt_state.outbound_message->data != NULL)
    {
      os_free(mqttClient->mqtt_state.outbound_message->data);
      mqttClient->mqtt_state.outbound_message->data = NULL;
    }
}

are not neccessary or even wrong. "mqttClient->mqtt_state.outbound_message->data" is never allocated but when not NULL just a reference to the "mqttClient->mqtt_state.out_buffer". Am I missing something?

[someburner]

Well, this is not the original implementation of a lot of the code (i.e. first line in readme). But, in any case, dangling pointers are a thing and can indeed bite you when you have malloc'd data attached to a struct like this that gets passed around.

[someburner]

@martin-ger

I think I see what you're saying now. In the case of outbound_message->data, perhaps this is to handle a failed or abruptly terminated espconn. Looks like this occurs in mqtt_send_keepalive and mqtt_tcpclient_connect_cb.

// this is what is called
espconn_send(client->pCon, client->mqtt_state.outbound_message->data, client->mqtt_state.outbound_message->length);

// espconn_sent == alias for espconn_send
espconn_sent(struct espconn *espconn, uint8 *psent, uint16 length) { ... }

// and in espconn_sent.. (relevant portion shown)
pbuf = (espconn_buf*) os_zalloc(sizeof(espconn_buf));
if (pbuf == NULL)
   return ESPCONN_MEM;
else {
   /* Backup the application packet information for send more data */
   pbuf->payload = psent;
   pbuf->punsent = pbuf->payload;
   pbuf->unsent = length;
   pbuf->len = length;
   /* insert the espconn_pbuf to the list */
   espconn_pbuf_create(&pnode->pcommon.pbuf, pbuf);
   if (pnode->pcommon.ptail == NULL)
      pnode->pcommon.ptail = pbuf;
}
/* when set the data copy option. change the flag for next packet */
if (espconn_copy_disabled(pnode))
   pnode->pcommon.write_flag = false;
error = espconn_tcp_write(pnode);

That would also explain the surrounding checks, since the SDK will also delete this itself at some point if it is allowed to terminate normally. I could be wrong though.