fix(l2cap): propagate send failures without replaying partial SDUs

.github/workflows/l2cap-tests.yml

@@ -0,0 +1,22 @@
+name: L2CAP native tests
+on:
+  workflow_dispatch:
+  pull_request:
+  push:
+    branches: [master]
+permissions:
+  contents: read
+
+jobs:
+  native:
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        cxx_flags: ["", "-fsanitize=address,undefined -fno-omit-frame-pointer"]
+    steps:
+      - uses: actions/checkout@v5
+        with:
+          persist-credentials: false
+      - run: cmake -S tests/l2cap -B build/l2cap-tests -DCMAKE_CXX_FLAGS="${{ matrix.cxx_flags }}"
+      - run: cmake --build build/l2cap-tests
+      - run: ctest --test-dir build/l2cap-tests --output-on-failure

src/NimBLEL2CAPChannel.cpp

@@ -8,6 +8,7 @@
 # include "NimBLEClient.h"
 # include "NimBLELog.h"
 # include "NimBLEUtils.h"
+# include <algorithm>
 
 # ifdef USING_NIMBLE_ARDUINO_HEADERS
 #  include "nimble/nimble/host/include/host/ble_gap.h"
@@ -21,9 +22,6 @@
 // Round-up integer division
 # define CEIL_DIVIDE(a, b)               (((a) + (b) - 1) / (b))
 # define ROUND_DIVIDE(a, b)              (((a) + (b) / 2) / (b))
-// Retry
-constexpr uint32_t RetryTimeout = 50;
-constexpr int      RetryCounter = 3;
 
 NimBLEL2CAPChannel::NimBLEL2CAPChannel(uint16_t psm, uint16_t mtu, NimBLEL2CAPChannelCallbacks* callbacks)
     : psm(psm), mtu(mtu), callbacks(callbacks) {
@@ -105,54 +103,30 @@ int NimBLEL2CAPChannel::writeFragment(std::vector<uint8_t>::const_iterator begin
         return -BLE_HS_EBADDATA;
     }
 
-    auto retries = RetryCounter;
-
-    while (retries--) {
-        auto txd = os_mbuf_get_pkthdr(&_coc_mbuf_pool, 0);
-        if (!txd) {
-            NIMBLE_LOGE(LOG_TAG, "Can't os_mbuf_get_pkthdr.");
-            return -BLE_HS_ENOMEM;
-        }
-        auto append = os_mbuf_append(txd, &(*begin), toSend);
-        if (append != 0) {
-            NIMBLE_LOGE(LOG_TAG, "Can't os_mbuf_append: %d", append);
-            return append;
-        }
+    auto* txd = os_mbuf_get_pkthdr(&_coc_mbuf_pool, 0);
+    if (!txd) {
+        return BLE_HS_ENOMEM;
+    }
+    const int append = os_mbuf_append(txd, &*begin, toSend);
+    if (append != 0) {
+        os_mbuf_free_chain(txd);
+        return append;
+    }
 
-        auto res = ble_l2cap_send(channel, txd);
-        switch (res) {
-            case 0:
-                NIMBLE_LOGD(LOG_TAG, "L2CAP COC 0x%04X sent %d bytes.", this->psm, toSend);
-                return 0;
-
-            case BLE_HS_ESTALLED:
-                stalled = true;
-                NIMBLE_LOGD(LOG_TAG, "L2CAP COC 0x%04X sent %d bytes.", this->psm, toSend);
-                NIMBLE_LOGW(LOG_TAG,
-                            "ble_l2cap_send returned BLE_HS_ESTALLED. Next send will wait for unstalled event...");
-                return 0;
-
-            case BLE_HS_ENOMEM:
-            case BLE_HS_EAGAIN:
-                /* ble_l2cap_send already consumed and freed txd on these errors */
-                NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (consumed buffer). Retrying shortly...", res);
-                ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout));
-                continue;
-
-            case BLE_HS_EBUSY:
-                /* Channel busy; txd not consumed */
-                NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (busy). Retrying shortly...", res);
-                os_mbuf_free_chain(txd);
-                ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout));
-                continue;
-
-            default:
-                NIMBLE_LOGE(LOG_TAG, "ble_l2cap_send failed: %d", res);
-                return res;
-        }
+    const int rc = ble_l2cap_send(channel, txd);
+    if (rc == BLE_HS_ESTALLED) {
+        // NimBLE owns the SDU and will finish it when credits arrive.
+        stalled = true;
+        return 0;
+    }
+    // These early validation/busy errors retain caller ownership. All other
+    // native send results consume the SDU, including ENOMEM and EAGAIN.
+    if (rc == BLE_HS_EBADDATA || rc == BLE_HS_EBUSY) {
+        os_mbuf_free_chain(txd);
     }
