Double delete/free in BLERemoteService causes crash

[nelarsen]

I'm developing an arduino-esp32 program that connects to a BLE peripheral device. It might lose connection and reconnect to this device many times and I would like to eliminate or at least minimize any memory leaks. I have seen the free heap size shrink for each new reconnection until no more space is left. Until now I did not do any delete/free/cleanup in response to the BLE onDisconnect() event. I also haven't found any documentation about what to do in order to delete/free the created BLEClient, BLERemoteService|s, BLECharacteristic|s etc. I'm guessing that I should

delete pMyClient; /* instance of BLEClient */

to cleanup, since the destructor of BLEClient will delete the BLERemoteService|s whose destructors call removeCharacteristics() to delete the BLECharacteristic|s, whose destructors in turn call removeDescriptors to delete the BLERemoteDescriptor|s.

The above "delete" causes a crash. This is due to a bug in removeCharacteristics() of BLERemoteService.cpp. Each BLECharacteristic gets deleted twice, one time per loop:

void BLERemoteService::removeCharacteristics() {
	for (auto &myPair : m_characteristicMap) {
	   delete myPair.second;
	   //m_characteristicMap.erase(myPair.first);  // Should be no need to delete as it will be deleted by the clear
	}
	m_characteristicMap.clear();   // Clear the map
	for (auto &myPair : m_characteristicMapByHandle) {
	   delete myPair.second;
	}
	m_characteristicMapByHandle.clear();   // Clear the map
} // removeCharacteristics

It is already fixed in the nkolban/esp32-snippets repository with this commit. Both maps m_characteristicMap and m_characteristicMapByHandle refer to the same BLECharacteristic|s. One of the for-loops must therefore be removed without replacement.

A local patch indeed works: No crash calling the statement above anymore. The memory leak is also gone or much smaller. I still ask myself if calling the statement is right thing to do. Normally a user should only call "delete" if "new" was called before (not counting "new" in libraries). Any hints are welcome.

[wakwak-koba]

At that time, I participated in the correction work, but many corrections have been rolled back.
I don't doubt what happened.

[stale]

[STALE_SET] This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale]

[STALE_DEL] This stale issue has been automatically closed. Thank you for your contributions.

[nelarsen]

The bug is not fixed and pretty critical, IMHO. Can I contribute with a pull request (it would be my first)?