Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1424304754-11874-1-git-send-email-armansito@chromium.org> Date: Thu, 19 Feb 2015 14:42:25 +0200 Message-ID: Subject: Re: [PATCH BlueZ 1/2] shared/gatt-db: Don't fail if realloc returns NULL From: Luiz Augusto von Dentz To: Arman Uguray Cc: "linux-bluetooth@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Arman, On Thu, Feb 19, 2015 at 2:18 PM, Luiz Augusto von Dentz wrote: > Hi Arman, > > On Thu, Feb 19, 2015 at 2:12 AM, Arman Uguray 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