Prevent stalling of Linux central BTLE implementation

Some peripheral implementations do not respond with a response upon
reception of GATT requests. Since the Linux implementation does not
progress until a response is received, it stalls forever.

A new timeout was introduced to counter this. If the response is not
received within the timeout period an artificial GATT error response is
injected into the queue. In addition, a very large warning is printed
to highlight the fact and force the user to deal with it. In extreme
cases this could create strange ordering problems for extremely
delayed responses. Hence the implementation continues under
reservation.

A disconnect as response to the missing response from the peripheral
was briefly considered too. However user reports indicate that not every
user is able to change the peripheral implementation. This would block
further usage of QtBluetooth (especially if one characteristic is
non-conformant but the other characteristics of the same service are
OK).

Task-number: QTBUG-52692
Change-Id: I49ad7b75215101b3132ba97794e71021ee25a30e
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>

2 files changed, 141 insertions, 3 deletions

diff --git a/src/bluetooth/qlowenergycontroller_bluez.cpp b/src/bluetooth/qlowenergycontroller_bluez.cpp
index e85fcb34..03580ff6 100644
--- a/src/bluetooth/qlowenergycontroller_bluez.cpp
+++ b/src/bluetooth/qlowenergycontroller_bluez.cpp
@@ -48,6 +48,7 @@
 #include <QtCore/QFileInfo>
 #include <QtCore/QLoggingCategory>
 #include <QtCore/QSettings>
+#include <QtCore/QTimer>
 #include <QtBluetooth/QBluetoothLocalDevice>
 #include <QtBluetooth/QBluetoothSocket>
 #include <QtBluetooth/QLowEnergyCharacteristicData>
@@ -132,6 +133,13 @@
 #define ATT_ERROR_UNSUPPRTED_GROUP_TYPE 0x10
 #define ATT_ERROR_INSUF_RESOURCES       0x11
 #define ATT_ERROR_APPLICATION_START     0x80
+//------------------------------------------
+// The error codes in this block are
+// implementation specific errors
+
+#define ATT_ERROR_REQUEST_STALLED       0x81
+
+//------------------------------------------
 #define ATT_ERROR_APPLICATION_END       0x9f
 
 #define APPEND_VALUE true
@@ -211,7 +219,7 @@ static void dumpErrorInformation(const QByteArray &response)
         errorString = QStringLiteral("insufficient resources to complete request"); break;
     default:
         if (errorCode >= ATT_ERROR_APPLICATION_START && errorCode <= ATT_ERROR_APPLICATION_END)
-            errorString = QStringLiteral("application error");
+            errorString = QStringLiteral("application error: %1").arg(errorCode);
         else
             errorString = QStringLiteral("unknown error code");
         break;
@@ -304,6 +312,100 @@ void QLowEnergyControllerPrivate::init()
                 signingData.insert(remoteDevice.toUInt64(), SigningData(csrk));
         }
     );
