Skip to content

Fix potential CORRUPT HEAP problem on libraries/BLE/src/BLEDevice.cpp #7597

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Dec 21, 2022
Merged

Fix potential CORRUPT HEAP problem on libraries/BLE/src/BLEDevice.cpp #7597

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe0b931
Update BLEDevice.cpp
CurryEx Dec 18, 2022
e1303f8
move mux to BLEDevice class
CurryEx Dec 18, 2022
ce32416
Merge branch 'master' into master
CurryEx Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions libraries/BLE/src/BLEDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,10 +629,15 @@ void BLEDevice::addPeerDevice(void* peer, bool _client, uint16_t conn_id) {
m_connectedClientsMap.insert(std::pair<uint16_t, conn_status_t>(conn_id, status));
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the same mux be used here and any place where m_connectedClientsMap is accessed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I'm noticed, the insert and other operations to m_connectedClientsMap will not be invoked concurrently. So, it should not be necessary🤔.

Copy link
Member

Choose a reason for hiding this comment

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

are you positive that insert and find/erase can not happen at the same time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

erase operation will happen at the same time above the situation of this pull request. find operation will not cause concurrent problem. Regretfully, I haven't found enough evidence so far to promise if the insert operation will be invoked simultaneously in this library, however this is unlikely to happen. Therefore, I only made the BLEDevice::removePeerDevice serializable.

}

//there may have some situation that invoking this function simultaneously, that will cause CORRUPT HEAP
//let this function serializable
portMUX_TYPE BLEDevice::mux = portMUX_INITIALIZER_UNLOCKED;
void BLEDevice::removePeerDevice(uint16_t conn_id, bool _client) {
portENTER_CRITICAL(&mux);
log_i("remove: %d, GATT role %s", conn_id, _client?"client":"server");
if(m_connectedClientsMap.find(conn_id) != m_connectedClientsMap.end())
m_connectedClientsMap.erase(conn_id);
portEXIT_CRITICAL(&mux);
}

/* multi connect support */
Expand Down
1 change: 1 addition & 0 deletions libraries/BLE/src/BLEDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ class BLEDevice {
static BLEAdvertising* m_bleAdvertising;
static esp_gatt_if_t getGattcIF();
static std::map<uint16_t, conn_status_t> m_connectedClientsMap;
static portMUX_TYPE mux;

static void gattClientEventHandler(
esp_gattc_cb_event_t event,
Expand Down