arduino-libraries / ArduinoBLE

ArduinoBLE library for Arduino
GNU Lesser General Public License v2.1
310 stars 205 forks source link

Fixes for memory leaks for Peers and around Services, Characteristics and Descriptors #129

Open dreamstyler opened 3 years ago

dreamstyler commented 3 years ago

I found some other issues after posting this #127

1) Add ATTClass end function to clean up _peers and call from BLELocalDevice::end() can lead to memory leaks for _peers[i].device and other issues, especially if disconnect does not not happen nicely

void ATTClass::end()
{  
  for (int i = 0; i < ATT_MAX_PEERS; i++) {
    if (_peers[i].connectionHandle == 0xffff) {
      continue;
    }

    _peers[i].connectionHandle = 0xffff;
    _peers[i].role = 0x00;
    _peers[i].addressType = 0x00;
    memset(_peers[i].address, 0x00, sizeof(_peers[i].address));
    _peers[i].mtu = 23;

    if (_peers[i].device) {
      delete _peers[i].device;
      _peers[i].device = NULL;
    }
  }
}

void BLELocalDevice::end()
{
  GATT.end();
  ATT.end();
  HCI.end();

  // ....
 }

2) Fix ref counting for BLELocalDescriptor in BLELocalCharacteristic Needed for 3 otherwise will delete BLELocalDescriptor prematurely.

BLELocalCharacteristic::BLELocalCharacteristic(const char* uuid, uint8_t properties, int valueSize, bool fixedLength) :
  BLELocalAttribute(uuid),
  _properties(properties),
  _valueSize(min(valueSize, 512)),
  _valueLength(0),
  _fixedLength(fixedLength),
  _handle(0x0000),
  _broadcast(false),
  _written(false),
  _cccdValue(0x0000)
{
  memset(_eventHandlers, 0x00, sizeof(_eventHandlers));

  if (properties & (BLENotify | BLEIndicate)) {
    BLELocalDescriptor* cccd = new BLELocalDescriptor("2902", (uint8_t*)&_cccdValue, sizeof(_cccdValue));

    cccd->retain(); // CALL RETAIN HERE
    _descriptors.add(cccd);
  }

  _value = (uint8_t*)malloc(valueSize);
}

3) call retain twice GATTClass::addService(..) This is needed for 4, otherwise will cause a premature delete issue

void GATTClass::addService(BLELocalService* service)
{
  service->retain();
  _attributes.add(service);

  uint16_t startHandle = attributeCount();

  for (unsigned int i = 0; i < service->characteristicCount(); i++) {
    BLELocalCharacteristic* characteristic = service->characteristic(i);

    characteristic->retain();
    _attributes.add(characteristic);
    characteristic->setHandle(attributeCount());

    // add the characteristic again to make space of the characteristic value handle
    characteristic->retain();  // IMPORTANT: call this again since we are adding it twice
    _attributes.add(characteristic);

    ......
}

4) perform clean-up in GATTClass::end() Need to preform proper housekeeping here, otherwise it will lead to memory leaks.

void GATTClass::end()
{
  clearAttributes(); // call this instead of _attributes.clear();
}
dreamstyler commented 3 years ago

The following function should have a timeout to prevent getting locked up in some error condition. Might be good to have a way for the user to detect if such errors happen so they can take action.

int HCIClass::sendAclPkt(uint16_t handle, uint8_t cid, uint8_t plen, void* data)
{
  const unsigned long start = millis();
  while (_pendingPkt >= _maxPkt) {
    Serial.print(';');
    const int a = poll();
    if (a < 0)
      Serial.print(a);

    if (millis() > (start + 3000))
    {
      Serial.println("HCIClass::sendAclPkt TIMEOUT");
      break;
    }
  }

Ideally, all while loops should have a timeout guard if they poll some peripheral.