+
+    if (role == QLowEnergyController::CentralRole) {
+        if (Q_UNLIKELY(!qEnvironmentVariableIsEmpty("BLUETOOTH_GATT_TIMEOUT"))) {
+            bool ok = false;
+            int value = qEnvironmentVariableIntValue("BLUETOOTH_GATT_TIMEOUT", &ok);
+            if (ok)
+                gattRequestTimeout = value;
+        }
+
+        // permit disabling of timeout behavior via environment variable
+        if (gattRequestTimeout > 0) {
+            qCWarning(QT_BT_BLUEZ) << "Enabling GATT request timeout behavior" << gattRequestTimeout;
+            requestTimer = new QTimer(this);
+            requestTimer->setSingleShot(true);
+            requestTimer->setInterval(gattRequestTimeout);
+            connect(requestTimer, &QTimer::timeout,
+                    this, &QLowEnergyControllerPrivate::handleGattRequestTimeout);
+        }
+    }
+}
+
+void QLowEnergyControllerPrivate::handleGattRequestTimeout()
+{
+    // antyhing open that might require cancellation or a warning?
+    if (encryptionChangePending) {
+        // We cannot really recover for now but the warning is essential for debugging
+        qCWarning(QT_BT_BLUEZ) << "****** Encryption change event blocking further GATT requests";
+        return;
+    }
+
+    if (!openRequests.isEmpty() && requestPending) {
+        requestPending = false; // reset pending flag
+        const Request currentRequest = openRequests.dequeue();
+
+        qCWarning(QT_BT_BLUEZ).nospace() << "****** Request type 0x" << hex << currentRequest.command
+                           << " to server/peripheral timed out";
+        qCWarning(QT_BT_BLUEZ) << "****** Looks like the characteristic or descriptor does NOT act in"
+                               <<  "accordance to Bluetooth 4.x spec.";
+        qCWarning(QT_BT_BLUEZ) << "****** Please check server implementation."
+                               << "Continuing under reservation.";
+
+        quint8 command = currentRequest.command;
+        const auto createRequestErrorMessage = [](quint8 opcodeWithError,
+                                           QLowEnergyHandle handle) {
+            QByteArray errorPackage(ERROR_RESPONSE_HEADER_SIZE, Qt::Uninitialized);
+            errorPackage[0] = ATT_OP_ERROR_RESPONSE;
+            errorPackage[1] = opcodeWithError; // e.g. ATT_OP_READ_REQUEST
+            putBtData(handle, errorPackage.data() + 2); //
+            errorPackage[4] = ATT_ERROR_REQUEST_STALLED;
+
+            return errorPackage;
+        };
+
+        switch (command) {
+        case ATT_OP_EXCHANGE_MTU_REQUEST:  // MTU change request
+            // never received reply to MTU request
+            // it is safe to skip and go to next request
+            sendNextPendingRequest();
+            break;
+        case ATT_OP_READ_BY_GROUP_REQUEST: // primary or secondary service discovery
+        case ATT_OP_READ_BY_TYPE_REQUEST:  // characteristic or included service discovery
+            // jump back into usual response handling with custom error code
+            // 2nd param "0" as required by spec
+            processReply(currentRequest, createRequestErrorMessage(command, 0));
+            break;
+        case ATT_OP_READ_REQUEST:          // read descriptor or characteristic value
+        case ATT_OP_READ_BLOB_REQUEST:     // read long descriptor or characteristic
+        case ATT_OP_WRITE_REQUEST:         // write descriptor or characteristic
+        {
+            uint handleData = currentRequest.reference.toUInt();
+            const QLowEnergyHandle charHandle = (handleData & 0xffff);
+            const QLowEnergyHandle descriptorHandle = ((handleData >> 16) & 0xffff);
+            processReply(currentRequest, createRequestErrorMessage(command,
+                                descriptorHandle ? descriptorHandle : charHandle));
+        }
+            break;
+        case ATT_OP_FIND_INFORMATION_REQUEST: // get descriptor information
+            processReply(currentRequest, createRequestErrorMessage(
+                                            command, currentRequest.reference2.toUInt()));
+            break;
+        case ATT_OP_PREPARE_WRITE_REQUEST: // prepare to write long desc or char
+        case ATT_OP_EXECUTE_WRITE_REQUEST: // execute long write of desc or char
+        {
+            uint handleData = currentRequest.reference.toUInt();
+            const QLowEnergyHandle attrHandle = (handleData & 0xffff);
+            processReply(currentRequest,
+                         createRequestErrorMessage(command, attrHandle));
+        }
+            break;
+        default:
+            // not a command used by central role implementation
+            return;
+        }
+    }
 }
 
 QLowEnergyControllerPrivate::~QLowEnergyControllerPrivate()
@@ -539,6 +641,15 @@ void QLowEnergyControllerPrivate::resetController()
     connectionHandle = 0;
 }
 
