Closed Bug 1217778 Opened 4 years ago Closed 4 years ago

Add a null check of sBluetoothGattInterface before accessing it in Gatt notifications.

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Tracking Status
firefox45 --- fixed

People

(Reporter: yrliou, Assigned: lochang)

Details

Attachments

(1 file, 14 obsolete files)

12.43 KB, patch
Details | Diff | Splinter Review
Currently in notification functions in BluetoothGattManager.cpp, we access sBluetoothGattInterface directly without a null check.
We should check if it's null and reject the Promise in this case.
Hi Louis,

Would you like to try this bug as your first bug?

Regards,
Jocelyn
Flags: needinfo?(lochang)
Assignee: nobody → lochang
Flags: needinfo?(lochang)
Attachment #8680544 - Flags: review?(joliu)
Comment on attachment 8680544 [details] [diff] [review]
0001-Bug-1217778-sBluetoothGattInterface-null-check.patch

Review of attachment 8680544 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Louis,

Please fix the indents and trailing spaces in your patch first.

Thanks,
Jocelyn
Attachment #8680544 - Flags: review?(joliu)
(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #3)
> Comment on attachment 8680544 [details] [diff] [review]
> 0001-Bug-1217778-sBluetoothGattInterface-null-check.patch
> 
> Review of attachment 8680544 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Louis,
> 
> Please fix the indents and trailing spaces in your patch first.
> 
> Thanks,
> Jocelyn

Two more general suggestions:
1) Commit messages should be more descriptive.
2) Revise the patch description as same as your commit message when you uploading the patch.
Attachment #8681037 - Flags: review?(joliu)
(In reply to lochang from comment #5)
> Created attachment 8681037 [details] [diff] [review]
> sBluetoothGattInterface* null check

Fix the indents and trailing spaces in sBluetoothGattInterface.cpp
Fix again the indents and trailing spaces in the patch.
Attachment #8681073 - Flags: review?(joliu)
Hi Jocelyn,
   This patch add the sBluetoothGattInterface pointer checking also fix the indents and trailing spaces. Please review again, thank you.
Attachment #8680544 - Attachment is obsolete: true
Attachment #8681037 - Attachment is obsolete: true
Attachment #8681073 - Attachment is obsolete: true
Attachment #8681037 - Flags: review?(joliu)
Attachment #8681073 - Flags: review?(joliu)
Attachment #8681113 - Flags: review?(joliu)
Comment on attachment 8681113 [details] [diff] [review]
Bug 1217778 - Patch1: sBluetoothGattInterface* null check.

Review of attachment 8681113 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for my late reply, please see my comments below, thanks.

Again, please revise the commit message to be more descriptive.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +2643,5 @@
>      NS_LITERAL_STRING("ClientRegistered"),
>      uuid, BluetoothValue(uint32_t(aClientIf)));
>  
>    if (client->mStartLeScanRunnable) {
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mStartLeScanRunnable);

Suggest to move to right before calling |sBluetoothGattInterface->Scan()|.

@@ +2650,5 @@
>      sBluetoothGattInterface->Scan(
>        aClientIf, true /* start */,
>        new StartLeScanResultHandler(client));
>    } else if (client->mConnectRunnable) {
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mConnectRunnable);

Suggest to move to right before calling |sBluetoothGattInterface->Connect()|.

@@ +2870,5 @@
>    bs->DistributeSignal(NS_LITERAL_STRING("ServicesDiscovered"),
>                         client->mAppUuid,
>                         BluetoothValue(client->mServices));
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

Suggest to move to right before |sBluetoothGattInterface->GetIncludedService()|.
Revise as |ENSURE_GATT_INTF_IS_READY_VOID(client->mDiscoverRunnable)| to reply error.

@@ +2920,5 @@
>  
>    RefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
>    MOZ_ASSERT(client->mDiscoverRunnable);
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

Please reject the request using |ENSURE_GATT_INTF_IS_READY_VOID(client->mDiscoverRunnable)|.

@@ +2976,5 @@
>  
>    RefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
>    MOZ_ASSERT(client->mDiscoverRunnable);
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

DITTO.

@@ +3025,5 @@
>  
>    RefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
>    MOZ_ASSERT(client->mDiscoverRunnable);
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

