gatt_db_attribute_write returns false if realloc returns NULL in the
case where there is no callback. If the write is performed with a 0
length value and 0 offset, realloc will return NULL according to its
specification (as it will practically act as 'free'). This patch fixes
the code so that this case isn't treated as an error.
---
src/shared/gatt-db.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 3d2e690..b331f54 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -1464,7 +1464,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
}
/* Guard against invalid access if offset equals to value length */
- value = offset == attrib->value_len ? NULL : &attrib->value[offset];
+ value = attrib->value_len - offset ? &attrib->value[offset] : NULL;
func(attrib, 0, value, attrib->value_len - offset, user_data);
@@ -1557,15 +1557,14 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
void *buf;
buf = realloc(attrib->value, len + offset);
- if (!buf)
+ if (!buf && len + offset > 0)
return false;
- attrib->value = buf;
-
/* Init data in the first allocation */
- if (!attrib->value_len)
- memset(attrib->value, 0, offset);
+ if (!attrib->value)
+ memset(buf, 0, offset);
+ attrib->value = buf;
attrib->value_len = len + offset;
}
--
2.2.0.rc0.207.ga3a616c
Hi Luiz,
> On Thu, Feb 19, 2015 at 4:42 AM, Luiz Augusto von Dentz <[email protected]> wrote:
> Hi Arman,
>
> On Thu, Feb 19, 2015 at 2:18 PM, Luiz Augusto von Dentz
> <[email protected]> wrote:
>> Hi Arman,
>>
>> On Thu, Feb 19, 2015 at 2:12 AM, Arman Uguray <[email protected]> wrote:
>>> gatt_db_attribute_write returns false if realloc returns NULL in the
>>> case where there is no callback. If the write is performed with a 0
>>> length value and 0 offset, realloc will return NULL according to its
>>> specification (as it will practically act as 'free'). This patch fixes
>>> the code so that this case isn't treated as an error.
>>> ---
>>> src/shared/gatt-db.c | 11 +++++------
>>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>>> index 3d2e690..b331f54 100644
>>> --- a/src/shared/gatt-db.c
>>> +++ b/src/shared/gatt-db.c
>>> @@ -1464,7 +1464,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>>> }
>>>
>>> /* Guard against invalid access if offset equals to value length */
>>> - value = offset == attrib->value_len ? NULL : &attrib->value[offset];
>>> + value = attrib->value_len - offset ? &attrib->value[offset] : NULL;
>>
>> Im not following why you are changing read as well here? Btw, all this
>> does is reverse the check so Im not sure how it would make any
>> difference.
>>
Yeah, I guess this one isn't necessary at all, it does the same thing as before.
>>>
>>> func(attrib, 0, value, attrib->value_len - offset, user_data);
>>>
>>> @@ -1557,15 +1557,14 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>>> void *buf;
>>>
>>> buf = realloc(attrib->value, len + offset);
>>> - if (!buf)
>>> + if (!buf && len + offset > 0)
>>> return false;
>>
>> We should not reach realloc in this case, Id say we return if len == 0
>> since it should really be a nop, for clearing there already exists
>> gatt_db_attribute_reset and I don't think it worth complicating more
>> the code bellow.
>>
We reach this code path since we clear the cached value in the writes,
but I like your approach better.
>>> - attrib->value = buf;
>>> -
>>> /* Init data in the first allocation */
>>> - if (!attrib->value_len)
>>> - memset(attrib->value, 0, offset);
>>> + if (!attrib->value)
>>> + memset(buf, 0, offset);
>>>
>>> + attrib->value = buf;
>>> attrib->value_len = len + offset;
>>> }
>>>
>>> --
>>> 2.2.0.rc0.207.ga3a616c
>
> I went ahead applied a fix.
>
Thanks, I tested your patch and fixes the issue.
>
> --
> Luiz Augusto von Dentz
Thanks!
Arman
Hi Arman,
On Thu, Feb 19, 2015 at 2:18 PM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Arman,
>
> On Thu, Feb 19, 2015 at 2:12 AM, Arman Uguray <[email protected]> wrote:
>> gatt_db_attribute_write returns false if realloc returns NULL in the
>> case where there is no callback. If the write is performed with a 0
>> length value and 0 offset, realloc will return NULL according to its
>> specification (as it will practically act as 'free'). This patch fixes
>> the code so that this case isn't treated as an error.
>> ---
>> src/shared/gatt-db.c | 11 +++++------
>> 1 file changed, 5 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
>> index 3d2e690..b331f54 100644
>> --- a/src/shared/gatt-db.c
>> +++ b/src/shared/gatt-db.c
>> @@ -1464,7 +1464,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
>> }
>>
>> /* Guard against invalid access if offset equals to value length */
>> - value = offset == attrib->value_len ? NULL : &attrib->value[offset];
>> + value = attrib->value_len - offset ? &attrib->value[offset] : NULL;
>
> Im not following why you are changing read as well here? Btw, all this
> does is reverse the check so Im not sure how it would make any
> difference.
>
>>
>> func(attrib, 0, value, attrib->value_len - offset, user_data);
>>
>> @@ -1557,15 +1557,14 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
>> void *buf;
>>
>> buf = realloc(attrib->value, len + offset);
>> - if (!buf)
>> + if (!buf && len + offset > 0)
>> return false;
>
> We should not reach realloc in this case, Id say we return if len == 0
> since it should really be a nop, for clearing there already exists
> gatt_db_attribute_reset and I don't think it worth complicating more
> the code bellow.
>
>> - attrib->value = buf;
>> -
>> /* Init data in the first allocation */
>> - if (!attrib->value_len)
>> - memset(attrib->value, 0, offset);
>> + if (!attrib->value)
>> + memset(buf, 0, offset);
>>
>> + attrib->value = buf;
>> attrib->value_len = len + offset;
>> }
>>
>> --
>> 2.2.0.rc0.207.ga3a616c
I went ahead applied a fix.
--
Luiz Augusto von Dentz
Hi Arman,
On Thu, Feb 19, 2015 at 2:12 AM, Arman Uguray <[email protected]> wrote:
> This patch fixes a bug where a CCC read on the local GATT server always
> returned a 0-length value.
> ---
> src/gatt-database.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 5e83ce1..e876141 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -495,7 +495,7 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib,
> goto done;
> }
>
> - len -= offset;
> + len = 2 - offset;
> value = len ? &ccc->value[offset] : NULL;
>
> done:
> @@ -517,7 +517,7 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
>
> handle = gatt_db_attribute_get_handle(attrib);
>
> - DBG("CCC read called for handle: 0x%04x", handle);
> + DBG("CCC write called for handle: 0x%04x", handle);
>
> if (!value || len != 2) {
> ecode = BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN;
> --
> 2.2.0.rc0.207.ga3a616c
Applied, thanks.
--
Luiz Augusto von Dentz
Hi Arman,
On Thu, Feb 19, 2015 at 2:12 AM, Arman Uguray <[email protected]> wrote:
> gatt_db_attribute_write returns false if realloc returns NULL in the
> case where there is no callback. If the write is performed with a 0
> length value and 0 offset, realloc will return NULL according to its
> specification (as it will practically act as 'free'). This patch fixes
> the code so that this case isn't treated as an error.
> ---
> src/shared/gatt-db.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 3d2e690..b331f54 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -1464,7 +1464,7 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> }
>
> /* Guard against invalid access if offset equals to value length */
> - value = offset == attrib->value_len ? NULL : &attrib->value[offset];
> + value = attrib->value_len - offset ? &attrib->value[offset] : NULL;
Im not following why you are changing read as well here? Btw, all this
does is reverse the check so Im not sure how it would make any
difference.
>
> func(attrib, 0, value, attrib->value_len - offset, user_data);
>
> @@ -1557,15 +1557,14 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> void *buf;
>
> buf = realloc(attrib->value, len + offset);
> - if (!buf)
> + if (!buf && len + offset > 0)
> return false;
We should not reach realloc in this case, Id say we return if len == 0
since it should really be a nop, for clearing there already exists
gatt_db_attribute_reset and I don't think it worth complicating more
the code bellow.
> - attrib->value = buf;
> -
> /* Init data in the first allocation */
> - if (!attrib->value_len)
> - memset(attrib->value, 0, offset);
> + if (!attrib->value)
> + memset(buf, 0, offset);
>
> + attrib->value = buf;
> attrib->value_len = len + offset;
> }
>
> --
> 2.2.0.rc0.207.ga3a616c
>
--
Luiz Augusto von Dentz
This patch fixes a bug where a CCC read on the local GATT server always
returned a 0-length value.
---
src/gatt-database.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index 5e83ce1..e876141 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -495,7 +495,7 @@ static void gatt_ccc_read_cb(struct gatt_db_attribute *attrib,
goto done;
}
- len -= offset;
+ len = 2 - offset;
value = len ? &ccc->value[offset] : NULL;
done:
@@ -517,7 +517,7 @@ static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
handle = gatt_db_attribute_get_handle(attrib);
- DBG("CCC read called for handle: 0x%04x", handle);
+ DBG("CCC write called for handle: 0x%04x", handle);
if (!value || len != 2) {
ecode = BT_ATT_ERROR_INVALID_ATTRIBUTE_VALUE_LEN;
--
2.2.0.rc0.207.ga3a616c