New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mosquitto_reconnect seems memory leak when TLS error #592

Closed
smartdabao opened this Issue Oct 19, 2017 · 6 comments

Comments

Projects
None yet
4 participants
@smartdabao

smartdabao commented Oct 19, 2017

I found it seems like memory leak when mosquitto_reconnect was used if TLS error. Plz reffer net_mosq.c line 880-901, version 1.4.14

while(packet->to_process > 0){
write_length = _mosquitto_net_write(mosq, &(packet->payload[packet->pos]), packet->to_process);
if(write_length > 0){
#if defined(WITH_BROKER) && defined(WITH_SYS_TREE)
g_bytes_sent += write_length;
#endif
packet->to_process -= write_length;
packet->pos += write_length;
}else{
#ifdef WIN32
errno = WSAGetLastError();
#endif
if(errno == EAGAIN || errno == COMPAT_EWOULDBLOCK){
pthread_mutex_unlock(&mosq->current_out_packet_mutex);
return MOSQ_ERR_SUCCESS; //The packet is not free here
}else{
pthread_mutex_unlock(&mosq->current_out_packet_mutex);
switch(errno){
case COMPAT_ECONNRESET:
return MOSQ_ERR_CONN_LOST; //The packet is not free here
default:
return MOSQ_ERR_ERRNO; //The packet is not free here
}
}
}
}

Is that right?
@toast-uz

This comment has been minimized.

Contributor

toast-uz commented Oct 19, 2017

Could you show any evidence of the memory leak except for reviewing code?

@smartdabao

This comment has been minimized.

smartdabao commented Oct 20, 2017

We use TLS and mosquitto. The procedure is as follows.

  1. mosquitto_connect is ok.
  2. for NTP reason, TLS failed, lead to mosquitto_loop return ERROR. So we reconnect, and mosquitto_reconnect return OK. Every time call mosquitto_reconnect, memory will raise, until memory exhausted. Our code is as follows.
    while(run){
    rc = mosquitto_loop(mosq, -1, 1);
    if(run && rc){
    printf("connection error!\n");
    sleep(10);
    mosquitto_reconnect(mosq);
    }
@toast-uz

This comment has been minimized.

Contributor

toast-uz commented Oct 25, 2017

I'm not familiar with libmosquitto, so I cannot reproduce your situation.
But I guess the code you pointed is not wrong.
Because the packet = mosq->current_out_packet is a part of mosq that is a parameter of function _mosquitto_packet_write, then it should be handled at the caller of the function.

@ralight

This comment has been minimized.

Contributor

ralight commented Dec 22, 2017

I've not been able to reproduce your problem. I've used the partial bit of code you provided. Can you provide any more details?

@aaronovz1

This comment has been minimized.

aaronovz1 commented Oct 19, 2018

This is definitely still a problem in 1.5.3

[DEBUG] : Client 1234 sending CONNECT
[ERROR] : OpenSSL Error: error:14094416:SSL routines:ssl3_read_bytes:sslv3 alert certificate unknown
[ERROR] : loop_forever failed (8): A TLS error occurred.
==4572== 1,876,996 (21,424 direct, 1,855,572 indirect) bytes in 26 blocks are definitely lost in loss record 267 of 267
==4572==    at 0x4C2CECB: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4572==    by 0x714107: CRYPTO_malloc (in ...)
==4572==    by 0x6E7DBF: SSL_new (in ...)
==4572==    by 0x6D2C2A: net__socket_connect_step3 (net_mosq.c:599)
==4572==    by 0x6D3632: net__socket_connect (net_mosq.c:648)
==4572==    by 0x6D0BF7: mosquitto__reconnect.part.1 (connect.c:184)
==4572==    by 0x6D1DB1: mosquitto_loop_forever (loop.c:284)
==4572==    by 0x506D33: sm::comms::MQTTClient::eventLoop() (mqttclient.cpp:123)
==4572==    by 0x61F2A8: boost::function0<void>::operator()() const (function_template.hpp:769)
==4572==    by 0x431FCB: sm::thread::Thread::main() (thread.cpp:274)
==4572==    by 0x85E424: thread_proxy (thread.cpp:175)
==4572==    by 0x598C476: start_thread (pthread_create.c:463)

There is a check in connect.c that does the following:

if(mosq->sock != INVALID_SOCKET){
	net__socket_close(mosq); //close socket
}

If I remove the check and always close the socket, then the leaks go away.

@ralight

This comment has been minimized.

Contributor

ralight commented Oct 23, 2018

Thanks for the hint, I believe this is now fixed. Are you able to check the fixes branch and report back?

@ralight ralight added this to the 1.5.4 milestone Oct 23, 2018

@ralight ralight closed this in 0a9ee5b Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment