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

Panic when trying to write_value() after the device disconnected #90

Closed
codepainters opened this issue Feb 18, 2024 · 2 comments
Closed

Comments

@codepainters
Copy link
Contributor

I observe a crash using esp32-nimble v0.5.1 .
The scenario is as follows:

  • I successfully connect to the BLE device with BLEClient::connect(), followed by get_service() and get_characteristic()
  • I periodically call BLERemoteCharacteristic::write_value() to control the device.
  • then I power off the device.

At that point my app panics:

thread 'main' panicked at /home/czajnik/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp32-nimble-0.5.1/src/client/ble_writer.rs:20:19:
attempt to subtract with overflow

Just before the panic I can see it disconnected the device:

I (67205) esp32_nimble::client::ble_client: Disconnected: 520

Corresponding line:

      let mtu = { esp_idf_sys::ble_att_mtu(self.conn_handle) - 3 } as usize;

Obviously the subtraction underflows, because ble_att_mtu() returns 0 if connection is gone:

/**
 * Retrieves the ATT MTU of the specified connection.  If an MTU exchange for
 * this connection has occurred, the MTU is the lower of the two peers'
 * preferred values.  Otherwise, the MTU is the default value of 23.
 *
 * @param conn_handle           The handle of the connection to query.
 *
 * @return                      The specified connection's ATT MTU, or 0 if
 *                                  there is no such connection.
 */
uint16_t ble_att_mtu(uint16_t conn_handle);

I could check the connection state before calling write_value(), but there's probably a race condition here (toctou error).
I assumed that write_value() should handle such a situation gracefully.

Do I miss something important here (I'm both Rust and BLE noob)?

@codepainters
Copy link
Contributor Author

It looks like this simple change indeed solved the problem for me:

diff --git a/src/client/ble_writer.rs b/src/client/ble_writer.rs
index 100b61b..5703129 100644
--- a/src/client/ble_writer.rs
+++ b/src/client/ble_writer.rs
@@ -17,7 +17,12 @@ impl BLEWriter {
 
   pub async fn write_value(&mut self, data: &[u8], response: bool) -> Result<(), BLEReturnCode> {
     unsafe {
-      let mtu = { esp_idf_sys::ble_att_mtu(self.conn_handle) - 3 } as usize;
+      // ble_att_mtu() returns 0 for a closed connection
+      let mtu = esp_idf_sys::ble_att_mtu(self.conn_handle);
+      if mtu == 0 {
+        return Err(BLEReturnCode(esp_idf_sys::BLE_HS_ENOTCONN));
+      }
+      let mtu = { mtu - 3 } as usize;
 
       if !response && data.len() <= mtu {
         return ble!(esp_idf_sys::ble_gattc_write_no_rsp_flat(

Shall I open a PR?

@taks
Copy link
Owner

taks commented Feb 18, 2024

Yes, please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants