2014-12-03 14:08:51

by Andrei Emeltchenko

Subject: [PATCH 1/3] unit/gatt: Fix using possible negative return value

From: Andrei Emeltchenko <[email protected]>

---
unit/test-gatt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/unit/test-gatt.c b/unit/test-gatt.c
index 03a66b9..f20da91 100644
--- a/unit/test-gatt.c
+++ b/unit/test-gatt.c
@@ -648,11 +648,11 @@ static void test_server(gconstpointer data)

len = write(context->fd, pdu.data, pdu.size);

+ g_assert_cmpint(len, ==, pdu.size);
+
if (g_test_verbose())
util_hexdump('<', pdu.data, len, test_debug, "GATT: ");

- g_assert_cmpint(len, ==, pdu.size);
-
execute_context(context);
}

--
1.9.1

2014-12-04 12:34:06

by Luiz Augusto von Dentz

Subject: Re: [PATCH 1/3] unit/gatt: Fix using possible negative return value

Hi Andrei,

On Thu, Dec 4, 2014 at 11:19 AM, Luiz Augusto von Dentz
<[email protected]> wrote:
> Hi Andrei,
>
> On Thu, Dec 4, 2014 at 10:15 AM, Andrei Emeltchenko
> <[email protected]> wrote:
>> Hi Luiz,
>>
>> On Wed, Dec 03, 2014 at 08:35:41PM +0200, Luiz Augusto von Dentz wrote:
>>> Hi Andrei,
>>>
>>> On Wed, Dec 3, 2014 at 4:08 PM, Andrei Emeltchenko
>>> <[email protected]> wrote:
>>> > From: Andrei Emeltchenko <[email protected]>
>>> >
>>> > ---
>>> > unit/test-gatt.c | 4 ++--
>>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>>> >
>>> > diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>>> > index 03a66b9..f20da91 100644
>>> > --- a/unit/test-gatt.c
>>> > +++ b/unit/test-gatt.c
>>> > @@ -648,11 +648,11 @@ static void test_server(gconstpointer data)
>>> >
>>> > len = write(context->fd, pdu.data, pdu.size);
>>> >
>>> > + g_assert_cmpint(len, ==, pdu.size);
>>> > +
>>> > if (g_test_verbose())
>>> > util_hexdump('<', pdu.data, len, test_debug, "GATT: ");
>>> >
>>> > - g_assert_cmpint(len, ==, pdu.size);
>>> > -
>>> > execute_context(context);
>>> > }
>>> >
>>> > --
>>> > 1.9.1
>>>
>>> What changes does this make?
>>
>> You do not use negative len in case of error in util_hexdump()
>
> Well then I guess this should be part of the path description.

I went ahead and fixed the commit message and applied them, thanks.


--
Luiz Augusto von Dentz

2014-12-04 09:19:58

by Luiz Augusto von Dentz

Subject: Re: [PATCH 1/3] unit/gatt: Fix using possible negative return value

Hi Andrei,

On Thu, Dec 4, 2014 at 10:15 AM, Andrei Emeltchenko
<[email protected]> wrote:
> Hi Luiz,
>
> On Wed, Dec 03, 2014 at 08:35:41PM +0200, Luiz Augusto von Dentz wrote:
>> Hi Andrei,
>>
>> On Wed, Dec 3, 2014 at 4:08 PM, Andrei Emeltchenko
>> <[email protected]> wrote:
>> > From: Andrei Emeltchenko <[email protected]>
>> >
>> > ---
>> > unit/test-gatt.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/unit/test-gatt.c b/unit/test-gatt.c
>> > index 03a66b9..f20da91 100644
>> > --- a/unit/test-gatt.c
>> > +++ b/unit/test-gatt.c
>> > @@ -648,11 +648,11 @@ static void test_server(gconstpointer data)
>> >
>> > len = write(context->fd, pdu.data, pdu.size);
>> >
>> > + g_assert_cmpint(len, ==, pdu.size);
>> > +
>> > if (g_test_verbose())
>> > util_hexdump('<', pdu.data, len, test_debug, "GATT: ");
>> >
>> > - g_assert_cmpint(len, ==, pdu.size);
>> > -
>> > execute_context(context);
>> > }
>> >
>> > --
>> > 1.9.1
>>
>> What changes does this make?
>
> You do not use negative len in case of error in util_hexdump()

Well then I guess this should be part of the path description.

--
Luiz Augusto von Dentz

2014-12-04 08:15:59

by Andrei Emeltchenko

Subject: Re: [PATCH 1/3] unit/gatt: Fix using possible negative return value

Hi Luiz,

On Wed, Dec 03, 2014 at 08:35:41PM +0200, Luiz Augusto von Dentz wrote:
> Hi Andrei,
>
> On Wed, Dec 3, 2014 at 4:08 PM, Andrei Emeltchenko
> <[email protected]> wrote:
> > From: Andrei Emeltchenko <[email protected]>
> >
> > ---
> > unit/test-gatt.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> > index 03a66b9..f20da91 100644
> > --- a/unit/test-gatt.c
> > +++ b/unit/test-gatt.c
> > @@ -648,11 +648,11 @@ static void test_server(gconstpointer data)
> >
> > len = write(context->fd, pdu.data, pdu.size);
> >
> > + g_assert_cmpint(len, ==, pdu.size);
> > +
> > if (g_test_verbose())
> > util_hexdump('<', pdu.data, len, test_debug, "GATT: ");
> >
> > - g_assert_cmpint(len, ==, pdu.size);
> > -
> > execute_context(context);
> > }
> >
> > --
> > 1.9.1
>
> What changes does this make?

You do not use negative len in case of error in util_hexdump()

Best regards
Andrei Emeltchenko

2014-12-03 21:32:07

by Luiz Augusto von Dentz

Subject: Re: [PATCH 3/3] obexd: Fix comparing array to 0

Hi Andrei,

On Wed, Dec 3, 2014 at 4:08 PM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Comparing an arrays to 0 is not useful.

I guess you mean this should never fail so it dead code in practice
since the array will never be NULL, we should probably fix the message
then.

> ---
> obexd/client/pbap.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
> index 57632b4..28203a8 100644
> --- a/obexd/client/pbap.c
> +++ b/obexd/client/pbap.c
> @@ -1111,9 +1111,6 @@ static gboolean get_databaseid(const GDBusPropertyTable *property,
> char value[33];
> const char *pvalue = value;
>
> - if (!pbap->databaseid)
> - return FALSE;
> -
> if (u128_to_string(pbap->databaseid, value, sizeof(value)) < 0)
> return FALSE;
>
> @@ -1137,9 +1134,6 @@ static gboolean get_primary(const GDBusPropertyTable *property,
> char value[33];
> const char *pvalue = value;
>
> - if (!pbap->primary)
> - return FALSE;
> -
> if (u128_to_string(pbap->primary, value, sizeof(value)) < 0)
> return FALSE;
>
> @@ -1155,9 +1149,6 @@ static gboolean get_secondary(const GDBusPropertyTable *property,
> char value[33];
> const char *pvalue = value;
>
> - if (!pbap->secondary)
> - return FALSE;
> -
> if (u128_to_string(pbap->secondary, value, sizeof(value)) < 0)
> return FALSE;
>
> --
> 1.9.1
>
> --
> 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

2014-12-03 14:08:53

by Andrei Emeltchenko

Subject: [PATCH 3/3] obexd: Fix comparing array to 0

From: Andrei Emeltchenko <[email protected]>

Comparing an arrays to 0 is not useful.
---
obexd/client/pbap.c | 9 ---------
1 file changed, 9 deletions(-)

diff --git a/obexd/client/pbap.c b/obexd/client/pbap.c
index 57632b4..28203a8 100644
--- a/obexd/client/pbap.c
+++ b/obexd/client/pbap.c
@@ -1111,9 +1111,6 @@ static gboolean get_databaseid(const GDBusPropertyTable *property,
char value[33];
const char *pvalue = value;

- if (!pbap->databaseid)
- return FALSE;
-
if (u128_to_string(pbap->databaseid, value, sizeof(value)) < 0)
return FALSE;

@@ -1137,9 +1134,6 @@ static gboolean get_primary(const GDBusPropertyTable *property,
char value[33];
const char *pvalue = value;

- if (!pbap->primary)
- return FALSE;
-
if (u128_to_string(pbap->primary, value, sizeof(value)) < 0)
return FALSE;

@@ -1155,9 +1149,6 @@ static gboolean get_secondary(const GDBusPropertyTable *property,
char value[33];
const char *pvalue = value;

- if (!pbap->secondary)
- return FALSE;
-
if (u128_to_string(pbap->secondary, value, sizeof(value)) < 0)
return FALSE;

--
1.9.1

2014-12-03 18:35:41

by Luiz Augusto von Dentz

Subject: Re: [PATCH 1/3] unit/gatt: Fix using possible negative return value

Hi Andrei,

On Wed, Dec 3, 2014 at 4:08 PM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> ---
> unit/test-gatt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/unit/test-gatt.c b/unit/test-gatt.c
> index 03a66b9..f20da91 100644
> --- a/unit/test-gatt.c
> +++ b/unit/test-gatt.c
> @@ -648,11 +648,11 @@ static void test_server(gconstpointer data)
>
> len = write(context->fd, pdu.data, pdu.size);
>
> + g_assert_cmpint(len, ==, pdu.size);
> +
> if (g_test_verbose())
> util_hexdump('<', pdu.data, len, test_debug, "GATT: ");
>
> - g_assert_cmpint(len, ==, pdu.size);
> -
> execute_context(context);
> }
>
> --
> 1.9.1

What changes does this make?


--
Luiz Augusto von Dentz

2014-12-03 14:08:52

by Andrei Emeltchenko

Subject: [PATCH 2/3] obexd: Fix memory leak not freeing desc

From: Andrei Emeltchenko <[email protected]>

descs->data might be NULL while is descs not. Follow scheme in
src/profile.c (in get_profile_version() function).
---
obexd/client/bluetooth.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/obexd/client/bluetooth.c b/obexd/client/bluetooth.c
index 589d7a5..8e09d03 100644
--- a/obexd/client/bluetooth.c
+++ b/obexd/client/bluetooth.c
@@ -486,6 +486,7 @@ static const void *bluetooth_getattribute(guint id, int attribute_id)
/* Read version since UUID is already known */
if (attribute_id == SDP_ATTR_PFILE_DESC_LIST) {
sdp_list_t *descs;
+ void *ret = NULL;

if (sdp_get_profile_descs(session->sdp_record,
&descs) < 0)
@@ -493,14 +494,12 @@ static const void *bluetooth_getattribute(guint id, int attribute_id)

if (descs && descs->data) {
sdp_profile_desc_t *desc = descs->data;
- uint16_t version = desc->version;
-
- sdp_list_free(descs, free);
-
- return GINT_TO_POINTER(version);
+ ret = GINT_TO_POINTER(desc->version);
}

- return NULL;
+ sdp_list_free(descs, free);
+
+ return ret;
}

data = sdp_data_get(session->sdp_record, attribute_id);
--
1.9.1