| Message ID | 20200717191515.220621-1-luiz.dentz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BlueZ,1/5] shared/att: Fix possible crash on disconnect | expand |
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While we are preparing for reviewing the patches, we found the following
issue/warning.
Test Result:
checkpatch Failed
Outputs:
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#17:
by 0x48E963B: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6400.4)
- total: 0 errors, 1 warnings, 83 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Your patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
This is automated email and please do not reply to this email! Dear submitter, Thank you for submitting the patches to the linux bluetooth mailing list. While we are preparing for reviewing the patches, we found the following issue/warning. Test Result: makecheck Failed Outputs: ./test-driver: line 107: 14830 Bus error (core dumped) "$@" > $log_file 2>&1 make[3]: *** [Makefile:9726: test-suite.log] Error 1 make[2]: *** [Makefile:9834: check-TESTS] Error 2 make[1]: *** [Makefile:10228: check-am] Error 2 make: *** [Makefile:10230: check] Error 2 --- Regards, Linux Bluetooth
Hi Luiz, The failure is from test-gatt DEBUG :FAIL: unit/test-gatt I will update the CI to include the test result. Regards, Tedd On Fri, 2020-07-17 at 12:23 -0700, bluez.test.bot@gmail.com wrote: > This is automated email and please do not reply to this email! > > Dear submitter, > > Thank you for submitting the patches to the linux bluetooth mailing list. > While we are preparing for reviewing the patches, we found the following > issue/warning. > > Test Result: > makecheck Failed > > Outputs: > ./test-driver: line 107: 14830 Bus error (core dumped) "$@" > > $log_file 2>&1 > make[3]: *** [Makefile:9726: test-suite.log] Error 1 > make[2]: *** [Makefile:9834: check-TESTS] Error 2 > make[1]: *** [Makefile:10228: check-am] Error 2 > make: *** [Makefile:10230: check] Error 2 > > > > --- > Regards, > Linux Bluetooth
Hi Tedd, On Fri, Jul 17, 2020 at 1:35 PM Tedd Ho-Jeong An <tedd.an@linux.intel.com> wrote: > > Hi Luiz, > > The failure is from test-gatt > > DEBUG :FAIL: unit/test-gatt > > I will update the CI to include the test result. > > Regards, > Tedd > > On Fri, 2020-07-17 at 12:23 -0700, bluez.test.bot@gmail.com wrote: > > This is automated email and please do not reply to this email! > > > > Dear submitter, > > > > Thank you for submitting the patches to the linux bluetooth mailing list. > > While we are preparing for reviewing the patches, we found the following > > issue/warning. > > > > Test Result: > > makecheck Failed > > > > Outputs: > > ./test-driver: line 107: 14830 Bus error (core dumped) "$@" > > > $log_file 2>&1 > > make[3]: *** [Makefile:9726: test-suite.log] Error 1 > > make[2]: *** [Makefile:9834: check-TESTS] Error 2 > > make[1]: *** [Makefile:10228: check-am] Error 2 > > make: *** [Makefile:10230: check] Error 2 Weird, it start failing for me as well but it doesn't seems to be causing any test to fail: Total: 192, Passed: 192 (100.0%), Failed: 0, Not Run: 0 Overall execution time: 0.849 seconds FAIL unit/test-gatt (exit status: 1) > > > > > > > > --- > > Regards, > > Linux Bluetooth >
Hi Tedd, On Fri, Jul 17, 2020 at 1:45 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Tedd, > > On Fri, Jul 17, 2020 at 1:35 PM Tedd Ho-Jeong An > <tedd.an@linux.intel.com> wrote: > > > > Hi Luiz, > > > > The failure is from test-gatt > > > > DEBUG :FAIL: unit/test-gatt > > > > I will update the CI to include the test result. > > > > Regards, > > Tedd > > > > On Fri, 2020-07-17 at 12:23 -0700, bluez.test.bot@gmail.com wrote: > > > This is automated email and please do not reply to this email! > > > > > > Dear submitter, > > > > > > Thank you for submitting the patches to the linux bluetooth mailing list. > > > While we are preparing for reviewing the patches, we found the following > > > issue/warning. > > > > > > Test Result: > > > makecheck Failed > > > > > > Outputs: > > > ./test-driver: line 107: 14830 Bus error (core dumped) "$@" > > > > $log_file 2>&1 > > > make[3]: *** [Makefile:9726: test-suite.log] Error 1 > > > make[2]: *** [Makefile:9834: check-TESTS] Error 2 > > > make[1]: *** [Makefile:10228: check-am] Error 2 > > > make: *** [Makefile:10230: check] Error 2 > > Weird, it start failing for me as well but it doesn't seems to be > causing any test to fail: > > Total: 192, Passed: 192 (100.0%), Failed: 0, Not Run: 0 > Overall execution time: 0.849 seconds > FAIL unit/test-gatt (exit status: 1) Ive send a fix for it, there was a crash detected by valgrind which don't make the test to fail but it actually shows in the logs, there might be a way to detect the crash and make the test fail to make this more obvious. > > > > > > > > > > > > > --- > > > Regards, > > > Linux Bluetooth > > > > > -- > Luiz Augusto von Dentz
diff --git a/src/shared/att.c b/src/shared/att.c index ed3af2920..58f23dfcb 100644 --- a/src/shared/att.c +++ b/src/shared/att.c @@ -84,6 +84,7 @@ struct bt_att { struct queue *req_queue; /* Queued ATT protocol requests */ struct queue *ind_queue; /* Queued ATT protocol indications */ struct queue *write_queue; /* Queue of PDUs ready to send */ + bool in_disc; /* Cleanup queues on disconnect_cb */ bt_att_timeout_func_t timeout_callback; bt_att_destroy_func_t timeout_destroy; @@ -222,8 +223,10 @@ static void destroy_att_send_op(void *data) free(op); } -static void cancel_att_send_op(struct att_send_op *op) +static void cancel_att_send_op(void *data) { + struct att_send_op *op = data; + if (op->destroy) op->destroy(op->user_data); @@ -631,11 +634,6 @@ static bool disconnect_cb(struct io *io, void *user_data) /* Dettach channel */ queue_remove(att->chans, chan); - /* Notify request callbacks */ - queue_remove_all(att->req_queue, NULL, NULL, disc_att_send_op); - queue_remove_all(att->ind_queue, NULL, NULL, disc_att_send_op); - queue_remove_all(att->write_queue, NULL, NULL, disc_att_send_op); - if (chan->pending_req) { disc_att_send_op(chan->pending_req); chan->pending_req = NULL; @@ -654,6 +652,15 @@ static bool disconnect_cb(struct io *io, void *user_data) bt_att_ref(att); + att->in_disc = true; + + /* Notify request callbacks */ + queue_remove_all(att->req_queue, NULL, NULL, disc_att_send_op); + queue_remove_all(att->ind_queue, NULL, NULL, disc_att_send_op); + queue_remove_all(att->write_queue, NULL, NULL, disc_att_send_op); + + att->in_disc = false; + queue_foreach(att->disconn_list, disconn_handler, INT_TO_PTR(err)); bt_att_unregister_all(att); @@ -1574,6 +1581,30 @@ bool bt_att_chan_cancel(struct bt_att_chan *chan, unsigned int id) return true; } +static bool bt_att_disc_cancel(struct bt_att *att, unsigned int id) +{ + struct att_send_op *op; + + op = queue_find(att->req_queue, match_op_id, UINT_TO_PTR(id)); + if (op) + goto done; + + op = queue_find(att->ind_queue, match_op_id, UINT_TO_PTR(id)); + if (op) + goto done; + + op = queue_find(att->write_queue, match_op_id, UINT_TO_PTR(id)); + +done: + if (!op) + return false; + + /* Just cancel since disconnect_cb will be cleaning up */ + cancel_att_send_op(op); + + return true; +} + bool bt_att_cancel(struct bt_att *att, unsigned int id) { const struct queue_entry *entry; @@ -1591,6 +1622,9 @@ bool bt_att_cancel(struct bt_att *att, unsigned int id) return true; } + if (att->in_disc) + return bt_att_disc_cancel(att, id); + op = queue_remove_if(att->req_queue, match_op_id, UINT_TO_PTR(id)); if (op) goto done;
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> If there are pending request while disconnecting they would be notified but clients may endup being freed in the proccess which will then be calling bt_att_cancel to cancal its requests causing the following trace: Invalid read of size 4 at 0x1D894C: enable_ccc_callback (gatt-client.c:1627) by 0x1D247B: disc_att_send_op (att.c:417) by 0x1CCC17: queue_remove_all (queue.c:354) by 0x1D47B7: disconnect_cb (att.c:635) by 0x1E0707: watch_callback (io-glib.c:170) by 0x48E963B: g_main_context_dispatch (in /usr/lib/libglib-2.0.so.0.6400.4) by 0x48E9AC7: ??? (in /usr/lib/libglib-2.0.so.0.6400.4) by 0x48E9ECF: g_main_loop_run (in /usr/lib/libglib-2.0.so.0.6400.4) by 0x1E0E97: mainloop_run (mainloop-glib.c:79) by 0x1E13B3: mainloop_run_with_signal (mainloop-notify.c:201) by 0x12BC3B: main (main.c:770) Address 0x7d40a28 is 24 bytes inside a block of size 32 free'd at 0x484A2E0: free (vg_replace_malloc.c:540) by 0x1CCC17: queue_remove_all (queue.c:354) by 0x1CCC83: queue_destroy (queue.c:73) by 0x1D7DD7: bt_gatt_client_free (gatt-client.c:2209) by 0x16497B: batt_free (battery.c:77) by 0x16497B: batt_remove (battery.c:286) by 0x1A0013: service_remove (service.c:176) by 0x1A9B7B: device_remove_gatt_service (device.c:3691) by 0x1A9B7B: gatt_service_removed (device.c:3805) by 0x1CC90B: queue_foreach (queue.c:220) by 0x1DE27B: notify_service_changed.isra.0.part.0 (gatt-db.c:369) by 0x1DE387: notify_service_changed (gatt-db.c:361) by 0x1DE387: gatt_db_service_destroy (gatt-db.c:385) by 0x1DE3EF: gatt_db_remove_service (gatt-db.c:519) by 0x1D674F: discovery_op_complete (gatt-client.c:388) by 0x1D6877: discover_primary_cb (gatt-client.c:1260) by 0x1E220B: discovery_op_complete (gatt-helpers.c:628) by 0x1E249B: read_by_grp_type_cb (gatt-helpers.c:730) by 0x1D247B: disc_att_send_op (att.c:417) by 0x1CCC17: queue_remove_all (queue.c:354) by 0x1D47B7: disconnect_cb (att.c:635) --- src/shared/att.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-)