Fix a race condition between a write and an incoming change

The Android API that is available to write to a characteristic requires
the user to first set the new value of the characteristic and then tell
Android to write the value to the device. If the phone processes an
incoming change for this same characteristic in between the setting of
the value and the actual write, it could happen that the characteristic
gets the incoming value and that incoming value is then sent to the
connected device. Something similar could occur for the incoming change
as the received value is not passed in the callback but it is set on the
characteristic and the user is expected to read the received value from
the characteristic.
To prevent these problems, a new connectGatt function that takes a
Handler as extra argument was added to SDK v26. The callbacks are then
always executed via this handler and thus on the associated handler
thread. If the user also performs all writes on this thread, the
described race condition can no longer occur.
This could have solved the problems, but Android 8.0 contained a race
condition in the callback part because it set the value of the
characteristic before passing the callback to the handler, still
allowing the same race condition for incoming changed. This was fixed in
Android 8.1.

This commit causes the code to use a Handler and the new connectGatt
function if it is running on SDK >= 27, which is associated with Android
8.1.
For older SDK versions, a different solution is implemented. In this
case, a temporary BluetoothGattCharacteristic is instantiated that can
hold the value to be written. This way, the write can no longer
interfere with changes that are being received. To instantiate the
BluetoothGattCharacteristic, a private constructor is invoked using
reflection. This private constructor is needed as only this constructor
allows to create an instance that has the required information for
Android to be able to write to the charatceristic (such as the
associated service and instanceId). This is also the reason why this
solution cannot be used on newer SDK's as this constructor is
blacklisted and a warning box will be shown when reflection is used to
get a reference to it. Because a temporary BluetoothGattCharacteristic
is instantiated that holds the written value, it is necessary to store
the value that was written and pass that in the callback.

More information on these Android issues and solution direction can be
found here:
https://stackoverflow.com/questions/38922639/how-could-i-achieve-maximum-thread-safety-with-a-read-write-ble-gatt-characteris
https://medium.com/@martijn.van.welie/making-android-ble-work-part-3-117d3a8aee23

Change-Id: I5a9eda501a20933e37f758a3114bf71ec3e7cfd4
Reviewed-by: Thiemo van Engelen <tvanengelen@victronenergy.com>
Reviewed-by: Konstantin Ritt <ritt.ks@gmail.com>
Reviewed-by: Oliver Wolff <oliver.wolff@qt.io>

1 files changed, 142 insertions, 36 deletions

diff --git a/src/android/bluetooth/src/org/qtproject/qt5/android/bluetooth/QtBluetoothLE.java b/src/android/bluetooth/src/org/qtproject/qt5/android/bluetooth/QtBluetoothLE.java
index f70ee8a4..3e602186 100644
--- a/src/android/bluetooth/src/org/qtproject/qt5/android/bluetooth/QtBluetoothLE.java
+++ b/src/android/bluetooth/src/org/qtproject/qt5/android/bluetooth/QtBluetoothLE.java
@@ -55,11 +55,12 @@ import android.bluetooth.le.ScanSettings;
 import android.content.Context;
 import android.os.Build;
 import android.os.Handler;
+import android.os.HandlerThread;
 import android.os.Looper;
 import android.util.Log;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.lang.reflect.Method;
 
 import java.util.ArrayList;
 import java.util.Hashtable;