DITTO.

@@ +3141,5 @@
>    MOZ_ASSERT(client->mReadCharacteristicState.mRunnable);
>    RefPtr<BluetoothReplyRunnable> runnable =
>      client->mReadCharacteristicState.mRunnable;
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

reject using mReadCharacteristicState.mRunnable.

@@ +3204,5 @@
>    MOZ_ASSERT(client->mWriteCharacteristicState.mRunnable);
>    RefPtr<BluetoothReplyRunnable> runnable =
>      client->mWriteCharacteristicState.mRunnable;
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

DITTO.

@@ +3250,5 @@
>    MOZ_ASSERT(client->mReadDescriptorState.mRunnable);
>    RefPtr<BluetoothReplyRunnable> runnable =
>      client->mReadDescriptorState.mRunnable;
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

DITTO.

@@ +3439,5 @@
>      NS_LITERAL_STRING("ServerRegistered"),
>      uuid, BluetoothValue(uint32_t(aServerIf)));
>  
>    if (server->mConnectPeripheralRunnable) {
> +    ENSURE_GATT_INTF_IS_READY_VOID(server->mConnectPeripheralRunnable);

I think the logic here is incorrect.
In this case, we should also remove this server from the array.
And should continue to handle the AddService case.

@@ +3458,5 @@
>        new ConnectPeripheralResultHandler(server, deviceAddr));
>    }
>  
>    if (server->mAddServiceState.mRunnable) {
> +    ENSURE_GATT_INTF_IS_READY_VOID(server->mAddServiceState.mRunnable);

Please see my above comment.
Attachment #8681113 - Flags: review?(joliu)
Hi Jocelyn,
  This patch covers
  (1) Fix ptr null checking by using ENSURE_GATT_INTF_IS_READY_VOID() instead 
      of NS_ENSURE_TURE_VOID.
  (2) Move the checking statement to more proper place to make it more readable
  (3) Fix the logic err you mentioned last comment also remove server from array

  Please review the patch again, thank you:)
Attachment #8681113 - Attachment is obsolete: true
Hi Jocelyn,
  This patch covers
  (1) Fix ptr null checking by using ENSURE_GATT_INTF_IS_READY_VOID() instead 
      of NS_ENSURE_TURE_VOID.
  (2) Move the checking statement to more proper place to make it more readable
  (3) Fix the logic err you mentioned last comment also remove server from array

  Please review the patch again, thank you:)
Attachment #8686416 - Attachment is obsolete: true
Hi Jocelyn,
  This patch covers
  (1) Fix ptr null checking by using ENSURE_GATT_INTF_IS_READY_VOID() instead 
      of NS_ENSURE_TURE_VOID.
  (2) Move the checking statement to more proper place to make it more readable
  (3) Fix the logic err you mentioned last comment also remove server from array

  Sorry, it's a bit messed up... Please review the patch again, thank you:)
Attachment #8686420 - Attachment is obsolete: true
Attachment #8686437 - Flags: review?(joliu)
Hi Jocelyn,
  This patch covers
  (1) Fix ptr null checking by using ENSURE_GATT_INTF_IS_READY_VOID() instead 
      of NS_ENSURE_TURE_VOID.
  (2) Move the checking statement to more proper place to make it more readable
  (3) Fix the logic err you mentioned last comment also remove server from array

  Sorry again, it's a bit messed up... Please review the patch again, thank you:)
Attachment #8686437 - Attachment is obsolete: true
Attachment #8686437 - Flags: review?(joliu)
Attachment #8687025 - Flags: review?(joliu)
Comment on attachment 8687025 [details] [diff] [review]
Bug 1217778(v2) - Patch: Add & Fix sBluetoothGattService ptr null checking

Review of attachment 8687025 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Louis,

Please see me comments below.

Thanks,
Jocelyn

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +2644,5 @@
>      uuid, BluetoothValue(uint32_t(aClientIf)));
>  
>    if (client->mStartLeScanRunnable) {
>      // Client just registered, proceed remaining startLeScan request.
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mStartLeScanRunnable);

Please add a line in this macro to null out the runnable.

