MEMORY LEAKS: PLEASE FIX!

toddaustin07 commented 3 years ago

I was finding that my device app memory usage was constantly growing so I've done some investigation using the Valgrind tool. I ran the example switch app for 24 hours: mostly just sitting idle, with a few on/off commands issued from the mobile app. I found that there are significant memory leaks in the core SDK code, most due to failure to free malloc'd memory. Here is the summary:

LEAK SUMMARY: ==7970== definitely lost: 3,462,608 bytes in 865,547 blocks ==7970== indirectly lost: 0 bytes in 0 blocks ==7970== possibly lost: 176 bytes in 6 blocks ==7970== still reachable: 46,148 bytes in 97 blocks ==7970== suppressed: 0 bytes in 0 blocks

By far, the biggest offender are constant calls to iot_os_timer_init which allocates memory that is never freed:

3,453,060 bytes in 863,265 blocks are definitely lost in loss record 134 of 134 ==7970== at 0x48476D4: malloc (vg_replace_malloc.c:307) ==7970== by 0x240B3: iot_os_timer_init (iot_os_util_posix.c:455) ==7970== by 0x1C95F: st_mqtt_yield (iot_mqtt_client.c:1029) ==7970== by 0x17A3F: _iot_main_task (iot_main.c:1135) ==7970== by 0x4916493: start_thread (pthread_create.c:486) ==7970== by 0x4C42577: ??? (clone.S:73)

Here is the second-largest leak originating from mbedtls_ssl_setup:

16,717 bytes in 1 blocks are still reachable in loss record 133 of 134 ==7970== at 0x484A2F4: calloc (vg_replace_malloc.c:760) ==7970== by 0x36213: mbedtls_ssl_setup (ssl_tls.c:5671) ==7970== by 0x2069F: _iot_net_tls_connect (iot_net_mbedtls.c:215) ==7970== by 0x1C087: _iot_mqtt_connect_net (iot_mqtt_client.c:786) ==7970== by 0x1CDA7: st_mqtt_connect (iot_mqtt_client.c:1147) ==7970== by 0x4CECB: _iot_es_mqtt_connect (iot_easysetup_st_mqtt.c:821) ==7970== by 0x4D367: iot_es_connect (iot_easysetup_st_mqtt.c:950) ==7970== by 0x16CFB: _do_iot_main_command (iot_main.c:819) ==7970== by 0x177A7: _iot_main_task (iot_main.c:1060) ==7970== by 0x4916493: start_thread (pthread_create.c:486) ==7970== by 0x4C42577: ??? (clone.S:73)

There are numerous other memory leaks as well and I will paste those here a bit later.

No long-running devices based on this code can be put into production until this issue is fixed. Please review all usage of malloc and calloc in all modules and ensure memory is appropriately freed.

The memory leak analysis above is from a device app that has already been provisioned. I have not done a similar memory leak analysis during an onboarding process. So please also review your onboarding-related code for similar memory leak problems.

Thank you for your prompt attention to this issue!

Kwang-Hui commented 3 years ago

Thanks for submitting issue. I'll check the issue and get back to you.

toddaustin07 commented 3 years ago

Thanks for looking at this, Kwang-Hui. I have a more complete detailed log of all memory leaks reported by the Valgrind tool which I will paste below.

Here is a summary of the offending functions:

iot_os_timer_init->malloc  (biggest issue)
    st_conn_init
    _iot_mqtt_run_read_stream
    _iot_mqtt_chunk_create
    _iot_mqtt_run_write_stream
    st_mqtt_create
    st_mqtt_yield
    _iot_mqtt_queue_init

_iot_net_tls_connect->mbedtls_net_connect->getaddrinfo->..->malloc

iot_os_mutex_init->malloc
    st_mqtt_create
    _iot_mqtt_queue_init

iot_os_thread_create->pthread_create->...->calloc
   (this one may not be an issue)

valgrind24hrlog1.log

Kwang-Hui commented 3 years ago

Hi, @toddaustin07

Your report was very useful. Really appreciate. We've added fix of posix port layer memory leaks. That would be part of 1.5.10 release - preview https://github.com/SmartThingsCommunity/st-device-sdk-c/tree/develop_rel_1_5_10/src/port/os/posix

But regarding the mbedtls part, I guess it might be tested during mqtt/tls connection. If the connection has finished, memory would be freed properly.

Thanks. Kwang-Hui Cho

SmartThingsCommunity / st-device-sdk-c