taks / esp32-nimble

A wrapper for the ESP32 NimBLE Bluetooth stack.
Apache License 2.0
118 stars 35 forks source link

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

Closed codepainters closed 8 months ago

codepainters commented 9 months ago

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

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 commented 9 months ago

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 commented 9 months ago

Yes, please.