@@ +2868,5 @@
>    bs->DistributeSignal(NS_LITERAL_STRING("ServicesDiscovered"),
>                         client->mAppUuid,
>                         BluetoothValue(client->mServices));
>  
> +

remove this extra line.

@@ +2873,4 @@
>    // All services are discovered, continue to search included services of each
>    // service if existed, otherwise, notify application that discover completed
>    if (!client->mServices.IsEmpty()) {
> +    NS_ENSURE_TRUE_VOID(client->mDiscoverRunnable);

Sorry, I missed that there're more cleanup steps should be done in the attribute discovering case.
Please revise as similar as below.

if (NS_WARN_IF(!sBluetoothGattInterface)) {
  client->NotifyDiscoverCompleted(false);
  return;
}

nit: Define a macro for these lines might be worth a try to save some duplicate codes.

@@ +2932,5 @@
>      // discovering descriptors of this characteristic later
>      client->mCharacteristics.AppendElement(attribute);
>  
>      // Get next characteristic of this service
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mDiscoverRunnable);

DITTO.

@@ +2978,5 @@
>      // Save to mDescriptors for distributing to applications
>      client->mDescriptors.AppendElement(aDescriptorId);
>  
>      // Get next descriptor of this characteristic
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mDiscoverRunnable);

DITTO.

@@ +3021,5 @@
>  
>    RefPtr<BluetoothGattClient> client = sClients->ElementAt(index);
>    MOZ_ASSERT(client->mDiscoverRunnable);
>  
> +  ENSURE_GATT_INTF_IS_READY_VOID(client->mDiscoverRunnable);

DITTO.

@@ +3167,5 @@
>               (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION ||
>                aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION)) {
>      client->mReadCharacteristicState.mAuthRetry = true;
>      // Retry with another authentication requirement
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mReadCharacteristicState.mRunnable);

if (NS_WARN_IF(!sBluetoothGattInterface)) {
  client->mReadCharacteristicState.Reset();
  // Reject the promise
  DispatchReplyError(runnable,
                     NS_LITERAL_STRING("ReadCharacteristicValue failed"));
  return;
}

client->mReadCharacteristicState.mAuthRetry = true;
// Retry with another authentication requirement
sBluetoothGattInterface->ReadCharacteristic(...);

@@ +3207,5 @@
>               (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION ||
>                aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION)) {
>      client->mWriteCharacteristicState.mAuthRetry = true;
>      // Retry with another authentication requirement
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mWriteCharacteristicState.mRunnable);

Same as ReadCharacteristic expect for the error string.

@@ +3263,5 @@
>               (aStatus == GATT_STATUS_INSUFFICIENT_AUTHENTICATION ||
>                aStatus == GATT_STATUS_INSUFFICIENT_ENCRYPTION)) {
>      client->mReadDescriptorState.mAuthRetry = true;
>      // Retry with another authentication requirement
> +    ENSURE_GATT_INTF_IS_READY_VOID(client->mReadDescriptorState.mRunnable);

Same as ReadCharacteristic expect for the error string.

@@ +3295,5 @@
>    MOZ_ASSERT(client->mWriteDescriptorState.mRunnable);
>    RefPtr<BluetoothReplyRunnable> runnable =
>      client->mWriteDescriptorState.mRunnable;
>  
> +  ENSURE_GATT_INTF_IS_READY_VOID(runnable);

Please be consistent with Read/WriteCharacteristic and ReadDescriptor.

@@ +3442,5 @@
>        server->mConnectionMap.Remove(server->mConnectionMap.Iter().Key());
>        return;
>      }
>  
> +    if (!sBluetoothGattInterface) {

How about we combine this check with if (!bs || aStatus != GATT_STATUS_SUCCESS) at the above directly.

@@ +3791,5 @@
>    NS_ENSURE_TRUE_VOID(index != sServers->NoIndex);
>  
>    RefPtr<BluetoothGattServer> server = (*sServers)[index];
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

Move to the line before BluetoothService* bs = BluetoothService::Get();.

@@ +3847,5 @@
>    NS_ENSURE_TRUE_VOID(index != sServers->NoIndex);
>  
>    RefPtr<BluetoothGattServer> server = (*sServers)[index];
>  
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

DITTO.

@@ +3942,5 @@
>     *      discover included services of the next service.
>     * 3) Both arrays are already empty:
>     *      Discover is done, notify application.
>     */
> +  NS_ENSURE_TRUE_VOID(sBluetoothGattInterface);

