* [PATCH] Honor offset when reading external chrc. @ 2015-03-16 15:11 Andrejs Hanins 2015-03-16 15:23 ` Arman Uguray 0 siblings, 1 reply; 3+ messages in thread From: Andrejs Hanins @ 2015-03-16 15:11 UTC (permalink / raw) To: linux-bluetooth@vger.kernel.org; +Cc: Arman Uguray Honor offset when reading external characteristics via D-Bus and respond with properly chunked data. --- src/gatt-database.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/gatt-database.c b/src/gatt-database.c index e07c70f..3ae2f62 100644 --- a/src/gatt-database.c +++ b/src/gatt-database.c @@ -119,6 +119,7 @@ struct pending_op { struct gatt_db_attribute *attrib; struct queue *owner_queue; struct iovec data; + uint16_t offset; }; struct device_state { @@ -1446,6 +1447,19 @@ static void read_reply_cb(DBusMessage *message, void *user_data) goto done; } + if (op->offset > len) { + ecode = BT_ATT_ERROR_INVALID_OFFSET; + value = NULL; + len = 0; + goto done; + } else if (op->offset == len) { + value = NULL; + len = 0; + } else { + len -= op->offset; + value = &value[op->offset]; + } + /* Truncate the value if it's too large */ len = MIN(BT_ATT_MAX_VALUE_LEN, len); value = len ? value : NULL; @@ -1466,7 +1480,7 @@ static void pending_op_free(void *data) static struct pending_op *pending_read_new(struct queue *owner_queue, struct gatt_db_attribute *attrib, - unsigned int id) + unsigned int id, uint16_t offset) { struct pending_op *op; @@ -1477,6 +1491,7 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, op->owner_queue = owner_queue; op->attrib = attrib; op->id = id; + op->offset = offset; queue_push_tail(owner_queue, op); return op; @@ -1484,12 +1499,12 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, static void send_read(struct gatt_db_attribute *attrib, GDBusProxy *proxy, struct queue *owner_queue, - unsigned int id) + unsigned int id, uint16_t offset) { struct pending_op *op; uint8_t ecode = BT_ATT_ERROR_UNLIKELY; - op = pending_read_new(owner_queue, attrib, id); + op = pending_read_new(owner_queue, attrib, id, offset); if (!op) { error("Failed to allocate memory for pending read call"); ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES; @@ -1785,7 +1800,7 @@ static void desc_read_cb(struct gatt_db_attribute *attrib, return; } - send_read(attrib, desc->proxy, desc->pending_reads, id); + send_read(attrib, desc->proxy, desc->pending_reads, id, offset); } static void desc_write_cb(struct gatt_db_attribute *attrib, @@ -1843,7 +1858,7 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib, return; } - send_read(attrib, chrc->proxy, chrc->pending_reads, id); + send_read(attrib, chrc->proxy, chrc->pending_reads, id, offset); } static void chrc_write_cb(struct gatt_db_attribute *attrib, -- 1.9.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Honor offset when reading external chrc. 2015-03-16 15:11 [PATCH] Honor offset when reading external chrc Andrejs Hanins @ 2015-03-16 15:23 ` Arman Uguray 2015-03-16 16:04 ` [PATCH] core/gatt-database: " Andrejs Hanins 0 siblings, 1 reply; 3+ messages in thread From: Arman Uguray @ 2015-03-16 15:23 UTC (permalink / raw) To: Andrejs Hanins; +Cc: linux-bluetooth@vger.kernel.org, Arman Uguray Hi Andrejs, > On Mon, Mar 16, 2015 at 8:11 AM, Andrejs Hanins <andrejs.hanins@ubnt.com> wrote: > Honor offset when reading external characteristics via D-Bus and respond with properly chunked data. > --- The subject should contain "core/gatt: " as its prefix, at least that's how we've been organizing patches to different parts of the code. Also, a similar problem may also exist with WriteValue but we may not be able to fully address it without an API change. > src/gatt-database.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/src/gatt-database.c b/src/gatt-database.c > index e07c70f..3ae2f62 100644 > --- a/src/gatt-database.c > +++ b/src/gatt-database.c > @@ -119,6 +119,7 @@ struct pending_op { > struct gatt_db_attribute *attrib; > struct queue *owner_queue; > struct iovec data; > + uint16_t offset; > }; > > struct device_state { > @@ -1446,6 +1447,19 @@ static void read_reply_cb(DBusMessage *message, void *user_data) > goto done; > } > > + if (op->offset > len) { > + ecode = BT_ATT_ERROR_INVALID_OFFSET; > + value = NULL; > + len = 0; > + goto done; > + } else if (op->offset == len) { No need for the else statement since the previous if-body ends in a goto. > + value = NULL; > + len = 0; > + } else { > + len -= op->offset; > + value = &value[op->offset]; > + } The above code is a bit redundant since we already do the NULL assignment below. Basically, all you should need to do is this: if (op->offset > len) { ecode = BT_ATT_ERROR_INVALID_OFFSET; value = NULL; len = 0; goto done; } len -= op->offset; /* Truncate the value if it's too large */ len = MIN(BT_ATT_MAX_VALUE_LEN, len); value = len ? &value[op->offset] : NULL; If len == op->offset, len will end up with 0 as its value and since you already handle the offset being larger than len above, this won't cause an overflow. > + > /* Truncate the value if it's too large */ > len = MIN(BT_ATT_MAX_VALUE_LEN, len); > value = len ? value : NULL; > @@ -1466,7 +1480,7 @@ static void pending_op_free(void *data) > > static struct pending_op *pending_read_new(struct queue *owner_queue, > struct gatt_db_attribute *attrib, > - unsigned int id) > + unsigned int id, uint16_t offset) > { > struct pending_op *op; > > @@ -1477,6 +1491,7 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, > op->owner_queue = owner_queue; > op->attrib = attrib; > op->id = id; > + op->offset = offset; > queue_push_tail(owner_queue, op); > > return op; > @@ -1484,12 +1499,12 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, > > static void send_read(struct gatt_db_attribute *attrib, GDBusProxy *proxy, > struct queue *owner_queue, > - unsigned int id) > + unsigned int id, uint16_t offset) > { > struct pending_op *op; > uint8_t ecode = BT_ATT_ERROR_UNLIKELY; > > - op = pending_read_new(owner_queue, attrib, id); > + op = pending_read_new(owner_queue, attrib, id, offset); > if (!op) { > error("Failed to allocate memory for pending read call"); > ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES; > @@ -1785,7 +1800,7 @@ static void desc_read_cb(struct gatt_db_attribute *attrib, > return; > } > > - send_read(attrib, desc->proxy, desc->pending_reads, id); > + send_read(attrib, desc->proxy, desc->pending_reads, id, offset); > } > > static void desc_write_cb(struct gatt_db_attribute *attrib, > @@ -1843,7 +1858,7 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib, > return; > } > > - send_read(attrib, chrc->proxy, chrc->pending_reads, id); > + send_read(attrib, chrc->proxy, chrc->pending_reads, id, offset); > } > > static void chrc_write_cb(struct gatt_db_attribute *attrib, > -- > 1.9.1 > Thanks, Arman ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] core/gatt-database: Honor offset when reading external chrc. 2015-03-16 15:23 ` Arman Uguray @ 2015-03-16 16:04 ` Andrejs Hanins 0 siblings, 0 replies; 3+ messages in thread From: Andrejs Hanins @ 2015-03-16 16:04 UTC (permalink / raw) To: Arman Uguray; +Cc: linux-bluetooth@vger.kernel.org Hi Arman, On 2015.03.16. 17:23, Arman Uguray wrote: > Hi Andrejs, > >> On Mon, Mar 16, 2015 at 8:11 AM, Andrejs Hanins <andrejs.hanins@ubnt.com> wrote: >> Honor offset when reading external characteristics via D-Bus and respond with properly chunked data. >> --- > > The subject should contain "core/gatt: " as its prefix, at least > that's how we've been organizing patches to different parts of the > code. > > Also, a similar problem may also exist with WriteValue but we may not > be able to fully address it without an API change. You are right. I just made a test by writing a big chunk of data (more than negotiated ATT_MTU) from the mobile phone and D-Bus method "WriteValue" is called twice - for the first and second pieces of data. But the data itself looks to be correct, i.e. both chunks concatenated together give the original data. IMO, the bluez daemon should do the assembly of long write chunks and call D-Bus "WriteValue" only once providing the whole write request as initiated by the other side. This way, the API will continue to be simple. > >> src/gatt-database.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/src/gatt-database.c b/src/gatt-database.c >> index e07c70f..3ae2f62 100644 >> --- a/src/gatt-database.c >> +++ b/src/gatt-database.c >> @@ -119,6 +119,7 @@ struct pending_op { >> struct gatt_db_attribute *attrib; >> struct queue *owner_queue; >> struct iovec data; >> + uint16_t offset; >> }; >> >> struct device_state { >> @@ -1446,6 +1447,19 @@ static void read_reply_cb(DBusMessage *message, void *user_data) >> goto done; >> } >> >> + if (op->offset > len) { >> + ecode = BT_ATT_ERROR_INVALID_OFFSET; >> + value = NULL; >> + len = 0; >> + goto done; >> + } else if (op->offset == len) { > > No need for the else statement since the previous if-body ends in a goto. > >> + value = NULL; >> + len = 0; >> + } else { >> + len -= op->offset; >> + value = &value[op->offset]; >> + } > > The above code is a bit redundant since we already do the NULL > assignment below. Basically, all you should need to do is this: > > if (op->offset > len) { > ecode = BT_ATT_ERROR_INVALID_OFFSET; > value = NULL; > len = 0; > goto done; > } > > len -= op->offset; > > /* Truncate the value if it's too large */ > len = MIN(BT_ATT_MAX_VALUE_LEN, len); > value = len ? &value[op->offset] : NULL; > > If len == op->offset, len will end up with 0 as its value and since > you already handle the offset being larger than len above, this won't > cause an overflow. Noted. Thank you! > >> + >> /* Truncate the value if it's too large */ >> len = MIN(BT_ATT_MAX_VALUE_LEN, len); >> value = len ? value : NULL; >> @@ -1466,7 +1480,7 @@ static void pending_op_free(void *data) >> >> static struct pending_op *pending_read_new(struct queue *owner_queue, >> struct gatt_db_attribute *attrib, >> - unsigned int id) >> + unsigned int id, uint16_t offset) >> { >> struct pending_op *op; >> >> @@ -1477,6 +1491,7 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, >> op->owner_queue = owner_queue; >> op->attrib = attrib; >> op->id = id; >> + op->offset = offset; >> queue_push_tail(owner_queue, op); >> >> return op; >> @@ -1484,12 +1499,12 @@ static struct pending_op *pending_read_new(struct queue *owner_queue, >> >> static void send_read(struct gatt_db_attribute *attrib, GDBusProxy *proxy, >> struct queue *owner_queue, >> - unsigned int id) >> + unsigned int id, uint16_t offset) >> { >> struct pending_op *op; >> uint8_t ecode = BT_ATT_ERROR_UNLIKELY; >> >> - op = pending_read_new(owner_queue, attrib, id); >> + op = pending_read_new(owner_queue, attrib, id, offset); >> if (!op) { >> error("Failed to allocate memory for pending read call"); >> ecode = BT_ATT_ERROR_INSUFFICIENT_RESOURCES; >> @@ -1785,7 +1800,7 @@ static void desc_read_cb(struct gatt_db_attribute *attrib, >> return; >> } >> >> - send_read(attrib, desc->proxy, desc->pending_reads, id); >> + send_read(attrib, desc->proxy, desc->pending_reads, id, offset); >> } >> >> static void desc_write_cb(struct gatt_db_attribute *attrib, >> @@ -1843,7 +1858,7 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib, >> return; >> } >> >> - send_read(attrib, chrc->proxy, chrc->pending_reads, id); >> + send_read(attrib, chrc->proxy, chrc->pending_reads, id, offset); >> } >> >> static void chrc_write_cb(struct gatt_db_attribute *attrib, >> -- >> 1.9.1 >> > > Thanks, > Arman > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-03-16 16:04 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-16 15:11 [PATCH] Honor offset when reading external chrc Andrejs Hanins 2015-03-16 15:23 ` Arman Uguray 2015-03-16 16:04 ` [PATCH] core/gatt-database: " Andrejs Hanins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox