Crashes on client disconnects...

[pottendo]

hi,

I'm using the uMQTT broker on a esp8266 - this controller offers the MQTT broker and publishes and subscribes also using its 'publish/subscribe()' methods for one other client (an esp32, using another mqtt client).
I use the 'older' lwip 1.x, to not run into the 5 connection limit/issue, as suggested by the readme.

I'm trying to hunt down some stability issue (broker crashes on client disconnects) and I found this:

mqtt_server.cpp::MQTT_server_deleteClientCon around line 225

[...call to `os_free(...)' below line 225...]
if (mqttClientCon->connect_info.client_id != NULL) {
/* Don't attempt to free if it's the zero_len array */
if (((uint8_t *) mqttClientCon->connect_info.client_id) != zero_len_id)
	if (local_disconnect_cb != NULL) {
		local_disconnect_cb(mqttClientCon->pCon, mqttClientCon->connect_info.client_id);
	}
        os_free(mqttClientCon->connect_info.client_id);
    mqttClientCon->connect_info.client_id = NULL;
}
[...]

Following the comment, I suspect that this os_free() shall not be called when the client_id == zero_len_id
however, the brackets are missing.
@martin-ger, what do you think?

As I still experience crashes when clients disconnect, so there must be something else, maybe pointing also to a misplaced 'os_free' somewhere.

I tracked my issue down to here:

uMQTTBroker.cpp::uMQTTBroker::_onDisconnect(...)

void uMQTTBroker::_onDisconnect(struct espconn *pesp_conn, const char *client_id) {
IPAddress connAddr(pesp_conn->proto.tcp->remote_ip[0], pesp_conn->proto.tcp->remote_ip[1],
		   pesp_conn->proto.tcp->remote_ip[2], pesp_conn->proto.tcp->remote_ip[3]);

    TheBroker->onDisconnect(connAddr, (String)client_id);
}
[...]

this callback is called from the code above through the registered callback in local_disconnect_cb(mqttClientCon->pCon, ...)

the pointer pesp_conn->tcp from the first parameter (type struct espconn *) sometimes is 0 or invalid, pointing to some weird address.
adding the following sanity checks helps to avoid the crash, but probably does not cure the root cause.

void uMQTTBroker::_onDisconnect(struct espconn *pesp_conn, const char *client_id) {
    if (!pesp_conn->proto.tcp) {
        printf("WARNING: pesp_conn->tcp == 0 - not calling callback\n");
        return;
    }
    if ((((int)(pesp_conn->proto.tcp)) & 0x3fff0000) != 0x3fff0000) {
        printf("WARNING: pesp_conn->tcp != 0x3fffxxxx - not calling callback\n");
        return;
    }
    IPAddress connAddr(pesp_conn->proto.tcp->remote_ip[0], pesp_conn->proto.tcp->remote_ip[1],
	  	   pesp_conn->proto.tcp->remote_ip[2], pesp_conn->proto.tcp->remote_ip[3]);

    TheBroker->onDisconnect(connAddr, (String)client_id);
}

For now my testcase is stable, but this may be a coincidence.
Any thoughts on this are much appreciated.

bye, pottendo

[martin-ger]

You are surely right with the assuption, that os_free() must not be called on the static zero_len_id, will fix this right now.

With the pesp_conn->tcp I have to investigate further...

[VovanPitersky]

good
morning, I'm afraid that the error has not been fixed ((((
#68

[pottendo]

Hi, first I lived with the instability,  then used a public broker , but as this was too limiting, I decided to place an old Raspberry int the network which hosts the broker now.Sorry, not the answer you probably wanted to hear.Bye pottendoAm 22.09.2022 01:21 schrieb VovanPitersky ***@***.***>: good morning, I'm afraid that the error has not been fixed (((( #68 —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>

[VovanPitersky]

thank you so much for the answer
I was just going the opposite way from the broker on the switch to the broker on the esp,
it turns out that the broker on the esp is unstable and there is no stable version to wait for (((
and I thought to simplify the scheme)))
thanks again