subsys/bluetooth/host/rfcomm.c: Missing unlock

[himanshujha199640]

Found using coccinelle.
#11148

himanshu@himanshu-Vostro-3559:~/zephyr$ spatch -D report --sp-file scripts/coccinelle/mini_lock.cocci --very-quiet subsys/
subsys/bluetooth/host/rfcomm.c:519:2-8: preceding lock on line 516
diff -u -p subsys/bluetooth/host/rfcomm.c /tmp/nothing/bluetooth/host/rfcomm.c
--- subsys/bluetooth/host/rfcomm.c
+++ /tmp/nothing/bluetooth/host/rfcomm.c
@@ -513,10 +513,8 @@ static void rfcomm_check_fc(struct bt_rf
 
 	BT_DBG("Wait for credits or MSC FC %p", dlc);
 	/* Wait for credits or MSC FC */
-	k_sem_take(&dlc->tx_credits, K_FOREVER);
 
 	if (dlc->session->cfc == BT_RFCOMM_CFC_SUPPORTED) {
-		return;
 	}
 
 	k_sem_take(&dlc->session->fc, K_FOREVER);
@@ -527,7 +525,6 @@ static void rfcomm_check_fc(struct bt_rf
 	 * with 1, is received.
 	 */
 	k_sem_give(&dlc->session->fc);
-	k_sem_give(&dlc->tx_credits);
 }

[jhedberg]

@jkanakkx any chance you could take a look at this? It seems you were the last one to touch these lines of code.

[jhedberg]

I took a look at the rfcomm.c code, and I'm not completely certain if the proposed fix is the correct one. We'd really need a comment from the code author for this. @jkanakkx would you have a chance to look at this?

[galak]

Seems like a simple fix would be:

diff --git a/subsys/bluetooth/host/rfcomm.c b/subsys/bluetooth/host/rfcomm.c
index 66549f7c5a..b9cc05016b 100644
--- a/subsys/bluetooth/host/rfcomm.c
+++ b/subsys/bluetooth/host/rfcomm.c
@@ -516,6 +516,7 @@ static void rfcomm_check_fc(struct bt_rfcomm_dlc *dlc)
        k_sem_take(&dlc->tx_credits, K_FOREVER);
 
        if (dlc->session->cfc == BT_RFCOMM_CFC_SUPPORTED) {
+               k_sem_give(&dlc->tx_credits);
                return;
        }
 

[jhedberg]

@galak it might seem so, but I'm not convinced after looking at the code that leaving the semaphore taken in this branch wasn't intentional. There are other places in rfcomm.c which will give it back as well.

[himanshujha199640]

@galak it might seem so, but I'm not convinced after looking at the code that leaving the semaphore taken in this branch wasn't intentional. There are other places in rfcomm.c which will give it back as well.

Agreed!
And I added a similar comment in the script about the false positives:
https://github.com/zephyrproject-rtos/zephyr/blob/master/scripts/coccinelle/mini_lock.cocci#L3

Such cases are hard to script/automate in coccinelle with all nested functions
wounded up.

[jhedberg]

@jkanakkx have you had a chance to look at this? It'd be good to get a comment from the original author (you) whether the semaphore handling is intentional or a bug.

[carlescufi]

@jhedberg could you find a way to close this one? Thanks!

[nashif]

@jhedberg ping.

[jhedberg]

@jhedberg ping.

@nashif I don't have any update on this one. I wonder if we could assign it to those who insisted on keeping BR/EDR support in the tree when we proposed to remove it.

[jhedberg]

..or then just close it since it hasn't been definitely proven that it's a bug