+void QLowEnergyControllerPrivate::restartRequestTimer()
+{
+    if (!requestTimer)
+        return;
+
+    if (gattRequestTimeout > 0)
+        requestTimer->start(gattRequestTimeout);
+}
+
 void QLowEnergyControllerPrivate::l2cpReadyRead()
 {
     const QByteArray incomingPacket = l2cpSocket->readAll();
@@ -564,7 +675,8 @@ void QLowEnergyControllerPrivate::l2cpReadyRead()
         processUnsolicitedReply(incomingPacket);
         return;
     }
-
+    //--------------------------------------------------
+    // Peripheral side packet handling
     case ATT_OP_EXCHANGE_MTU_REQUEST:
         handleExchangeMtuRequest(incomingPacket);
         return;
@@ -608,6 +720,7 @@ void QLowEnergyControllerPrivate::l2cpReadyRead()
             qCWarning(QT_BT_BLUEZ) << "received unexpected handle value confirmation";
         }
         return;
+    //--------------------------------------------------
     default:
         //only solicited replies finish pending requests
         requestPending = false;
@@ -713,6 +826,7 @@ void QLowEnergyControllerPrivate::sendNextPendingRequest()
 //             << request.payload.toHex();
 
     requestPending = true;
+    restartRequestTimer();
     sendPacket(request.payload);
 }
 
@@ -1920,8 +2034,10 @@ bool QLowEnergyControllerPrivate::increaseEncryptLevelfRequired(quint8 errorCode
             return false;
         if (securityLevelValue != BT_SECURITY_HIGH) {
             qCDebug(QT_BT_BLUEZ) << "Requesting encrypted link";
-            if (setSecurityLevel(BT_SECURITY_HIGH))
+            if (setSecurityLevel(BT_SECURITY_HIGH)) {
+                restartRequestTimer();
                 return true;
+            }
         }
         break;
     default:
diff --git a/src/bluetooth/qlowenergycontroller_p.h b/src/bluetooth/qlowenergycontroller_p.h
index dcbdcdb9..19d10567 100644
--- a/src/bluetooth/qlowenergycontroller_p.h
+++ b/src/bluetooth/qlowenergycontroller_p.h
@@ -94,6 +94,7 @@ class QWinRTLowEnergyServiceHandler;
 QT_BEGIN_NAMESPACE
 
 class QLowEnergyServiceData;
+class QTimer;
 
 #if defined(QT_BLUEZ_BLUETOOTH) && !defined(QT_BLUEZ_NO_BTLE)
 class HciManager;
@@ -276,6 +277,24 @@ private:
     HciManager *hciManager;
     QLeAdvertiser *advertiser;
     QSocketNotifier *serverSocketNotifier;
+    QTimer *requestTimer = nullptr;
+
+    /*
+      Defines the maximum number of milliseconds the implementation will
+      wait for requests that require a response.
+
+      This addresses the problem that some non-conformant BTLE devices
+      do not implement the request/response system properly. In such cases
+      the queue system would hang forever.
+
+      Once timeout has been triggered we gracefully continue with the next request.
+      Depending on the type of the timed out ATT command we either ignore it
+      or artifically trigger an error response to ensure the API gives the
+      appropriate response. Potentially this can cause problems when the
+      response for the dropped requests arrives very late. That's why a big warning
+      is printed about the compromised state when a timeout is triggered.
+     */
+    int gattRequestTimeout = 20000;
 
     void handleConnectionRequest();
     void closeServerSocket();
@@ -392,12 +411,15 @@ private:
             const QLowEnergyHandle descriptorHandle,
             const QByteArray &newValue);
 
+    void restartRequestTimer();
+
 private slots:
     void l2cpConnected();
     void l2cpDisconnected();
     void l2cpErrorChanged(QBluetoothSocket::SocketError);
     void l2cpReadyRead();
     void encryptionChangedEvent(const QBluetoothAddress&, bool);
+    void handleGattRequestTimeout();
 #elif defined(QT_ANDROID_BLUETOOTH)
     LowEnergyNotificationHub *hub;