Skip to content
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

Kernel panic when delete BLEClient object with descriptor characteristic #8010

Merged
merged 4 commits into from
Nov 30, 2023

Conversation

chesterkwan
Copy link
Contributor

Fix kernel panic issue when trying to release BLEClient when the target device have a ble descriptor.

Description of Change

When I try to release BLEClient to free memory, sometimes it causes a kernel panic. After tracing down the code, I discover that when it try to delete the characteristic map of the BLEClient, it will also have to remove the descriptors under it if it exists.

When removing the descriptor from the map in BLERemoteCharacteristic::removeDescriptors() function, the code will first remove the map entry and then will try to access that deleted entry to remove the descriptor object. Which caused the kernel panic as entry are not exist.

The fix is simply removing the line which remove the map entry, and let the following code to remove then descriptor object. The final clear after the for-loop will ensure the map being cleared.

Tests scenarios

Tested on ESP32C3 and ESP32

Fix kernel panic issue when trying to release BLEClient when the target device have a ble descriptor.
@CLAassistant
Copy link

CLAassistant commented Mar 30, 2023

CLA assistant check
All committers have signed the CLA.

@SuGlider
Copy link
Collaborator

@chesterkwan - Thanks for the PR!
Could you please provide a testing case code that causes the issue?

@VojtechBartoska VojtechBartoska added Area: BLE Issues related to BLE Resolution: More info needed More info must be provided in this issue labels Apr 4, 2023
@VojtechBartoska
Copy link
Contributor

Any updates on this @chesterkwan ?

@chesterkwan
Copy link
Contributor Author

chesterkwan commented May 4, 2023

Thanks for reminding me of the pull request, too busy on other projects and nearly forgot about it.

The following section is callback class to handle the connect and disconnect of the BLE device.
What I have done different from normal approach is deleting the BLEClient to prevent memory leak for multiple connect and disconnection.

The faulty condition is that when you try to disconnect a device with write or notify characteristic, it will cause a kernel panic. But if you try to disconnect a device only with read characteristic, everything goes fine.

After drilling into the library, and find out the write and notify both characteristic include a descriptor. And the kernel panic were caused by the code trying to remove an item which have already been removed.

Thanks

class HandCallback : public BLEClientCallbacks {
  void onConnect(BLEClient* pclient) {
    connected_hand = pclient;
    Serial.println("Connected");
  }

  void onDisconnect(BLEClient* pclient) {
    Serial.println("Disconnect");
    delete(pclient);
    connected_hand = NULL;
    if (connection_state == Connected){
      connection_state = Disconnect;
    }
  }
};

@chesterkwan
Copy link
Contributor Author

Any updates on this @chesterkwan ?

Any comment?

@VojtechBartoska VojtechBartoska added Status: Review needed Issue or PR is awaiting review and removed Resolution: More info needed More info must be provided in this issue labels May 25, 2023
@chesterkwan
Copy link
Contributor Author

Any updates on this @chesterkwan ?

Sorry is there any question regarding this pull request?

@VojtechBartoska
Copy link
Contributor

Hi @chesterkwan, not right now, it's awaiting the review from our side.

@VojtechBartoska VojtechBartoska added this to the 3.0.0-Aplha3 milestone Nov 28, 2023
@VojtechBartoska VojtechBartoska requested review from P-R-O-C-H-Y and lucasssvaz and removed request for me-no-dev, PilnyTomas and SuGlider November 28, 2023 12:18
Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@lucasssvaz lucasssvaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just a small suggestion.

libraries/BLE/src/BLERemoteCharacteristic.cpp Outdated Show resolved Hide resolved
Co-authored-by: Lucas Saavedra Vaz <[email protected]>
@chesterkwan
Copy link
Contributor Author

Committed. Thanks

@VojtechBartoska VojtechBartoska added Status: Pending Merge Pull Request is ready to be merged and removed Status: Review needed Issue or PR is awaiting review labels Nov 30, 2023
@me-no-dev me-no-dev merged commit 5fcdb84 into espressif:master Nov 30, 2023
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: BLE Issues related to BLE Status: Pending Merge Pull Request is ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants