[PATCH 1/4] android/gatt: Fix included service notification send

[PATCH 1/4] android/gatt: Fix included service notification send

[Grzegorz Kolodziejczyk]

In case of service not found there was read from invalid memory of a
primary service needed by send notification. Also there is no need to
additionaly check if included service is null outside notifiaction
function because it's already done in this function.
---
 android/gatt.c | 50 ++++++++++++++++++--------------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index e768856..bbf899f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1600,7 +1600,7 @@ reply:
 			HAL_OP_GATT_CLIENT_SEARCH_SERVICE, status);
 }
 
-static void send_client_incl_service_notify(const struct service *prim,
+static void send_client_incl_service_notify(const struct element_id *srvc_id,
 						const struct service *incl,
 						int32_t conn_id,
 						int32_t status)
@@ -1612,7 +1612,7 @@ static void send_client_incl_service_notify(const struct service *prim,
 	ev.conn_id = conn_id;
 	ev.status = status;
 
-	element_id_to_hal_srvc_id(&prim->id, 1, &ev.srvc_id);
+	element_id_to_hal_srvc_id(srvc_id, 1, &ev.srvc_id);
 
 	if (incl)
 		element_id_to_hal_srvc_id(&incl->id, 0, &ev.incl_srvc_id);
@@ -1642,7 +1642,7 @@ static void get_included_cb(uint8_t status, GSList *included, void *user_data)
 	struct get_included_data *data = user_data;
 	struct app_connection *conn = data->conn;
 	struct service *service = data->prim;
-	struct service *incl;
+	struct service *incl = NULL;
 	int instance_id;
 
 	DBG("");
@@ -1695,15 +1695,9 @@ static void get_included_cb(uint8_t status, GSList *included, void *user_data)
 	 */
 	incl = queue_peek_head(service->included);
 
-	if (incl) {
-		send_client_incl_service_notify(service, incl, conn->id,
-								GATT_SUCCESS);
-		return;
-	}
-
 failed:
-	send_client_incl_service_notify(service, NULL, conn->id,
-								GATT_FAILURE);
+	send_client_incl_service_notify(&service->id, incl, conn->id,
+					incl ? GATT_SUCCESS : GATT_FAILURE);
 }
 
 static bool search_included_services(struct app_connection *connection,
@@ -1758,12 +1752,15 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
 	const struct hal_cmd_gatt_client_get_included_service *cmd = buf;
 	struct app_connection *conn;
 	struct service *prim_service;
-	struct service *incl_service;
+	struct service *incl_service = NULL;
 	struct element_id match_id;
+	struct element_id srvc_id;
 	uint8_t status;
 
 	DBG("");
 
+	hal_srvc_id_to_element_id(&cmd->srvc_id, &srvc_id);
+
 	if (len != sizeof(*cmd) +
 			(cmd->continuation ? sizeof(cmd->incl_srvc_id[0]) : 0)) {
 		error("Invalid get incl services size (%u bytes), terminating",
@@ -1784,7 +1781,10 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
 		else
 			status = HAL_STATUS_FAILED;
 
-		goto reply;
+		ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
+				HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
+				status);
+		return;
 	}
 
 	/* Try to use cache here */
@@ -1797,31 +1797,17 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
 						INT_TO_PTR(inst_id));
 	}
 
-	/*
-	 * Note that Android framework expects failure notification
-	 * which is treat as the end of included services
-	 */
-	if (!incl_service)
-		send_client_incl_service_notify(prim_service, NULL,
-						conn->id, GATT_FAILURE);
-	else
-		send_client_incl_service_notify(prim_service, incl_service,
-						conn->id, GATT_SUCCESS);
-
 	status = HAL_STATUS_SUCCESS;
 
 reply:
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
-					HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
-					status);
+			HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE, status);
 
-	/*
-	 * In case of error in handling request we need to send event with
-	 * Android framework is stupid and do not check status of response
+	/* In case of error in handling request we need to send event with
+	 * service id of cmd and gatt failure status.
 	 */
-	if (status)
-		send_client_incl_service_notify(prim_service, NULL,
-						conn->id, GATT_FAILURE);
+	send_client_incl_service_notify(&srvc_id, incl_service, cmd->conn_id,
+					incl_service ? GATT_SUCCESS : GATT_FAILURE);
 }
 
 static void send_client_char_notify(const struct characteristic *ch,
-- 
1.9.2

[PATCH 2/4] android/gatt: Trivial style fix of argument list

[Grzegorz Kolodziejczyk]

---
 android/gatt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index bbf899f..6262c93 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -1832,8 +1832,7 @@ static void send_client_char_notify(const struct characteristic *ch,
 					sizeof(ev), &ev);
 }
 