@@ -75,6 +76,9 @@ public class QtBluetoothLE {
     private boolean mLeScanRunning = false;
 
     private BluetoothGatt mBluetoothGatt = null;
+    private HandlerThread mHandlerThread = null;
+    private Handler mHandler = null;
+    private Constructor mCharacteristicConstructor = null;
     private String mRemoteGattAddress;
     private final UUID clientCharacteristicUuid = UUID.fromString("00002902-0000-1000-8000-00805f9b34fb");
     private final int MAX_MTU = 512;
@@ -219,8 +223,13 @@ public class QtBluetoothLE {
                     resetData();
                     // reset mBluetoothGatt, reusing same object is not very reliable
                     // sometimes it reconnects and sometimes it does not.
-                    if (mBluetoothGatt != null)
+                    if (mBluetoothGatt != null) {
                         mBluetoothGatt.close();
+                        if (mHandler != null) {
+                            mHandler.getLooper().quitSafely();
+                            mHandler = null;
+                        }
+                    }
                     mBluetoothGatt = null;
                     break;
                 case BluetoothProfile.STATE_CONNECTED:
@@ -377,10 +386,11 @@ public class QtBluetoothLE {
                     errorCode = 2; break; // CharacteristicWriteError
             }
 
+            byte[] value = pendingJob.newValue;
             synchronized (readWriteQueue) {
                 ioJobPending = false;
             }
-            leCharacteristicWritten(qtObject, handle+1, characteristic.getValue(), errorCode);
+            leCharacteristicWritten(qtObject, handle+1, value, errorCode);
             performNextIO();
         }
 
@@ -572,29 +582,86 @@ public class QtBluetoothLE {
             return false;
         }
 
-        try {
-            // BluetoothDevice.connectGatt(Context, boolean, BluetoothGattCallback, int) was
-            // officially introduced by Android API v23. Earlier Android versions have a private
-            // implementation already though. Let's check at runtime and use it if possible.
-            //
-            // In general the new connectGatt() seems to be much more reliable than the function
-            // that doesn't specify the transport layer.
-
-            Class[] args = new Class[4];
+        /* The required connectGatt function is already available in SDK v26, but Android 8.0
+         * contains a race condition in the Changed callback such that it can return the value that
+         * was written. This is fixed in Android 8.1, which matches SDK v27. */
+        if (Build.VERSION.SDK_INT >= 27) {
+            HandlerThread handlerThread = new HandlerThread("QtBluetoothLEHandlerThread");
+            handlerThread.start();
+            mHandler = new Handler(handlerThread.getLooper());
+
+            Class[] args = new Class[6];
             args[0] = android.content.Context.class;
             args[1] = boolean.class;
             args[2] = android.bluetooth.BluetoothGattCallback.class;
             args[3] = int.class;
-            Method connectMethod = mRemoteGattDevice.getClass().getDeclaredMethod("connectGatt", args);
-            if (connectMethod != null) {
-                mBluetoothGatt = (BluetoothGatt) connectMethod.invoke(mRemoteGattDevice, qtContext,
-                                                                      false, gattCallback,
-                                                                      2 /*TRANSPORT_LE*/);
-                Log.w(TAG, "Using Android v23 BluetoothDevice.connectGatt()");
+            args[4] = int.class;
+            args[5] = android.os.Handler.class;
+
+            try {
+                Method connectMethod = mRemoteGattDevice.getClass().getDeclaredMethod("connectGatt", args);
+                if (connectMethod != null) {
+                    mBluetoothGatt = (BluetoothGatt) connectMethod.invoke(mRemoteGattDevice, qtContext, false,
+                            gattCallback, 2 /* TRANSPORT_LE */, 1 /*BluetoothDevice.PHY_LE_1M*/, mHandler);
+                    Log.w(TAG, "Using Android v26 BluetoothDevice.connectGatt()");
+                }
+            } catch (Exception ex) {
+                Log.w(TAG, "connectGatt() v26 not available");
+                ex.printStackTrace();
+            }
+
+            if (mBluetoothGatt == null) {
+                mHandler.getLooper().quitSafely();
+                mHandler = null;
+            }
+        }
+
+        if (mBluetoothGatt == null) {
+            try {
+                //This API element is currently: greylist-max-o, reflection, allowed
+                //It may change in the future
+                Class[] constr_args = new Class[5];
+                constr_args[0] = android.bluetooth.BluetoothGattService.class;
+                constr_args[1] = java.util.UUID.class;
+                constr_args[2] = int.class;
+                constr_args[3] = int.class;
+                constr_args[4] = int.class;
+                mCharacteristicConstructor = BluetoothGattCharacteristic.class.getDeclaredConstructor(constr_args);
+                mCharacteristicConstructor.setAccessible(true);
+            } catch (NoSuchMethodException ex) {
+                Log.w(TAG, "Unable get characteristic constructor. Buffer race condition are possible");
+                /*  For some reason we don't get the private BluetoothGattCharacteristic ctor.
+                    This means that we cannot protect ourselves from issues where concurrent
+                    read and write operations on the same char can overwrite each others buffer.
+                    Nevertheless we continue with best effort.
+                */
+            }
+
+            try {
+                // BluetoothDevice.connectGatt(Context, boolean, BluetoothGattCallback, int) was
+                // officially introduced by Android API v23. Earlier Android versions have a
+                // private
+                // implementation already though. Let's check at runtime and use it if possible.
+                //
+                // In general the new connectGatt() seems to be much more reliable than the
+                // function
+                // that doesn't specify the transport layer.
+
+                Class[] args = new Class[4];
+                args[0] = android.content.Context.class;
+                args[1] = boolean.class;
+                args[2] = android.bluetooth.BluetoothGattCallback.class;
+                args[3] = int.class;
+                Method connectMethod = mRemoteGattDevice.getClass().getDeclaredMethod("connectGatt", args);
+                if (connectMethod != null) {
+                    mBluetoothGatt = (BluetoothGatt) connectMethod.invoke(mRemoteGattDevice, qtContext, false,
+                            gattCallback, 2 /* TRANSPORT_LE */);
+                    Log.w(TAG, "Using Android v23 BluetoothDevice.connectGatt()");
+                }
+            } catch (Exception ex) {
+                // fallback to less reliable API 18 version
+                mBluetoothGatt = mRemoteGattDevice.connectGatt(qtContext, false, gattCallback);
             }
-        } catch (Exception ex) {
-            // fallback to less reliable API 18 version
-            mBluetoothGatt = mRemoteGattDevice.connectGatt(qtContext, false, gattCallback);
         }
 
         return mBluetoothGatt != null;
@@ -658,6 +725,7 @@ public class QtBluetoothLE {
 
     private final LinkedList<ReadWriteJob> readWriteQueue = new LinkedList<ReadWriteJob>();
     private boolean ioJobPending;
+    private ReadWriteJob pendingJob;
 
     /*
         Internal helper function
@@ -864,8 +932,7 @@ public class QtBluetoothLE {
 
             servicesToBeDiscovered.add(serviceHandle);
             scheduleServiceDetailDiscovery(serviceHandle);
-            performNextIO();
-
+            performNextIOThreaded();
         } catch (Exception ex) {
             ex.printStackTrace();
             return false;
@@ -949,8 +1016,11 @@ public class QtBluetoothLE {
         return true;
     }
 
-    private void scheduleMtuExchange()
-    {
+    /*
+     * Already executed in GattCallback so executed by the HandlerThread. No need to
+     * post it to the Hander.
+     */
+    private void scheduleMtuExchange() {
         ReadWriteJob newJob = new ReadWriteJob();
         newJob.jobType = IoJobType.Mtu;
         newJob.entry = null;
@@ -1063,7 +1133,7 @@ public class QtBluetoothLE {
             return false;
         }
 
-        performNextIO();
+        performNextIOThreaded();
         return true;
     }
 
@@ -1100,7 +1170,7 @@ public class QtBluetoothLE {
             return false;
         }
 
-        performNextIO();
+        performNextIOThreaded();
         return true;
     }
 
@@ -1135,7 +1205,7 @@ public class QtBluetoothLE {
             return false;
         }
 
-        performNextIO();
+        performNextIOThreaded();
         return true;
     }
 
@@ -1166,7 +1236,7 @@ public class QtBluetoothLE {
             return false;
         }
 
-        performNextIO();
+        performNextIOThreaded();
         return true;
     }
 
@@ -1180,7 +1250,7 @@ public class QtBluetoothLE {
             ioJobPending = false;
         }
 
-        performNextIO();
+        performNextIOThreaded();
 
         if (handle == HANDLE_FOR_MTU_EXCHANGE)
             return;
@@ -1206,6 +1276,24 @@ public class QtBluetoothLE {
     }
 
     /*
+        Wrapper around performNextIO() ensuring that performNextIO() is executed inside
+        the mHandler/mHandlerThread if it exists.
+    */
+    private void performNextIOThreaded()
+    {
+        if (mHandler != null) {
+            mHandler.post(new Runnable() {
+                @Override
+                public void run() {
+                    performNextIO();
+                }
+            });
+        } else {
+            performNextIO();
+        }
+    }
+
+    /*
        The queuing is required because two writeCharacteristic/writeDescriptor calls
        cannot execute at the same time. The second write must happen after the
        previous write has finished with on(Characteristic|Descriptor)Write().
@@ -1265,6 +1353,7 @@ public class QtBluetoothLE {
                 handleForTimeout.set(HANDLE_FOR_RESET); // not a pending call -> release atomic
             } else {
                 ioJobPending = true;
+                pendingJob = nextJob;
                 timeoutHandler.postDelayed(new TimeoutRunnable(
                         modifiedReadWriteHandle(handle, nextJob.jobType)), RUNNABLE_TIMEOUT);
             }
@@ -1353,6 +1442,16 @@ public class QtBluetoothLE {
         }
     }
 
+    private BluetoothGattCharacteristic cloneChararacteristic(BluetoothGattCharacteristic other) {
+        try {
+            return (BluetoothGattCharacteristic) mCharacteristicConstructor.newInstance(other.getService(),
+                    other.getUuid(), other.getInstanceId(), other.getProperties(), other.getPermissions());
+        } catch (Exception ex) {
+            Log.w(TAG, "Cloning characteristic failed!" + ex);
+            return null;
+        }
+    }
+
     // Runs inside the Mutex on readWriteQueue.
     // Returns true if nextJob should be skipped.
     private boolean executeWriteJob(ReadWriteJob nextJob)
@@ -1360,13 +1459,20 @@ public class QtBluetoothLE {
         boolean result;
         switch (nextJob.entry.type) {
             case Characteristic:
-                if (nextJob.entry.characteristic.getWriteType() != nextJob.requestedWriteType) {
-                    nextJob.entry.characteristic.setWriteType(nextJob.requestedWriteType);
+                if (mHandler != null || mCharacteristicConstructor == null) {
+                    if (nextJob.entry.characteristic.getWriteType() != nextJob.requestedWriteType) {
+                        nextJob.entry.characteristic.setWriteType(nextJob.requestedWriteType);
+                    }
+                    result = nextJob.entry.characteristic.setValue(nextJob.newValue);
+                    return !result || !mBluetoothGatt.writeCharacteristic(nextJob.entry.characteristic);
+                } else {
+                    BluetoothGattCharacteristic orig = nextJob.entry.characteristic;
+                    BluetoothGattCharacteristic tmp = cloneChararacteristic(orig);
+                    if (tmp == null)
+                        return true;
+                    tmp.setWriteType(nextJob.requestedWriteType);
+                    return !tmp.setValue(nextJob.newValue) || !mBluetoothGatt.writeCharacteristic(tmp);
                 }
-                result = nextJob.entry.characteristic.setValue(nextJob.newValue);
-                if (!result || !mBluetoothGatt.writeCharacteristic(nextJob.entry.characteristic))
-                    return true;
-                break;
             case Descriptor:
                 if (nextJob.entry.descriptor.getUuid().compareTo(clientCharacteristicUuid) == 0) {
                         /*