Hi Gowtham, On Wed, Jan 28, 2015 at 4:18 PM, Gowtham Anandha Babu <gowtham.ab@xxxxxxxxxxx> wrote: > Hi Luiz, > >> -----Original Message----- >> From: Luiz Augusto von Dentz [mailto:luiz.dentz@xxxxxxxxx] >> Sent: Wednesday, January 28, 2015 6:37 PM >> To: Gowtham Anandha Babu >> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Bharat Panda; cpgs@xxxxxxxxxxx >> 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@xxxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