-static void cache_all_srvc_chars(struct service *srvc,
-							GSList *characteristics)
+static void cache_all_srvc_chars(struct service *srvc, GSList *characteristics)
 {
 	uint16_t inst_id = 0;
 	bt_uuid_t uuid;
-- 
1.9.2

[PATCH 3/4] android/gatt: Trivial fix of indention style

[Grzegorz Kolodziejczyk]

---
 android/gatt.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 6262c93..810a8c7 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -778,8 +778,7 @@ static void send_client_primary_notify(void *data, void *user_data)
 	uuid2android(&p->id.uuid, ev.srvc_id.uuid);
 
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
-					HAL_EV_GATT_CLIENT_SEARCH_RESULT,
-					sizeof(ev), &ev);
+			HAL_EV_GATT_CLIENT_SEARCH_RESULT, sizeof(ev), &ev);
 }
 
 static void send_client_all_primary(struct app_connection *connection,
@@ -1379,7 +1378,7 @@ static void handle_client_disconnect(const void *buf, uint16_t len)
 	status = HAL_STATUS_SUCCESS;
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
-			HAL_OP_GATT_CLIENT_DISCONNECT, status);
+					HAL_OP_GATT_CLIENT_DISCONNECT, status);
 }
 
 static void send_client_listen_notify(int32_t id, int32_t status)
@@ -1390,9 +1389,8 @@ static void send_client_listen_notify(int32_t id, int32_t status)
 	ev.server_if = id;
 	ev.status = status;
 
-	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
-						HAL_EV_GATT_CLIENT_LISTEN,
-						sizeof(ev), &ev);
+	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT, HAL_EV_GATT_CLIENT_LISTEN,
+							sizeof(ev), &ev);
 }
 
 struct listen_data {
@@ -2938,8 +2936,7 @@ failed:
 	send_register_for_notification_ev(conn_id, 1, gatt_status,
 						&cmd->srvc_id, &cmd->char_id);
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
-				HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION,
-				status);
+			HAL_OP_GATT_CLIENT_REGISTER_FOR_NOTIFICATION, status);
 }
 
 static void handle_client_deregister_for_notification(const void *buf,
@@ -2986,8 +2983,7 @@ failed:
 						&cmd->srvc_id, &cmd->char_id);
 
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
-				HAL_OP_GATT_CLIENT_DEREGISTER_FOR_NOTIFICATION,
-				status);
+			HAL_OP_GATT_CLIENT_DEREGISTER_FOR_NOTIFICATION, status);
 }
 
 static void handle_client_read_remote_rssi(const void *buf, uint16_t len)
-- 
1.9.2

[PATCH 4/4] android/gatt: Remove redundant lines

[Grzegorz Kolodziejczyk]

---
 android/gatt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/android/gatt.c b/android/gatt.c
index 810a8c7..3064b7f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -795,7 +795,6 @@ static void send_client_all_primary(struct app_connection *connection,
 	ev.conn_id = connection->id;
 	ipc_send_notif(hal_ipc, HAL_SERVICE_ID_GATT,
 			HAL_EV_GATT_CLIENT_SEARCH_COMPLETE, sizeof(ev), &ev);
-
 }
 
 static struct service *create_service(uint8_t id, bool primary, char *uuid,
@@ -2393,7 +2392,6 @@ static void handle_client_write_characteristic(const void *buf, uint16_t len)
 	status = HAL_STATUS_SUCCESS;
 
 failed:
-
 	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
 			HAL_OP_GATT_CLIENT_WRITE_CHARACTERISTIC, status);
 
-- 
1.9.2

Re: [PATCH 1/4] android/gatt: Fix included service notification send

[Szymon Janc]

Hi Grzegorz,