-    NIMBLE_LOGE(LOG_TAG, "Retries exhausted, dropping %d bytes to send.", toSend);
-    return -BLE_HS_EREJECT;
+    // A native error can follow partial transmission of this SDU. Resubmitting
+    // it could duplicate bytes; let write() abort the incomplete stream.
+    return rc;
 }
 
 # if MYNEWT_VAL(BLE_ROLE_CENTRAL)
@@ -194,8 +168,9 @@ bool NimBLEL2CAPChannel::write(const std::vector<uint8_t>& bytes) {
 
     auto start = bytes.begin();
     while (start != bytes.end()) {
-        auto end = start + mtu < bytes.end() ? start + mtu : bytes.end();
-        if (writeFragment(start, end) < 0) {
+        auto end = start + std::min<size_t>(mtu, bytes.end() - start);
+        if (writeFragment(start, end) != 0) {
+            disconnect();
             return false;
         }
         start = end;

tests/l2cap/CMakeLists.txt

@@ -0,0 +1,10 @@
+cmake_minimum_required(VERSION 3.20)
+project(L2CAPTests LANGUAGES CXX)
+enable_testing()
+add_executable(l2cap_send_tests SendErrorsTests.cpp ../../src/NimBLEL2CAPChannel.cpp)
+target_compile_features(l2cap_send_tests PRIVATE cxx_std_17)
+target_include_directories(l2cap_send_tests PRIVATE stubs ../../src)
+target_compile_options(l2cap_send_tests PRIVATE -UNDEBUG -include ${CMAKE_CURRENT_SOURCE_DIR}/stubs/Host.h)
+foreach(case errors partial ownership append success stalled)
+  add_test(NAME l2cap_send_${case} COMMAND l2cap_send_tests ${case})
+endforeach()

tests/l2cap/README.md

@@ -0,0 +1,13 @@
+# L2CAP send regression tests
+
+The tests compile the production `NimBLEL2CAPChannel.cpp` with minimal native
+host replacements. They check positive native error propagation, abort after a
+partly submitted SDU without replay, caller/native mbuf ownership, append
+failure cleanup, successful fragmentation and accepted credit-stalled sends.
+They do not simulate RF, controller buffers or concurrency.
+
+```sh
+cmake -S tests/l2cap -B build/l2cap-tests
+cmake --build build/l2cap-tests
+ctest --test-dir build/l2cap-tests --output-on-failure
+```

tests/l2cap/SendErrorsTests.cpp

@@ -0,0 +1,60 @@
+#include "NimBLEL2CAPChannel.h"
+#include <iostream>
+#include <stdexcept>
+
+static void require(bool value,const char* message) { if (!value) throw std::runtime_error(message); }
+struct Channel : NimBLEL2CAPChannel {
+    ble_l2cap_chan native;
+    Channel() : NimBLEL2CAPChannel(129,64,new NimBLEL2CAPChannelCallbacks) {
+        ble_l2cap_event e{};e.connect.chan=&native;handleConnectionEvent(&e);
+    }
+};
+int main(int argc,char** argv) {
+    try {
+        const std::string test=argc>1?argv[1]:"errors";
+        int sends=0;
+        if (test=="errors" || test=="ownership") {
+            const std::vector<int> codes=test=="errors" ?
+                std::vector<int>{BLE_HS_ENOMEM,BLE_HS_EAGAIN,BLE_HS_ENOTCONN,BLE_HS_EINVAL} :
+                std::vector<int>{BLE_HS_EBADDATA,BLE_HS_EBUSY};
+            for (int error:codes) {
+                Channel c;sends=0;hostDisconnects=0;
+                hostSend=[&](os_mbuf* b) {
+                    ++sends;
+                    if (error!=BLE_HS_EBADDATA && error!=BLE_HS_EBUSY) os_mbuf_free_chain(b);
+                    return error;
+                };
+                require(!c.write(std::vector<uint8_t>(20,0x55)),"positive error reported success");
+                require(sends==1,"failed SDU was retried or later fragment submitted");
+                require(hostDisconnects==1,"failed stream was not disconnected");
+                require(liveMbufs==0,"send buffer leaked or freed twice");
+            }
+        } else if (test=="partial") {
+            Channel c;std::vector<uint8_t> delivered;
+            hostSend=[&](os_mbuf* b) {
+                ++sends;
+                const size_t length=sends==1?b->bytes.size():2;
+                delivered.insert(delivered.end(),b->bytes.begin(),b->bytes.begin()+length);
+                os_mbuf_free_chain(b);return sends==1?0:BLE_HS_ENOMEM;
+            };
+            require(!c.write(std::vector<uint8_t>(24,0x55)),"partially sent stream reported success");
+            require(sends==2 && delivered.size()==10,"partial SDU prefix replayed or remaining fragment sent");
+            require(hostDisconnects==1 && liveMbufs==0,"partial-send abort/ownership failed");
+        } else if (test=="append") {
+            Channel c;appendError=BLE_HS_ENOMEM;
+            hostSend=[&](os_mbuf*) { ++sends;return 0; };
+            require(!c.write({1,2,3}),"append failure reported success");
+            require(sends==0 && liveMbufs==0 && hostDisconnects==1,"append error leaked buffer or submitted data");
+        } else if (test=="success") {
+            Channel c;std::vector<uint8_t> received;std::vector<uint8_t> input;
+            for (int i=0;i<20;++i) input.push_back(uint8_t(i));
+            hostSend=[&](os_mbuf* b) {++sends;received.insert(received.end(),b->bytes.begin(),b->bytes.end());return os_mbuf_free_chain(b);};
+            require(c.write(input) && sends==3 && received==input && liveMbufs==0 && hostDisconnects==0,"valid fragmented write failed");
+        } else if (test=="stalled") {
+            Channel c;
+            hostSend=[&](os_mbuf* b) {++sends;os_mbuf_free_chain(b);return BLE_HS_ESTALLED;};
+            require(c.write({1,2,3}) && sends==1 && liveMbufs==0 && hostDisconnects==0,"accepted stalled SDU incorrectly treated as error");
+        } else { throw std::runtime_error("unknown test"); }
+        std::cout<<test<<" passed\n";return 0;
+    } catch (const std::exception& e) {std::cerr<<e.what()<<'\n';return 1;}
+}

tests/l2cap/stubs/Host.h

@@ -0,0 +1,68 @@
+#pragma once
+// Minimal host replacements; tests compile the production channel source.
+#define NIMBLE_CPP_CLIENT_H_
+#define NIMBLE_CPP_LOG_H_
+#define NIMBLE_CPP_UTILS_H_
+#define NIMBLE_LOGI(...) ((void)0)
+#define NIMBLE_LOGD(...) ((void)0)
+#define NIMBLE_LOGW(...) ((void)0)
+#define NIMBLE_LOGE(...) ((void)0)
+#define CONFIG_BT_NIMBLE_ENABLED 1
+#define MYNEWT_VAL(x) MYNEWT_VAL_##x
+#define MYNEWT_VAL_BLE_L2CAP_COC_MAX_NUM 1
+#define MYNEWT_VAL_BLE_ROLE_CENTRAL 0
+#include <atomic>
+#include <cassert>
+#include <cerrno>
+#include <cstdint>
+#include <cstdlib>
+#include <cstring>
+#include <functional>
+#include <vector>
+struct os_mempool {};
+struct os_mbuf_pool {};
+using os_membuf_t = uint8_t;
+struct os_mbuf { std::vector<uint8_t> bytes; };
+#define OS_MEMPOOL_SIZE(n,s) ((n)*(s))
+#define OS_MBUF_PKTLEN(m) ((m)->bytes.size())
+inline int liveMbufs=0, appendError=0;
+inline int os_mempool_init(os_mempool*,size_t,size_t,void*,const char*) { return 0; }
+inline int os_mbuf_pool_init(os_mbuf_pool*,os_mempool*,size_t,size_t) { return 0; }
+inline os_mbuf* os_mbuf_get_pkthdr(os_mbuf_pool*,int) { ++liveMbufs; return new os_mbuf; }
+inline int os_mbuf_append(os_mbuf* b,const void* p,size_t n) {
+    if (appendError) return appendError;
+    auto a=static_cast<const uint8_t*>(p); b->bytes.insert(b->bytes.end(),a,a+n); return 0;
+}
+inline int os_mbuf_free_chain(os_mbuf* b) { --liveMbufs; delete b; return 0; }
+inline int os_mbuf_copydata(os_mbuf* b,size_t off,size_t n,void* p) { memcpy(p,b->bytes.data()+off,n); return 0; }
+constexpr int BLE_HS_EAGAIN=1, BLE_HS_ENOMEM=6, BLE_HS_ENOTCONN=7, BLE_HS_EINVAL=3,
+ BLE_HS_EALREADY=2, BLE_HS_EBADDATA=10, BLE_HS_EBUSY=15, BLE_HS_EREJECT=14,
+ BLE_HS_ESTALLED=31, BLE_HS_EUNKNOWN=17;
+constexpr uint16_t BLE_HS_CONN_HANDLE_NONE=0xffff;
+constexpr uint32_t BLE_NPL_TIME_FOREVER=UINT32_MAX;
+inline uint32_t ble_npl_time_ms_to_ticks32(uint32_t ms) { return ms; }
+inline void ble_npl_time_delay(uint32_t) {} // Retry count, not elapsed radio time, is tested.
+struct ble_l2cap_chan {};
+struct ble_l2cap_chan_info { uint16_t peer_coc_mtu=8,our_coc_mtu=8,peer_l2cap_mtu=8,our_l2cap_mtu=8; };
+constexpr int BLE_L2CAP_EVENT_COC_CONNECTED=1,BLE_L2CAP_EVENT_COC_DISCONNECTED=2,
+ BLE_L2CAP_EVENT_COC_ACCEPT=3,BLE_L2CAP_EVENT_COC_DATA_RECEIVED=4,BLE_L2CAP_EVENT_COC_TX_UNSTALLED=5;
+struct ble_l2cap_event {
+ int type=0;
+ struct { ble_l2cap_chan* chan; } connect{},accept{},disconnect{};
+ struct { os_mbuf* sdu_rx; } receive{};
+ struct { int status; ble_l2cap_chan* chan; } tx_unstalled{};
+};
+inline std::function<int(os_mbuf*)> hostSend;
+inline int hostDisconnects=0;
+inline int ble_l2cap_get_chan_info(ble_l2cap_chan*,ble_l2cap_chan_info* i) { *i={}; return 0; }
+inline uint16_t ble_l2cap_get_conn_handle(ble_l2cap_chan*) { return 1; }
+inline int ble_l2cap_send(ble_l2cap_chan*,os_mbuf* b) { return hostSend(b); }
+inline int ble_l2cap_disconnect(ble_l2cap_chan*) { ++hostDisconnects; return 0; }
+inline int ble_l2cap_recv_ready(ble_l2cap_chan*,os_mbuf* b) { return os_mbuf_free_chain(b); }
+class NimBLEUtils {
+ public:
+ struct TaskData { mutable int m_flags=0; };
+ static bool taskWait(const TaskData&,uint32_t) { assert(false && "credit wait not exercised by send error tests"); return false; }
+ static void taskRelease(const TaskData&,int=0) {}
+ static const char* returnCodeToString(int) { return "stub"; }
+};

tests/l2cap/stubs/host/ble_gap.h

@@ -0,0 +1 @@
+#include "Host.h"

tests/l2cap/stubs/host/ble_l2cap.h

@@ -0,0 +1 @@
+#include "Host.h"

tests/l2cap/stubs/os/os_mbuf.h

@@ -0,0 +1 @@
+#include "Host.h"

tests/l2cap/stubs/syscfg/syscfg.h

@@ -0,0 +1 @@
+#include "Host.h"