MOZ_ASSERT(aClient->mDiscoverRunnable);
if (NS_WARN_IF(!sBluetoothGattInterface)) {
  aClient->NotifyDiscoverCompleted(false);
  return;
}
Attachment #8687025 - Flags: review?(joliu)
Hi Jocelyn,
  This patch covers
  (1) add a macro to do sBluetoothGattInterface ptr null checking in attribute  
      discovering base (not sure the name of macro is suitable) 
  (2) Move some checking statement to more proper place to make it more readable
      & remove extra line
  (3) combine sBluetoothGattInterface ptr null checking with (!bs || aStatus  
      != GATT_STATUS_SUCCESS)

  Please review the patch again, thanks.
Attachment #8687025 - Attachment is obsolete: true
Attachment #8691715 - Flags: review?(joliu)
Comment on attachment 8691715 [details] [diff] [review]
Bug 1217778(v3) - Patch: Add sBluetoothGattService ptr null checking

Review of attachment 8691715 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks good to me, only few things to be revised, thanks.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +22,2 @@
>    do {                                                                        \
>      if (!sBluetoothGattInterface) {                                           \

Please add NS_WARN_IF here to be consistent.

@@ +3963,5 @@
>     *      discover included services of the next service.
>     * 3) Both arrays are already empty:
>     *      Discover is done, notify application.
>     */
> +  MOZ_ASSERT(aClinet->mDiscoverRunnable);

aClient

@@ +3967,5 @@
> +  MOZ_ASSERT(aClinet->mDiscoverRunnable);
> +  if (NS_WARN_IF(!sBluetoothGattInterface)) {
> +    aClient->NotifyDiscoverCompleted(false);
> +    return;
> +  }

Please revise to use the same macro for all attr discovery cases.
Attachment #8691715 - Flags: review?(joliu)
Hi Jocelyn,
  In the patch, i fixed up some syntax errors and revise the ENSURE_GATT_INTF_IS_READY_VOID macro with NS_WARN_IF. Also, i revise the ENSURE_GATT_INTF_IN_ATTR_DISCOVER macro and use it overall attr discovery cases.

Thanks.
Attachment #8691715 - Attachment is obsolete: true
Attachment #8693146 - Flags: review?(joliu)
Hi Jocelyn,
  Sorry there is a little mistake in the last patch. I reattach a fixed patch.

In the patch, i fixed up some syntax errors and revise the ENSURE_GATT_INTF_IS_READY_VOID macro with NS_WARN_IF. Also, i revise the ENSURE_GATT_INTF_IN_ATTR_DISCOVER macro and use it overall attr discovery cases.

Thanks.
Attachment #8693146 - Attachment is obsolete: true
Attachment #8693146 - Flags: review?(joliu)
Attachment #8693157 - Flags: review?(joliu)
Comment on attachment 8693157 [details] [diff] [review]
Bug 1217778(v4) - Patch: Add & Fix sBluetoothGattService ptr null checking

Review of attachment 8693157 [details] [diff] [review]:
-----------------------------------------------------------------

Suggest to revise the commit message as "Ensure sBluetoothGattService is not null before accessing GattInterface in GattManager."
Attachment #8693157 - Flags: review?(joliu) → review+
Keywords: checkin-needed
hi, this patch does not apply cleanly:

patching file dom/bluetooth/bluedroid/BluetoothGattManager.cpp
Hunk #2 succeeded at 2573 with fuzz 1 (offset -81 lines).
Hunk #3 FAILED at 2674
Hunk #12 FAILED at 3430
2 out of 15 hunks FAILED -- saving rejects to file dom/bluetooth/bluedroid/BluetoothGattManager.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory


can you take a look, thanks!
Flags: needinfo?(lochang)
Keywords: checkin-needed
Sorry for my carelessness, i upload the wrong patch. The new attachment is the final patch. 

Thanks.
Attachment #8693513 - Attachment is obsolete: true
Flags: needinfo?(lochang)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f514c344a397
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.