* [PATCH v1 0/3] Fix memory issues and trivial code cleaup
@ 2015-01-27 8:53 Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory Gowtham Anandha Babu
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Gowtham Anandha Babu @ 2015-01-27 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: bharat.panda, cpgs, Gowtham Anandha Babu
In gatt-client and gatt-helpers, the variables are accessed
even after it is freed. Making the unref at appropriate
places solved the issues.
In tools, removed dead code warnings.
*v1: Rebased.
Gowtham Anandha Babu (3):
shared/gatt-helpers: Fix usage of freed memory
shared/gatt-client: Fix usage of freed memory
tools/hciattach_ath3k: Remove dead code warnings
src/shared/gatt-client.c | 16 +++++++++-------
src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
tools/hciattach_ath3k.c | 11 +----------
3 files changed, 26 insertions(+), 26 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
2015-01-27 8:53 [PATCH v1 0/3] Fix memory issues and trivial code cleaup Gowtham Anandha Babu
@ 2015-01-27 8:53 ` Gowtham Anandha Babu
2015-01-28 13:07 ` Luiz Augusto von Dentz
2015-01-27 8:53 ` [PATCH v1 2/3] shared/gatt-client: " Gowtham Anandha Babu
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Gowtham Anandha Babu @ 2015-01-27 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: bharat.panda, cpgs, Gowtham Anandha Babu
src/shared/gatt-helpers.c:709:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:780:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:998:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:1020:32: warning: Use of memory after it is freed
op->callback(false, 0, NULL, data->op->user_data);
^~~~~~~~
src/shared/gatt-helpers.c:1112:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:1226:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:1332:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
src/shared/gatt-helpers.c:1452:6: warning: Use of memory after it is freed
if (op->callback)
^~~~~~~~~~~~
---
src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
index 3864ed0..c9b24cc 100644
--- a/src/shared/gatt-helpers.c
+++ b/src/shared/gatt-helpers.c
@@ -686,7 +686,6 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -708,6 +707,8 @@ success:
done:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);
+
+ discovery_op_unref(op);
}
static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
@@ -766,7 +767,6 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -779,6 +779,8 @@ success:
done:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);
+
+ discovery_op_unref(op);
}
static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
@@ -977,7 +979,6 @@ static void read_included_cb(uint8_t opcode, const void *pdu,
discovery_op_ref(op), discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -997,6 +998,8 @@ static void read_included_cb(uint8_t opcode, const void *pdu,
done:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);
+
+ discovery_op_unref(op);
}
static void read_included(struct read_incl_data *data)
@@ -1014,10 +1017,10 @@ static void read_included(struct read_incl_data *data)
read_included_unref))
return;
- read_included_unref(data);
-
if (op->callback)
op->callback(false, 0, NULL, data->op->user_data);
+
+ read_included_unref(data);
}
static void discover_included_cb(uint8_t opcode, const void *pdu,
@@ -1099,7 +1102,6 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto failed;
}
@@ -1111,6 +1113,8 @@ done:
failed:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);
+
+ discovery_op_unref(op);
}
bool bt_gatt_discover_included_services(struct bt_att *att,
@@ -1213,7 +1217,6 @@ static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -1226,6 +1229,8 @@ done:
if (op->callback)
op->callback(success, att_ecode, final_result,
op->user_data);
+
+ discovery_op_unref(op);
}
bool bt_gatt_discover_characteristics(struct bt_att *att,
@@ -1321,7 +1326,6 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -1332,6 +1336,8 @@ done:
if (op->callback)
op->callback(success, att_ecode, success ? op->result_head :
NULL, op->user_data);
+
+ discovery_op_unref(op);
}
bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
@@ -1439,7 +1445,6 @@ static void discover_descs_cb(uint8_t opcode, const void *pdu,
discovery_op_unref))
return;
- discovery_op_unref(op);
success = false;
goto done;
}
@@ -1451,6 +1456,8 @@ success:
done:
if (op->callback)
op->callback(success, att_ecode, final_result, op->user_data);
+
+ discovery_op_unref(op);
}
bool bt_gatt_discover_descriptors(struct bt_att *att,
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v1 2/3] shared/gatt-client: Fix usage of freed memory
2015-01-27 8:53 [PATCH v1 0/3] Fix memory issues and trivial code cleaup Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory Gowtham Anandha Babu
@ 2015-01-27 8:53 ` Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 3/3] tools/hciattach_ath3k: Remove dead code warnings Gowtham Anandha Babu
2015-01-27 13:39 ` [PATCH v1 0/3] Fix memory issues and trivial code cleaup Luiz Augusto von Dentz
3 siblings, 0 replies; 8+ messages in thread
From: Gowtham Anandha Babu @ 2015-01-27 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: bharat.panda, cpgs, Gowtham Anandha Babu
src/shared/gatt-client.c:534:14: warning: Use of memory after it is freed
op->success = false;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:689:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:790:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:882:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:950:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
---
src/shared/gatt-client.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/src/shared/gatt-client.c b/src/shared/gatt-client.c
index 1acd34f..12c9b2f 100644
--- a/src/shared/gatt-client.c
+++ b/src/shared/gatt-client.c
@@ -511,7 +511,6 @@ next:
util_debug(client->debug_callback, client->debug_data,
"Failed to start characteristic discovery");
- discovery_op_unref(op);
goto failed;
}
@@ -528,11 +527,11 @@ next:
util_debug(client->debug_callback, client->debug_data,
"Failed to start included discovery");
- discovery_op_unref(op);
failed:
op->success = false;
op->complete_func(op, false, att_ecode);
+ discovery_op_unref(op);
}
struct chrc {
@@ -587,8 +586,11 @@ static bool discover_descs(struct discovery_op *op, bool *discovering)
util_debug(client->debug_callback, client->debug_data,
"Failed to start descriptor discovery");
- discovery_op_unref(op);
+ /*
+ * discovery_op_unref done in discover_chrcs_cb
+ * and discover_descs_cb functions
+ */
goto failed;
}
@@ -680,7 +682,6 @@ next:
util_debug(client->debug_callback, client->debug_data,
"Failed to start characteristic discovery");
- discovery_op_unref(op);
failed:
success = false;
@@ -688,6 +689,7 @@ failed:
done:
op->success = success;
op->complete_func(op, success, att_ecode);
+ discovery_op_unref(op);
}
static void discover_chrcs_cb(bool success, uint8_t att_ecode,
@@ -781,7 +783,6 @@ next:
util_debug(client->debug_callback, client->debug_data,
"Failed to start characteristic discovery");
- discovery_op_unref(op);
failed:
success = false;
@@ -789,6 +790,7 @@ failed:
done:
op->success = success;
op->complete_func(op, success, att_ecode);
+ discovery_op_unref(op);
}
static void discover_secondary_cb(bool success, uint8_t att_ecode,
@@ -876,11 +878,11 @@ next:
util_debug(client->debug_callback, client->debug_data,
"Failed to start included services discovery");
- discovery_op_unref(op);
done:
op->success = success;
op->complete_func(op, success, att_ecode);
+ discovery_op_unref(op);
}
static void discover_primary_cb(bool success, uint8_t att_ecode,
@@ -943,12 +945,12 @@ static void discover_primary_cb(bool success, uint8_t att_ecode,
util_debug(client->debug_callback, client->debug_data,
"Failed to start secondary service discovery");
- discovery_op_unref(op);
success = false;
done:
op->success = success;
op->complete_func(op, success, att_ecode);
+ discovery_op_unref(op);
}
static void notify_client_ready(struct bt_gatt_client *client, bool success,
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v1 3/3] tools/hciattach_ath3k: Remove dead code warnings
2015-01-27 8:53 [PATCH v1 0/3] Fix memory issues and trivial code cleaup Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 2/3] shared/gatt-client: " Gowtham Anandha Babu
@ 2015-01-27 8:53 ` Gowtham Anandha Babu
2015-01-27 13:39 ` [PATCH v1 0/3] Fix memory issues and trivial code cleaup Luiz Augusto von Dentz
3 siblings, 0 replies; 8+ messages in thread
From: Gowtham Anandha Babu @ 2015-01-27 8:53 UTC (permalink / raw)
To: linux-bluetooth; +Cc: bharat.panda, cpgs, Gowtham Anandha Babu
tools/hciattach_ath3k.c:848:3: warning: Value stored to 'err' is never read
err = 0;
^ ~
tools/hciattach_ath3k.c:852:3: warning: Value stored to 'err' is never read
err = 0;
^ ~
---
tools/hciattach_ath3k.c | 11 +----------
1 file changed, 1 insertion(+), 10 deletions(-)
diff --git a/tools/hciattach_ath3k.c b/tools/hciattach_ath3k.c
index 23208c6..d31732e 100644
--- a/tools/hciattach_ath3k.c
+++ b/tools/hciattach_ath3k.c
@@ -840,17 +840,8 @@ static int ath_ps_download(int fd)
goto download_cmplete;
}
- /*
- * It is not necessary that Patch file be available,
- * continue with PS Operations if patch file is not available.
- */
- if (patch_file[0] == '\0')
- err = 0;
-
stream = fopen(patch_file, "r");
- if (!stream)
- err = 0;
- else {
+ if(stream) {
patch_count = ps_patch_download(fd, stream);
fclose(stream);
--
1.9.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v1 0/3] Fix memory issues and trivial code cleaup
2015-01-27 8:53 [PATCH v1 0/3] Fix memory issues and trivial code cleaup Gowtham Anandha Babu
` (2 preceding siblings ...)
2015-01-27 8:53 ` [PATCH v1 3/3] tools/hciattach_ath3k: Remove dead code warnings Gowtham Anandha Babu
@ 2015-01-27 13:39 ` Luiz Augusto von Dentz
3 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2015-01-27 13:39 UTC (permalink / raw)
To: Gowtham Anandha Babu; +Cc: linux-bluetooth, Bharat Panda, cpgs
Hi Gowtham,
On Tue, Jan 27, 2015 at 10:53 AM, Gowtham Anandha Babu
<gowtham.ab@samsung.com> wrote:
> In gatt-client and gatt-helpers, the variables are accessed
> even after it is freed. Making the unref at appropriate
> places solved the issues.
>
> In tools, removed dead code warnings.
>
> *v1: Rebased.
>
> Gowtham Anandha Babu (3):
> shared/gatt-helpers: Fix usage of freed memory
> shared/gatt-client: Fix usage of freed memory
> tools/hciattach_ath3k: Remove dead code warnings
>
> src/shared/gatt-client.c | 16 +++++++++-------
> src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
> tools/hciattach_ath3k.c | 11 +----------
> 3 files changed, 26 insertions(+), 26 deletions(-)
>
> --
> 1.9.1
Applied, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
2015-01-27 8:53 ` [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory Gowtham Anandha Babu
@ 2015-01-28 13:07 ` Luiz Augusto von Dentz
2015-01-28 14:18 ` Gowtham Anandha Babu
0 siblings, 1 reply; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2015-01-28 13:07 UTC (permalink / raw)
To: Gowtham Anandha Babu; +Cc: linux-bluetooth, Bharat Panda, cpgs
Hi Gowtham,
On Tue, Jan 27, 2015 at 10:53 AM, Gowtham Anandha Babu
<gowtham.ab@samsung.com> wrote:
> src/shared/gatt-helpers.c:709:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:780:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:998:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:1020:32: warning: Use of memory after it is freed
> op->callback(false, 0, NULL, data->op->user_data);
> ^~~~~~~~
> src/shared/gatt-helpers.c:1112:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:1226:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:1332:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> src/shared/gatt-helpers.c:1452:6: warning: Use of memory after it is freed
> if (op->callback)
> ^~~~~~~~~~~~
> ---
> src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> index 3864ed0..c9b24cc 100644
> --- a/src/shared/gatt-helpers.c
> +++ b/src/shared/gatt-helpers.c
> @@ -686,7 +686,6 @@ static void read_by_grp_type_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -708,6 +707,8 @@ success:
> done:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
> @@ -766,7 +767,6 @@ static void find_by_type_val_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -779,6 +779,8 @@ success:
> done:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> static bool discover_services(struct bt_att *att, bt_uuid_t *uuid,
> @@ -977,7 +979,6 @@ static void read_included_cb(uint8_t opcode, const void *pdu,
> discovery_op_ref(op), discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -997,6 +998,8 @@ static void read_included_cb(uint8_t opcode, const void *pdu,
> done:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> static void read_included(struct read_incl_data *data)
> @@ -1014,10 +1017,10 @@ static void read_included(struct read_incl_data *data)
> read_included_unref))
> return;
>
> - read_included_unref(data);
> -
> if (op->callback)
> op->callback(false, 0, NULL, data->op->user_data);
> +
> + read_included_unref(data);
> }
>
> static void discover_included_cb(uint8_t opcode, const void *pdu,
> @@ -1099,7 +1102,6 @@ static void discover_included_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto failed;
> }
> @@ -1111,6 +1113,8 @@ done:
> failed:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> bool bt_gatt_discover_included_services(struct bt_att *att,
> @@ -1213,7 +1217,6 @@ static void discover_chrcs_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -1226,6 +1229,8 @@ done:
> if (op->callback)
> op->callback(success, att_ecode, final_result,
> op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> bool bt_gatt_discover_characteristics(struct bt_att *att,
> @@ -1321,7 +1326,6 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -1332,6 +1336,8 @@ done:
> if (op->callback)
> op->callback(success, att_ecode, success ? op->result_head :
> NULL, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start, uint16_t end,
> @@ -1439,7 +1445,6 @@ static void discover_descs_cb(uint8_t opcode, const void *pdu,
> discovery_op_unref))
> return;
>
> - discovery_op_unref(op);
> success = false;
> goto done;
> }
> @@ -1451,6 +1456,8 @@ success:
> done:
> if (op->callback)
> op->callback(success, att_ecode, final_result, op->user_data);
> +
> + discovery_op_unref(op);
> }
>
> bool bt_gatt_discover_descriptors(struct bt_att *att,
> --
> 1.9.1
These were actually false positives of your static analyzer, in all
occurrences there are actually extra references preventing the memory
to be freed before use, next time please take the time to test this or
even better create a unit test to reproduce the problem. For this
specifically I had done something to track the actual pending
operation but for gatt-client changes I would have had to change some
APIs to do it properly and because of that I just revert it, perhaps
it is worth doing something similar there if it continues to show as a
problem in the static analyzers.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
2015-01-28 13:07 ` Luiz Augusto von Dentz
@ 2015-01-28 14:18 ` Gowtham Anandha Babu
2015-01-28 14:20 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 8+ messages in thread
From: Gowtham Anandha Babu @ 2015-01-28 14:18 UTC (permalink / raw)
To: 'Luiz Augusto von Dentz'
Cc: linux-bluetooth, 'Bharat Panda', cpgs
Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
> Sent: Wednesday, January 28, 2015 6:37 PM
> To: Gowtham Anandha Babu
> Cc: linux-bluetooth@vger.kernel.org; Bharat Panda; cpgs@samsung.com
> Subject: Re: [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
>
> Hi Gowtham,
>
> On Tue, Jan 27, 2015 at 10:53 AM, Gowtham Anandha Babu
> <gowtham.ab@samsung.com> wrote:
> > src/shared/gatt-helpers.c:709:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:780:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:998:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:1020:32: warning: Use of memory after it is freed
> > op->callback(false, 0, NULL, data->op->user_data);
> > ^~~~~~~~
> > src/shared/gatt-helpers.c:1112:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:1226:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:1332:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > src/shared/gatt-helpers.c:1452:6: warning: Use of memory after it is freed
> > if (op->callback)
> > ^~~~~~~~~~~~
> > ---
> > src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
> > 1 file changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
> > index 3864ed0..c9b24cc 100644
> > --- a/src/shared/gatt-helpers.c
> > +++ b/src/shared/gatt-helpers.c
> > @@ -686,7 +686,6 @@ static void read_by_grp_type_cb(uint8_t opcode,
> const void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -708,6 +707,8 @@ success:
> > done:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > static void find_by_type_val_cb(uint8_t opcode, const void *pdu, @@
> > -766,7 +767,6 @@ static void find_by_type_val_cb(uint8_t opcode, const
> void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -779,6 +779,8 @@ success:
> > done:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > static bool discover_services(struct bt_att *att, bt_uuid_t *uuid, @@
> > -977,7 +979,6 @@ static void read_included_cb(uint8_t opcode, const void
> *pdu,
> > discovery_op_ref(op), discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -997,6 +998,8 @@ static void read_included_cb(uint8_t opcode, const
> > void *pdu,
> > done:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > static void read_included(struct read_incl_data *data) @@ -1014,10
> > +1017,10 @@ static void read_included(struct read_incl_data *data)
> > read_included_unref))
> > return;
> >
> > - read_included_unref(data);
> > -
> > if (op->callback)
> > op->callback(false, 0, NULL, data->op->user_data);
> > +
> > + read_included_unref(data);
> > }
> >
> > static void discover_included_cb(uint8_t opcode, const void *pdu, @@
> > -1099,7 +1102,6 @@ static void discover_included_cb(uint8_t opcode,
> const void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto failed;
> > }
> > @@ -1111,6 +1113,8 @@ done:
> > failed:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > bool bt_gatt_discover_included_services(struct bt_att *att, @@
> > -1213,7 +1217,6 @@ static void discover_chrcs_cb(uint8_t opcode, const
> void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -1226,6 +1229,8 @@ done:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> >
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > bool bt_gatt_discover_characteristics(struct bt_att *att, @@ -1321,7
> > +1326,6 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -1332,6 +1336,8 @@ done:
> > if (op->callback)
> > op->callback(success, att_ecode, success ? op->result_head :
> > NULL,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start,
> > uint16_t end, @@ -1439,7 +1445,6 @@ static void
> discover_descs_cb(uint8_t opcode, const void *pdu,
> > discovery_op_unref))
> > return;
> >
> > - discovery_op_unref(op);
> > success = false;
> > goto done;
> > }
> > @@ -1451,6 +1456,8 @@ success:
> > done:
> > if (op->callback)
> > op->callback(success, att_ecode, final_result,
> > op->user_data);
> > +
> > + discovery_op_unref(op);
> > }
> >
> > bool bt_gatt_discover_descriptors(struct bt_att *att,
> > --
> > 1.9.1
>
> These were actually false positives of your static analyzer, in all occurrences
> there are actually extra references preventing the memory to be freed
> before use, next time please take the time to test this or even better create
> a unit test to reproduce the problem. For this specifically I had done
> something to track the actual pending operation but for gatt-client changes I
> would have had to change some APIs to do it properly and because of that I
> just revert it, perhaps it is worth doing something similar there if it continues
> to show as a problem in the static analyzers.
>
Sorry for the false positives. I will check once again and test it before sending.
Meanwhile I am using clang static analyzer for testing. I did git pull and ran the tool.
It gave me the following warnings:
src/shared/gatt-helpers.c:851:9: warning: Use of memory after it is freed
return op->id ? true : false;
^~~~~~
1 warning generated.
src/shared/gatt-client.c:534:14: warning: Use of memory after it is freed
op->success = false;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:689:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:790:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:882:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:950:14: warning: Use of memory after it is freed
op->success = success;
~~~~~~~~~~~ ^
src/shared/gatt-client.c:2328:2: warning: Use of memory after it is freed
complete_write_long_op(req, success, 0, false);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/shared/gatt-client.c:2350:2: warning: Use of memory after it is freed
request_unref(req);
^~~~~~~~~~~~~~~~~~
7 warnings generated.
I think the gatt-helpers: warning seems valid for me. But in gatt-client it is still
giving the warnings. Is it still false positive?
Regards,
Gowtham Anandha Babu
>
> --
> Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
2015-01-28 14:18 ` Gowtham Anandha Babu
@ 2015-01-28 14:20 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 8+ messages in thread
From: Luiz Augusto von Dentz @ 2015-01-28 14:20 UTC (permalink / raw)
To: Gowtham Anandha Babu; +Cc: linux-bluetooth, Bharat Panda, cpgs
Hi Gowtham,
On Wed, Jan 28, 2015 at 4:18 PM, Gowtham Anandha Babu
<gowtham.ab@samsung.com> wrote:
> Hi Luiz,
>
>> -----Original Message-----
>> From: Luiz Augusto von Dentz [mailto:luiz.dentz@gmail.com]
>> Sent: Wednesday, January 28, 2015 6:37 PM
>> To: Gowtham Anandha Babu
>> Cc: linux-bluetooth@vger.kernel.org; Bharat Panda; cpgs@samsung.com
>> Subject: Re: [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory
>>
>> Hi Gowtham,
>>
>> On Tue, Jan 27, 2015 at 10:53 AM, Gowtham Anandha Babu
>> <gowtham.ab@samsung.com> wrote:
>> > src/shared/gatt-helpers.c:709:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:780:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:998:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:1020:32: warning: Use of memory after it is freed
>> > op->callback(false, 0, NULL, data->op->user_data);
>> > ^~~~~~~~
>> > src/shared/gatt-helpers.c:1112:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:1226:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:1332:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > src/shared/gatt-helpers.c:1452:6: warning: Use of memory after it is freed
>> > if (op->callback)
>> > ^~~~~~~~~~~~
>> > ---
>> > src/shared/gatt-helpers.c | 25 ++++++++++++++++---------
>> > 1 file changed, 16 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/src/shared/gatt-helpers.c b/src/shared/gatt-helpers.c
>> > index 3864ed0..c9b24cc 100644
>> > --- a/src/shared/gatt-helpers.c
>> > +++ b/src/shared/gatt-helpers.c
>> > @@ -686,7 +686,6 @@ static void read_by_grp_type_cb(uint8_t opcode,
>> const void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -708,6 +707,8 @@ success:
>> > done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > static void find_by_type_val_cb(uint8_t opcode, const void *pdu, @@
>> > -766,7 +767,6 @@ static void find_by_type_val_cb(uint8_t opcode, const
>> void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -779,6 +779,8 @@ success:
>> > done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > static bool discover_services(struct bt_att *att, bt_uuid_t *uuid, @@
>> > -977,7 +979,6 @@ static void read_included_cb(uint8_t opcode, const void
>> *pdu,
>> > discovery_op_ref(op), discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -997,6 +998,8 @@ static void read_included_cb(uint8_t opcode, const
>> > void *pdu,
>> > done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > static void read_included(struct read_incl_data *data) @@ -1014,10
>> > +1017,10 @@ static void read_included(struct read_incl_data *data)
>> > read_included_unref))
>> > return;
>> >
>> > - read_included_unref(data);
>> > -
>> > if (op->callback)
>> > op->callback(false, 0, NULL, data->op->user_data);
>> > +
>> > + read_included_unref(data);
>> > }
>> >
>> > static void discover_included_cb(uint8_t opcode, const void *pdu, @@
>> > -1099,7 +1102,6 @@ static void discover_included_cb(uint8_t opcode,
>> const void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto failed;
>> > }
>> > @@ -1111,6 +1113,8 @@ done:
>> > failed:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > bool bt_gatt_discover_included_services(struct bt_att *att, @@
>> > -1213,7 +1217,6 @@ static void discover_chrcs_cb(uint8_t opcode, const
>> void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -1226,6 +1229,8 @@ done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> >
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > bool bt_gatt_discover_characteristics(struct bt_att *att, @@ -1321,7
>> > +1326,6 @@ static void read_by_type_cb(uint8_t opcode, const void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -1332,6 +1336,8 @@ done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, success ? op->result_head :
>> > NULL,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > bool bt_gatt_read_by_type(struct bt_att *att, uint16_t start,
>> > uint16_t end, @@ -1439,7 +1445,6 @@ static void
>> discover_descs_cb(uint8_t opcode, const void *pdu,
>> > discovery_op_unref))
>> > return;
>> >
>> > - discovery_op_unref(op);
>> > success = false;
>> > goto done;
>> > }
>> > @@ -1451,6 +1456,8 @@ success:
>> > done:
>> > if (op->callback)
>> > op->callback(success, att_ecode, final_result,
>> > op->user_data);
>> > +
>> > + discovery_op_unref(op);
>> > }
>> >
>> > bool bt_gatt_discover_descriptors(struct bt_att *att,
>> > --
>> > 1.9.1
>>
>> These were actually false positives of your static analyzer, in all occurrences
>> there are actually extra references preventing the memory to be freed
>> before use, next time please take the time to test this or even better create
>> a unit test to reproduce the problem. For this specifically I had done
>> something to track the actual pending operation but for gatt-client changes I
>> would have had to change some APIs to do it properly and because of that I
>> just revert it, perhaps it is worth doing something similar there if it continues
>> to show as a problem in the static analyzers.
>>
>
> Sorry for the false positives. I will check once again and test it before sending.
> Meanwhile I am using clang static analyzer for testing. I did git pull and ran the tool.
> It gave me the following warnings:
>
> src/shared/gatt-helpers.c:851:9: warning: Use of memory after it is freed
> return op->id ? true : false;
> ^~~~~~
> 1 warning generated.
Yep, this one seems valid, I will fix it asap.
> src/shared/gatt-client.c:534:14: warning: Use of memory after it is freed
> op->success = false;
> ~~~~~~~~~~~ ^
> src/shared/gatt-client.c:689:14: warning: Use of memory after it is freed
> op->success = success;
> ~~~~~~~~~~~ ^
> src/shared/gatt-client.c:790:14: warning: Use of memory after it is freed
> op->success = success;
> ~~~~~~~~~~~ ^
> src/shared/gatt-client.c:882:14: warning: Use of memory after it is freed
> op->success = success;
> ~~~~~~~~~~~ ^
> src/shared/gatt-client.c:950:14: warning: Use of memory after it is freed
> op->success = success;
> ~~~~~~~~~~~ ^
> src/shared/gatt-client.c:2328:2: warning: Use of memory after it is freed
> complete_write_long_op(req, success, 0, false);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> src/shared/gatt-client.c:2350:2: warning: Use of memory after it is freed
> request_unref(req);
> ^~~~~~~~~~~~~~~~~~
> 7 warnings generated.
>
> I think the gatt-helpers: warning seems valid for me. But in gatt-client it is still
> giving the warnings. Is it still false positive?
These are false positives, it cause double free if we fix them in the
way you did before but anyway I would have this code to cleanup in a
single function and properly track pending requests back to ATT but in
order to do that we need to change the APIs.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-01-28 14:20 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-27 8:53 [PATCH v1 0/3] Fix memory issues and trivial code cleaup Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 1/3] shared/gatt-helpers: Fix usage of freed memory Gowtham Anandha Babu
2015-01-28 13:07 ` Luiz Augusto von Dentz
2015-01-28 14:18 ` Gowtham Anandha Babu
2015-01-28 14:20 ` Luiz Augusto von Dentz
2015-01-27 8:53 ` [PATCH v1 2/3] shared/gatt-client: " Gowtham Anandha Babu
2015-01-27 8:53 ` [PATCH v1 3/3] tools/hciattach_ath3k: Remove dead code warnings Gowtham Anandha Babu
2015-01-27 13:39 ` [PATCH v1 0/3] Fix memory issues and trivial code cleaup Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.