On Tuesday 06 of May 2014 11:57:03 Grzegorz Kolodziejczyk wrote:
> In case of service not found there was read from invalid memory of a
> primary service needed by send notification. Also there is no need to
> additionaly check if included service is null outside notifiaction
> function because it's already done in this function.
> ---
>  android/gatt.c | 50 ++++++++++++++++++--------------------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/android/gatt.c b/android/gatt.c
> index e768856..bbf899f 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -1600,7 +1600,7 @@ reply:
>  			HAL_OP_GATT_CLIENT_SEARCH_SERVICE, status);
>  }
>  
> -static void send_client_incl_service_notify(const struct service *prim,
> +static void send_client_incl_service_notify(const struct element_id *srvc_id,
>  						const struct service *incl,
>  						int32_t conn_id,
>  						int32_t status)
> @@ -1612,7 +1612,7 @@ static void send_client_incl_service_notify(const struct service *prim,
>  	ev.conn_id = conn_id;
>  	ev.status = status;
>  
> -	element_id_to_hal_srvc_id(&prim->id, 1, &ev.srvc_id);
> +	element_id_to_hal_srvc_id(srvc_id, 1, &ev.srvc_id);
>  
>  	if (incl)
>  		element_id_to_hal_srvc_id(&incl->id, 0, &ev.incl_srvc_id);
> @@ -1642,7 +1642,7 @@ static void get_included_cb(uint8_t status, GSList *included, void *user_data)
>  	struct get_included_data *data = user_data;
>  	struct app_connection *conn = data->conn;
>  	struct service *service = data->prim;
> -	struct service *incl;
> +	struct service *incl = NULL;
>  	int instance_id;
>  
>  	DBG("");
> @@ -1695,15 +1695,9 @@ static void get_included_cb(uint8_t status, GSList *included, void *user_data)
>  	 */
>  	incl = queue_peek_head(service->included);
>  
> -	if (incl) {
> -		send_client_incl_service_notify(service, incl, conn->id,
> -								GATT_SUCCESS);
> -		return;
> -	}
> -
>  failed:
> -	send_client_incl_service_notify(service, NULL, conn->id,
> -								GATT_FAILURE);
> +	send_client_incl_service_notify(&service->id, incl, conn->id,
> +					incl ? GATT_SUCCESS : GATT_FAILURE);
>  }
>  
>  static bool search_included_services(struct app_connection *connection,
> @@ -1758,12 +1752,15 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
>  	const struct hal_cmd_gatt_client_get_included_service *cmd = buf;
>  	struct app_connection *conn;
>  	struct service *prim_service;
> -	struct service *incl_service;
> +	struct service *incl_service = NULL;
>  	struct element_id match_id;
> +	struct element_id srvc_id;
>  	uint8_t status;
>  
>  	DBG("");
>  
> +	hal_srvc_id_to_element_id(&cmd->srvc_id, &srvc_id);
> +
>  	if (len != sizeof(*cmd) +
>  			(cmd->continuation ? sizeof(cmd->incl_srvc_id[0]) : 0)) {
>  		error("Invalid get incl services size (%u bytes), terminating",
> @@ -1784,7 +1781,10 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
>  		else
>  			status = HAL_STATUS_FAILED;
>  
> -		goto reply;
> +		ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
> +				HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
> +				status);
> +		return;
>  	}
>  
>  	/* Try to use cache here */
> @@ -1797,31 +1797,17 @@ static void handle_client_get_included_service(const void *buf, uint16_t len)
>  						INT_TO_PTR(inst_id));
>  	}
>  
> -	/*
> -	 * Note that Android framework expects failure notification
> -	 * which is treat as the end of included services
> -	 */
> -	if (!incl_service)
> -		send_client_incl_service_notify(prim_service, NULL,
> -						conn->id, GATT_FAILURE);
> -	else
> -		send_client_incl_service_notify(prim_service, incl_service,
> -						conn->id, GATT_SUCCESS);
> -
>  	status = HAL_STATUS_SUCCESS;
>  
>  reply:
>  	ipc_send_rsp(hal_ipc, HAL_SERVICE_ID_GATT,
> -					HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE,
> -					status);
> +			HAL_OP_GATT_CLIENT_GET_INCLUDED_SERVICE, status);
>  
> -	/*
> -	 * In case of error in handling request we need to send event with
> -	 * Android framework is stupid and do not check status of response
> +	/* In case of error in handling request we need to send event with
> +	 * service id of cmd and gatt failure status.
>  	 */
> -	if (status)
> -		send_client_incl_service_notify(prim_service, NULL,
> -						conn->id, GATT_FAILURE);
> +	send_client_incl_service_notify(&srvc_id, incl_service, cmd->conn_id,
> +					incl_service ? GATT_SUCCESS : GATT_FAILURE);
>  }
>  
>  static void send_client_char_notify(const struct characteristic *ch,
> 

Pushed (patches 2-4 squashed). Thanks.

-- 
Best regards, 
Szymon Janc