From: Andrei Emeltchenko <[email protected]>
gatt_db_attribute_get_permissions() used everywhere without check since
those conditions are checked already.
---
android/gatt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 086bb94..04101f6 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- if (!gatt_db_attribute_get_permissions(attrib, &permissions))
- return;
+ gatt_db_attribute_get_permissions(attrib, &permissions);
if (check_device_permissions(dev, cmd[0], permissions))
return;
--
1.9.1
Hi Andrei,
On Mon, Nov 17, 2014 at 3:03 PM, Andrei Emeltchenko
<[email protected]> wrote:
> ping
>
> On Thu, Nov 13, 2014 at 07:13:37PM +0200, Andrei Emeltchenko wrote:
>> From: Andrei Emeltchenko <[email protected]>
>>
>> Condition checks inside gatt_db_attribute_get_permissions() do not make
>> sense. Simplify function.
>> ---
>> android/gatt.c | 11 +++++------
>> src/shared/gatt-db.c | 10 ++--------
>> src/shared/gatt-db.h | 3 +--
>> src/shared/gatt-server.c | 20 ++++----------------
>> 4 files changed, 12 insertions(+), 32 deletions(-)
>>
>> diff --git a/android/gatt.c b/android/gatt.c
>> index 086bb94..085b1e5 100644
>> --- a/android/gatt.c
>> +++ b/android/gatt.c
>> @@ -4753,7 +4753,7 @@ static void read_requested_attributes(void *data, void *user_data)
>> return;
>> }
>>
>> - gatt_db_attribute_get_permissions(attrib, &permissions);
>> + permissions = gatt_db_attribute_get_permissions(attrib);
>>
>> /*
>> * Check if it is attribute we didn't declare permissions, like service
>> @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>> if (!attrib)
>> return;
>>
>> - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
>> - return;
>> + permissions = gatt_db_attribute_get_permissions(attrib);
>>
>> if (check_device_permissions(dev, cmd[0], permissions))
>> return;
>> @@ -6041,7 +6040,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
>> if (!attrib)
>> return;
>>
>> - gatt_db_attribute_get_permissions(attrib, &permissions);
>> + permissions = gatt_db_attribute_get_permissions(attrib);
>>
>> if (check_device_permissions(dev, cmd[0], permissions))
>> return;
>> @@ -6109,7 +6108,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
>> if (!attrib)
>> return ATT_ECODE_ATTR_NOT_FOUND;
>>
>> - gatt_db_attribute_get_permissions(attrib, &permissions);
>> + permissions = gatt_db_attribute_get_permissions(attrib);
>>
>> error = check_device_permissions(dev, cmd[0], permissions);
>> if (error)
>> @@ -6165,7 +6164,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
>> if (!attrib)
>> return ATT_ECODE_ATTR_NOT_FOUND;
>>
>> - gatt_db_attribute_get_permissions(attrib, &permissions);
>> + permissions = gatt_db_attribute_get_permissions(attrib);
>>
>> error = check_device_permissions(dev, cmd[0], permissions);
>> if (error)
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index a39eec2..3c916f9 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -769,15 +769,9 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
>> return true;
>> }
>>
>> -bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> - uint32_t *permissions)
>> +uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib)
>> {
>> - if (!attrib || !permissions)
>> - return false;
>> -
>> - *permissions = attrib->permissions;
>> -
>> - return true;
>> + return attrib->permissions;
>> }
You are actually changing the API here, perhaps you want to explain
why you want to do that. There doesn't seem to be anything blocking us
to change this, except that we do return false if there is no
permission set but anyway I don't think we do anything special in this
case but it could in theory be used to bypass checks.
>> static void pending_read_result(struct pending_read *p, int err,
>> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
>> index 9c71814..03eebd4 100644
>> --- a/src/shared/gatt-db.h
>> +++ b/src/shared/gatt-db.h
>> @@ -103,8 +103,7 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
>> uint16_t *start_handle,
>> uint16_t *end_handle);
>>
>> -bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
>> - uint32_t *permissions);
>> +uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
>>
>> typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
>> int err, const uint8_t *value,
>> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
>> index 2ca318a..dbcb89a 100644
>> --- a/src/shared/gatt-server.c
>> +++ b/src/shared/gatt-server.c
>> @@ -412,10 +412,7 @@ static void process_read_by_type(struct async_read_op *op)
>> goto done;
>> }
>>
>> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
>> - ecode = BT_ATT_ERROR_UNLIKELY;
>> - goto error;
>> - }
>> + perm = gatt_db_attribute_get_permissions(attr);
>>
>> /*
>> * Check for the READ access permission. Encryption,
>> @@ -739,10 +736,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
>> (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
>> handle);
>>
>> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
>> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
>> - goto error;
>> - }
>> + perm = gatt_db_attribute_get_permissions(attr);
>>
>> if (!(perm & BT_ATT_PERM_WRITE)) {
>> ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
>> @@ -863,10 +857,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
>> opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
>> handle);
>>
>> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
>> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
>> - goto error;
>> - }
>> + perm = gatt_db_attribute_get_permissions(attr);
>>
>> if (perm && !(perm & BT_ATT_PERM_READ)) {
>> ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
>> @@ -980,10 +971,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
>> util_debug(server->debug_callback, server->debug_data,
>> "Prep Write Req - handle: 0x%04x", handle);
>>
>> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
>> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
>> - goto error;
>> - }
>> + perm = gatt_db_attribute_get_permissions(attr);
>>
>> /*
>> * TODO: The "Prepare Write" request requires security permission checks
>> --
>> 2.1.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Luiz Augusto von Dentz
ping
On Thu, Nov 13, 2014 at 07:13:37PM +0200, Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Condition checks inside gatt_db_attribute_get_permissions() do not make
> sense. Simplify function.
> ---
> android/gatt.c | 11 +++++------
> src/shared/gatt-db.c | 10 ++--------
> src/shared/gatt-db.h | 3 +--
> src/shared/gatt-server.c | 20 ++++----------------
> 4 files changed, 12 insertions(+), 32 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 086bb94..085b1e5 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4753,7 +4753,7 @@ static void read_requested_attributes(void *data, void *user_data)
> return;
> }
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> /*
> * Check if it is attribute we didn't declare permissions, like service
> @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> - return;
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
> @@ -6041,7 +6040,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
> @@ -6109,7 +6108,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return ATT_ECODE_ATTR_NOT_FOUND;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> error = check_device_permissions(dev, cmd[0], permissions);
> if (error)
> @@ -6165,7 +6164,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return ATT_ECODE_ATTR_NOT_FOUND;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> error = check_device_permissions(dev, cmd[0], permissions);
> if (error)
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index a39eec2..3c916f9 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -769,15 +769,9 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
> return true;
> }
>
> -bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> - uint32_t *permissions)
> +uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib)
> {
> - if (!attrib || !permissions)
> - return false;
> -
> - *permissions = attrib->permissions;
> -
> - return true;
> + return attrib->permissions;
> }
>
> static void pending_read_result(struct pending_read *p, int err,
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index 9c71814..03eebd4 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -103,8 +103,7 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
> uint16_t *start_handle,
> uint16_t *end_handle);
>
> -bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
> - uint32_t *permissions);
> +uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
>
> typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
> int err, const uint8_t *value,
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 2ca318a..dbcb89a 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -412,10 +412,7 @@ static void process_read_by_type(struct async_read_op *op)
> goto done;
> }
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_UNLIKELY;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> /*
> * Check for the READ access permission. Encryption,
> @@ -739,10 +736,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
> (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
> handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> if (!(perm & BT_ATT_PERM_WRITE)) {
> ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
> @@ -863,10 +857,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
> opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
> handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> if (perm && !(perm & BT_ATT_PERM_READ)) {
> ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
> @@ -980,10 +971,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
> util_debug(server->debug_callback, server->debug_data,
> "Prep Write Req - handle: 0x%04x", handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> /*
> * TODO: The "Prepare Write" request requires security permission checks
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Andrei Emeltchenko <[email protected]>
Condition checks inside gatt_db_attribute_get_permissions() do not make
sense. Simplify function.
---
android/gatt.c | 11 +++++------
src/shared/gatt-db.c | 10 ++--------
src/shared/gatt-db.h | 3 +--
src/shared/gatt-server.c | 20 ++++----------------
4 files changed, 12 insertions(+), 32 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 086bb94..085b1e5 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4753,7 +4753,7 @@ static void read_requested_attributes(void *data, void *user_data)
return;
}
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
/*
* Check if it is attribute we didn't declare permissions, like service
@@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- if (!gatt_db_attribute_get_permissions(attrib, &permissions))
- return;
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6041,7 +6040,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6109,7 +6108,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
@@ -6165,7 +6164,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index a39eec2..3c916f9 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -769,15 +769,9 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
return true;
}
-bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
- uint32_t *permissions)
+uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib)
{
- if (!attrib || !permissions)
- return false;
-
- *permissions = attrib->permissions;
-
- return true;
+ return attrib->permissions;
}
static void pending_read_result(struct pending_read *p, int err,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 9c71814..03eebd4 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -103,8 +103,7 @@ bool gatt_db_attribute_get_service_handles(struct gatt_db_attribute *attrib,
uint16_t *start_handle,
uint16_t *end_handle);
-bool gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib,
- uint32_t *permissions);
+uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
int err, const uint8_t *value,
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 2ca318a..dbcb89a 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -412,10 +412,7 @@ static void process_read_by_type(struct async_read_op *op)
goto done;
}
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_UNLIKELY;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* Check for the READ access permission. Encryption,
@@ -739,10 +736,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
(opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (!(perm & BT_ATT_PERM_WRITE)) {
ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
@@ -863,10 +857,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (perm && !(perm & BT_ATT_PERM_READ)) {
ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
@@ -980,10 +971,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
util_debug(server->debug_callback, server->debug_data,
"Prep Write Req - handle: 0x%04x", handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* TODO: The "Prepare Write" request requires security permission checks
--
2.1.0
Hi Szymon,
On Thu, Nov 13, 2014 at 02:24:40PM +0100, Szymon Janc wrote:
> Hi Andrei,
>
> On Tuesday 11 of November 2014 10:22:43 Andrei Emeltchenko wrote:
> > Hi Szymon,
> >
> > On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote:
> > > Hi Andrei,
> > >
> > > On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote:
> > > > From: Andrei Emeltchenko <[email protected]>
> > > >
> > > > gatt_db_attribute_get_permissions() used everywhere without check since
> > > > those conditions are checked already.
> > >
> > > It would make sense to put such warning into commit message if you are
> > > referring to it in subject.
> > >
> > > > ---
> > > > android/gatt.c | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/android/gatt.c b/android/gatt.c
> > > > index 086bb94..04101f6 100644
> > > > --- a/android/gatt.c
> > > > +++ b/android/gatt.c
> > > > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd,
> > > > uint16_t cmd_len, if (!attrib)
> > > > return;
> > > >
> > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > > > - return;
> > > > + gatt_db_attribute_get_permissions(attrib, &permissions);
> > > >
> > > > if (check_device_permissions(dev, cmd[0], permissions))
> > > > return;
> > >
> > > So if we always pass valid pointers to this function, then maybe we should
> > > change it definition to something like:
> > >
> > > uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
> > >
> >
> > Do we really need a helper which just does reference?
>
> gatt_db_attribute is private structure.
Ah, true. I will rebase the patch following you suggestion.
Best regards
Andrei Emeltchenko
Hi Andrei,
On Tuesday 11 of November 2014 10:22:43 Andrei Emeltchenko wrote:
> Hi Szymon,
>
> On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote:
> > Hi Andrei,
> >
> > On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote:
> > > From: Andrei Emeltchenko <[email protected]>
> > >
> > > gatt_db_attribute_get_permissions() used everywhere without check since
> > > those conditions are checked already.
> >
> > It would make sense to put such warning into commit message if you are
> > referring to it in subject.
> >
> > > ---
> > > android/gatt.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/android/gatt.c b/android/gatt.c
> > > index 086bb94..04101f6 100644
> > > --- a/android/gatt.c
> > > +++ b/android/gatt.c
> > > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd,
> > > uint16_t cmd_len, if (!attrib)
> > > return;
> > >
> > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > > - return;
> > > + gatt_db_attribute_get_permissions(attrib, &permissions);
> > >
> > > if (check_device_permissions(dev, cmd[0], permissions))
> > > return;
> >
> > So if we always pass valid pointers to this function, then maybe we should
> > change it definition to something like:
> >
> > uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
> >
>
> Do we really need a helper which just does reference?
gatt_db_attribute is private structure.
--
Best regards,
Szymon Janc
Hi Szymon,
On Mon, Nov 10, 2014 at 09:05:41PM +0100, Szymon Janc wrote:
> Hi Andrei,
>
> On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > gatt_db_attribute_get_permissions() used everywhere without check since
> > those conditions are checked already.
>
> It would make sense to put such warning into commit message if you are
> referring to it in subject.
>
> > ---
> > android/gatt.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/android/gatt.c b/android/gatt.c
> > index 086bb94..04101f6 100644
> > --- a/android/gatt.c
> > +++ b/android/gatt.c
> > @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd,
> > uint16_t cmd_len, if (!attrib)
> > return;
> >
> > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > - return;
> > + gatt_db_attribute_get_permissions(attrib, &permissions);
> >
> > if (check_device_permissions(dev, cmd[0], permissions))
> > return;
>
> So if we always pass valid pointers to this function, then maybe we should
> change it definition to something like:
>
> uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
>
Do we really need a helper which just does reference?
Best regards
Andrei Emeltchenko
Hi Andrei,
On Monday 10 of November 2014 15:25:18 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> gatt_db_attribute_get_permissions() used everywhere without check since
> those conditions are checked already.
It would make sense to put such warning into commit message if you are
referring to it in subject.
> ---
> android/gatt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 086bb94..04101f6 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -5992,8 +5992,7 @@ static void write_cmd_request(const uint8_t *cmd,
> uint16_t cmd_len, if (!attrib)
> return;
>
> - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> - return;
> + gatt_db_attribute_get_permissions(attrib, &permissions);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
So if we always pass valid pointers to this function, then maybe we should
change it definition to something like:
uint32_t gatt_db_attribute_get_permissions(struct gatt_db_attribute *attrib);
Thoughts?
--
BR
Szymon Janc
From: Andrei Emeltchenko <[email protected]>
Condition checks inside gatt_db_attribute_get_permissions() do not make
sense. Simplify function.
---
android/gatt.c | 10 +++++-----
src/shared/gatt-db.c | 10 ++--------
src/shared/gatt-db.h | 3 +--
src/shared/gatt-server.c | 20 ++++----------------
4 files changed, 12 insertions(+), 31 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 169f6db..6828f2f 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4790,7 +4790,7 @@ static void read_requested_attributes(void *data, void *user_data)
return;
}
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
/*
* Check if it is attribute we didn't declare permissions, like service
@@ -6311,7 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6359,7 +6359,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6430,7 +6430,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
@@ -6486,7 +6486,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index bb60904..4b91ae6 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1301,15 +1301,9 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
return true;
}
-bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
- uint32_t *permissions)
+uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib)
{
- if (!attrib || !permissions)
- return false;
-
- *permissions = attrib->permissions;
-
- return true;
+ return attrib->permissions;
}
static void pending_read_result(struct pending_read *p, int err,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index e5fe6bb..1379aae 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -171,8 +171,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
uint16_t *start_handle,
uint16_t *end_handle);
-bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
- uint32_t *permissions);
+uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib);
typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
int err, const uint8_t *value,
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index 00f36fd..f9e412d 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -389,10 +389,7 @@ static void process_read_by_type(struct async_read_op *op)
return;
}
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_UNLIKELY;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* Check for the READ access permission. Encryption,
@@ -697,10 +694,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
(opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (!(perm & BT_ATT_PERM_WRITE)) {
ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
@@ -813,10 +807,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (perm && !(perm & BT_ATT_PERM_READ)) {
ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
@@ -919,10 +910,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
util_debug(server->debug_callback, server->debug_data,
"Prep Write Req - handle: 0x%04x", handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* TODO: The "Prepare Write" request requires security permission checks
--
2.1.0
Hi Szymon,
On Fri, Dec 19, 2014 at 11:15:43AM +0100, Szymon Janc wrote:
> Hi Andrei,
>
> On Friday 19 of December 2014 12:14:50 Andrei Emeltchenko wrote:
> > Hi Szymon,
> >
> > On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote:
> > > Hi Andrei,
> > >
> > > On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote:
> > > > From: Andrei Emeltchenko <[email protected]>
> > > >
> > > > gatt_db_attribute_get_permissions() used everywhere without check since
> > > > those conditions are checked already.
> > > > ---
> > > > android/gatt.c | 3 +--
> > > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/android/gatt.c b/android/gatt.c
> > > > index 0258d91..169f6db 100644
> > > > --- a/android/gatt.c
> > > > +++ b/android/gatt.c
> > > > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> > > > if (!attrib)
> > > > return;
> > > >
> > > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > > > - return;
> > > > + gatt_db_attribute_get_permissions(attrib, &permissions);
> > > >
> > > > if (check_device_permissions(dev, cmd[0], permissions))
> > > > return;
> > >
> > > Wasn't identical patch already reviewed few weeks ago?
> > >
> >
> > It is still valid. No better approach was found.
>
> I suggested possible solution which you implemented in V2. But you seem to
> ignored comment from Luiz so this didn't moved on.
I still have the patch, it shall be applied anyway on top of this patch
since they are fixing __different__ things. AFAIK Luiz did not propose
anything he was just against the patch.
Best regards
Andrei Emeltchenko
Hi Andrei,
On Friday 19 of December 2014 12:14:50 Andrei Emeltchenko wrote:
> Hi Szymon,
>
> On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote:
> > Hi Andrei,
> >
> > On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote:
> > > From: Andrei Emeltchenko <[email protected]>
> > >
> > > gatt_db_attribute_get_permissions() used everywhere without check since
> > > those conditions are checked already.
> > > ---
> > > android/gatt.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/android/gatt.c b/android/gatt.c
> > > index 0258d91..169f6db 100644
> > > --- a/android/gatt.c
> > > +++ b/android/gatt.c
> > > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> > > if (!attrib)
> > > return;
> > >
> > > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > > - return;
> > > + gatt_db_attribute_get_permissions(attrib, &permissions);
> > >
> > > if (check_device_permissions(dev, cmd[0], permissions))
> > > return;
> >
> > Wasn't identical patch already reviewed few weeks ago?
> >
>
> It is still valid. No better approach was found.
I suggested possible solution which you implemented in V2. But you seem to
ignored comment from Luiz so this didn't moved on.
--
Best regards,
Szymon Janc
Hi Szymon,
On Fri, Dec 19, 2014 at 11:07:55AM +0100, Szymon Janc wrote:
> Hi Andrei,
>
> On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > gatt_db_attribute_get_permissions() used everywhere without check since
> > those conditions are checked already.
> > ---
> > android/gatt.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/android/gatt.c b/android/gatt.c
> > index 0258d91..169f6db 100644
> > --- a/android/gatt.c
> > +++ b/android/gatt.c
> > @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> > if (!attrib)
> > return;
> >
> > - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> > - return;
> > + gatt_db_attribute_get_permissions(attrib, &permissions);
> >
> > if (check_device_permissions(dev, cmd[0], permissions))
> > return;
>
> Wasn't identical patch already reviewed few weeks ago?
>
It is still valid. No better approach was found.
Best regards
Andrei Emeltchenko
Hi Andrei,
On Friday 19 of December 2014 11:24:40 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> gatt_db_attribute_get_permissions() used everywhere without check since
> those conditions are checked already.
> ---
> android/gatt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 0258d91..169f6db 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -6311,8 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> - return;
> + gatt_db_attribute_get_permissions(attrib, &permissions);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
Wasn't identical patch already reviewed few weeks ago?
--
Best regards,
Szymon Janc
Hi Andrei,
On Tuesday 13 of January 2015 12:09:18 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> gatt_db_attribute_get_permissions() used everywhere without check since
> those conditions are checked already.
> ---
> android/gatt.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index b749705..8542f45 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -6355,8 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - if (!gatt_db_attribute_get_permissions(attrib, &permissions))
> - return;
> + gatt_db_attribute_get_permissions(attrib, &permissions);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
>
Both patches applied, thanks.
--
Best regards,
Szymon Janc
From: Andrei Emeltchenko <[email protected]>
gatt_db_attribute_get_permissions() used everywhere without check since
those conditions are checked already.
---
android/gatt.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index b749705..8542f45 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -6355,8 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- if (!gatt_db_attribute_get_permissions(attrib, &permissions))
- return;
+ gatt_db_attribute_get_permissions(attrib, &permissions);
if (check_device_permissions(dev, cmd[0], permissions))
return;
--
2.1.0
From: Andrei Emeltchenko <[email protected]>
Refactoring makes code cleaner and consistent with other code like
function gatt_db_attribute_get_handle(), etc. Also removes compile
warnings related to unneeded checks like check for !permissions and
warnings related to dead check for construction below:
(!gatt_db_attribute_get_permissions(attr, &perm)
those checks are useless since attr is always checked before and check
for &perm is really stupid.
---
android/gatt.c | 10 +++++-----
src/shared/gatt-db.c | 11 ++++-------
src/shared/gatt-db.h | 3 +--
src/shared/gatt-server.c | 20 ++++----------------
4 files changed, 14 insertions(+), 30 deletions(-)
diff --git a/android/gatt.c b/android/gatt.c
index 8542f45..1d65c2a 100644
--- a/android/gatt.c
+++ b/android/gatt.c
@@ -4817,7 +4817,7 @@ static void read_requested_attributes(void *data, void *user_data)
return;
}
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
/*
* Check if it is attribute we didn't declare permissions, like service
@@ -6355,7 +6355,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6403,7 +6403,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
if (check_device_permissions(dev, cmd[0], permissions))
return;
@@ -6474,7 +6474,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
@@ -6530,7 +6530,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
if (!attrib)
return ATT_ECODE_ATTR_NOT_FOUND;
- gatt_db_attribute_get_permissions(attrib, &permissions);
+ permissions = gatt_db_attribute_get_permissions(attrib);
error = check_device_permissions(dev, cmd[0], permissions);
if (error)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 9a9cadc..2b997f1 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1345,15 +1345,12 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
return true;
}
-bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
- uint32_t *permissions)
+uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib)
{
- if (!attrib || !permissions)
- return false;
-
- *permissions = attrib->permissions;
+ if (!attrib)
+ return 0;
- return true;
+ return attrib->permissions;
}
static void pending_read_result(struct pending_read *p, int err,
diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
index 1f4005e..169108a 100644
--- a/src/shared/gatt-db.h
+++ b/src/shared/gatt-db.h
@@ -181,8 +181,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
uint16_t *start_handle,
uint16_t *end_handle);
-bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
- uint32_t *permissions);
+uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib);
typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
int err, const uint8_t *value,
diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
index ed1b274..f12ac47 100644
--- a/src/shared/gatt-server.c
+++ b/src/shared/gatt-server.c
@@ -391,10 +391,7 @@ static void process_read_by_type(struct async_read_op *op)
return;
}
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_UNLIKELY;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* Check for the READ access permission. Encryption,
@@ -791,10 +788,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
(opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (!(perm & BT_ATT_PERM_WRITE)) {
ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
@@ -907,10 +901,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
if (perm && !(perm & BT_ATT_PERM_READ)) {
ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
@@ -1013,10 +1004,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
util_debug(server->debug_callback, server->debug_data,
"Prep Write Req - handle: 0x%04x", handle);
- if (!gatt_db_attribute_get_permissions(attr, &perm)) {
- ecode = BT_ATT_ERROR_INVALID_HANDLE;
- goto error;
- }
+ perm = gatt_db_attribute_get_permissions(attr);
/*
* TODO: The "Prepare Write" request requires security permission checks
--
2.1.0
Hi Andrei,
On Friday 19 of December 2014 12:18:29 Andrei Emeltchenko wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Condition checks inside gatt_db_attribute_get_permissions() do not make
> sense. Simplify function.
In general I'm fine with this change but commit message should be improved.
ie. there is API change so this is not just simplification.
> ---
> android/gatt.c | 10 +++++-----
> src/shared/gatt-db.c | 10 ++--------
> src/shared/gatt-db.h | 3 +--
> src/shared/gatt-server.c | 20 ++++----------------
> 4 files changed, 12 insertions(+), 31 deletions(-)
>
> diff --git a/android/gatt.c b/android/gatt.c
> index 169f6db..6828f2f 100644
> --- a/android/gatt.c
> +++ b/android/gatt.c
> @@ -4790,7 +4790,7 @@ static void read_requested_attributes(void *data, void *user_data)
> return;
> }
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> /*
> * Check if it is attribute we didn't declare permissions, like service
> @@ -6311,7 +6311,7 @@ static void write_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
> @@ -6359,7 +6359,7 @@ static void write_signed_cmd_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> if (check_device_permissions(dev, cmd[0], permissions))
> return;
> @@ -6430,7 +6430,7 @@ static uint8_t write_req_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return ATT_ECODE_ATTR_NOT_FOUND;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> error = check_device_permissions(dev, cmd[0], permissions);
> if (error)
> @@ -6486,7 +6486,7 @@ static uint8_t write_prep_request(const uint8_t *cmd, uint16_t cmd_len,
> if (!attrib)
> return ATT_ECODE_ATTR_NOT_FOUND;
>
> - gatt_db_attribute_get_permissions(attrib, &permissions);
> + permissions = gatt_db_attribute_get_permissions(attrib);
>
> error = check_device_permissions(dev, cmd[0], permissions);
> if (error)
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index bb60904..4b91ae6 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -1301,15 +1301,9 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
> return true;
> }
>
> -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
> - uint32_t *permissions)
> +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib)
> {
> - if (!attrib || !permissions)
> - return false;
> -
> - *permissions = attrib->permissions;
> -
> - return true;
> + return attrib->permissions;
> }
Let check if attrib is NULL and return 0 in such case. This is to keep convention with
other getter-like functions.
>
> static void pending_read_result(struct pending_read *p, int err,
> diff --git a/src/shared/gatt-db.h b/src/shared/gatt-db.h
> index e5fe6bb..1379aae 100644
> --- a/src/shared/gatt-db.h
> +++ b/src/shared/gatt-db.h
> @@ -171,8 +171,7 @@ bool gatt_db_attribute_get_incl_data(const struct gatt_db_attribute *attrib,
> uint16_t *start_handle,
> uint16_t *end_handle);
>
> -bool gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib,
> - uint32_t *permissions);
> +uint32_t gatt_db_attribute_get_permissions(const struct gatt_db_attribute *attrib);
>
> typedef void (*gatt_db_attribute_read_t) (struct gatt_db_attribute *attrib,
> int err, const uint8_t *value,
> diff --git a/src/shared/gatt-server.c b/src/shared/gatt-server.c
> index 00f36fd..f9e412d 100644
> --- a/src/shared/gatt-server.c
> +++ b/src/shared/gatt-server.c
> @@ -389,10 +389,7 @@ static void process_read_by_type(struct async_read_op *op)
> return;
> }
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_UNLIKELY;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> /*
> * Check for the READ access permission. Encryption,
> @@ -697,10 +694,7 @@ static void write_cb(uint8_t opcode, const void *pdu,
> (opcode == BT_ATT_OP_WRITE_REQ) ? "Req" : "Cmd",
> handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> if (!(perm & BT_ATT_PERM_WRITE)) {
> ecode = BT_ATT_ERROR_WRITE_NOT_PERMITTED;
> @@ -813,10 +807,7 @@ static void handle_read_req(struct bt_gatt_server *server, uint8_t opcode,
> opcode == BT_ATT_OP_READ_BLOB_REQ ? "Blob " : "",
> handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> if (perm && !(perm & BT_ATT_PERM_READ)) {
> ecode = BT_ATT_ERROR_READ_NOT_PERMITTED;
> @@ -919,10 +910,7 @@ static void prep_write_cb(uint8_t opcode, const void *pdu,
> util_debug(server->debug_callback, server->debug_data,
> "Prep Write Req - handle: 0x%04x", handle);
>
> - if (!gatt_db_attribute_get_permissions(attr, &perm)) {
> - ecode = BT_ATT_ERROR_INVALID_HANDLE;
> - goto error;
> - }
> + perm = gatt_db_attribute_get_permissions(attr);
>
> /*
> * TODO: The "Prepare Write" request requires security permission checks
>
--
Best regards,
Szymon Janc