* Fwd: a2dp.c: finalize_config(setup) can destroy setup [not found] <CAGd9gwgKMt21TnGLJ09+gAkP-=Jn9mfoAThiBuqup_vTyCshuw@mail.gmail.com> @ 2013-05-16 4:06 ` Alex Deymo 2013-05-16 11:02 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Alex Deymo @ 2013-05-16 4:06 UTC (permalink / raw) To: keybuk, linux-bluetooth Hello, I just found this invalid read where finalize_config(setup); is actually destroying the setup pointer passed there. The log attached below is running with valgrind on bluez 5.5 (with a tiny patch to src/log.{c|h} to have the debug file:line:function on error() as you may see on the logs) The code around the open_cfm (a2dp.c:730) is: finalize_config(setup); if (!setup->start || !err) return; and the invalid must be the "setup->start". The debug log shows that setup_free was called just before and if I'm not missing something, it must come from finalize_setup()... but not sure why. The steps to run into this (although there may be some timing involved) are simply scan, pair, trust and connect the device with bluetoothctl. Device is a Bose SoundLink model 404600. I hope it helps, Alex. bluetoothd[11636]: profiles/audio/a2dp.c:open_cfm() Source 0x6124b40: Open_Cfm bluetoothd[11636]: profiles/audio/sink.c:stream_setup_complete() Stream successfully created bluetoothd[11636]: src/service.c:change_state() 0x62897b0: device 00:0C:8A:XX:XX:XX profile audio-sink state changed: connecting -> connected (0) bluetoothd[11636]: src/device.c:device_profile_connected() audio-sink Success (0) bluetoothd[11636]: src/service.c:change_state() 0x62999d0: device 00:0C:8A:XX:XX:XX profile audio-avrcp-target state changed: disconnected -> connecting (0) bluetoothd[11636]: profiles/audio/manager.c:avrcp_target_connect() path /org/bluez/hci0/dev_00_0C_8A_XX_XX_XX bluetoothd[11636]: src/service.c:208:btd_service_connect() audio-avrcp-target profile connect failed for 00:0C:8A:XX:XX:XX: Operation already in progress bluetoothd[11636]: src/service.c:change_state() 0x62999d0: device 00:0C:8A:XX:XX:XX profile audio-avrcp-target state changed: connecting -> disconnected (-114) bluetoothd[11636]: src/device.c:device_profile_connected() audio-avrcp-target Operation already in progress (114) bluetoothd[11636]: src/device.c:device_profile_connected() returning response to :1.156 bluetoothd[11636]: profiles/audio/a2dp.c:setup_unref() 0x62c8520: ref=0 bluetoothd[11636]: profiles/audio/a2dp.c:setup_free() 0x62c8520 bluetoothd[11636]: profiles/audio/avdtp.c:avdtp_unref() 0x62a98b0: ref=1 ==11636== Invalid read of size 4 ==11636== at 0x4214AD: open_cfm (a2dp.c:730) ==11636== by 0x424D07: handle_transport_connect (avdtp.c:878) ==11636== by 0x4288F2: avdtp_connect_cb (avdtp.c:2419) ==11636== by 0x4458B8: connect_cb (btio.c:230) ==11636== by 0x4E79D12: g_main_context_dispatch (gmain.c:2539) ==11636== by 0x4E7A05F: g_main_context_iterate.isra.23 (gmain.c:3146) ==11636== by 0x4E7A459: g_main_loop_run (gmain.c:3340) ==11636== by 0x44B0AC: main (main.c:583) ==11636== Address 0x62c8564 is 68 bytes inside a block of size 88 free'd ==11636== at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==11636== by 0x420094: setup_free (a2dp.c:156) ==11636== by 0x420101: setup_unref (a2dp.c:168) ==11636== by 0x4201CF: setup_cb_free (a2dp.c:191) ==11636== by 0x4203DC: finalize_config (a2dp.c:234) ==11636== by 0x4214A8: open_cfm (a2dp.c:728) ==11636== by 0x424D07: handle_transport_connect (avdtp.c:878) ==11636== by 0x4288F2: avdtp_connect_cb (avdtp.c:2419) ==11636== by 0x4458B8: connect_cb (btio.c:230) ==11636== by 0x4E79D12: g_main_context_dispatch (gmain.c:2539) ==11636== by 0x4E7A05F: g_main_context_iterate.isra.23 (gmain.c:3146) ==11636== by 0x4E7A459: g_main_loop_run (gmain.c:3340) ==11636== ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: a2dp.c: finalize_config(setup) can destroy setup 2013-05-16 4:06 ` Fwd: a2dp.c: finalize_config(setup) can destroy setup Alex Deymo @ 2013-05-16 11:02 ` Luiz Augusto von Dentz 2013-05-16 20:34 ` Alex Deymo 0 siblings, 1 reply; 4+ messages in thread From: Luiz Augusto von Dentz @ 2013-05-16 11:02 UTC (permalink / raw) To: Alex Deymo; +Cc: keybuk, linux-bluetooth Hi Alex, On Thu, May 16, 2013 at 7:06 AM, Alex Deymo <deymo@chromium.org> wrote: > Hello, > > I just found this invalid read where finalize_config(setup); is > actually destroying the setup pointer passed there. The log attached > below is running with valgrind on bluez 5.5 (with a tiny patch to > src/log.{c|h} to have the debug file:line:function on error() as you > may see on the logs) > > The code around the open_cfm (a2dp.c:730) is: > finalize_config(setup); > > if (!setup->start || !err) > return; > and the invalid must be the "setup->start". The debug log shows that > setup_free was called just before and if I'm not missing something, it > must come from finalize_setup()... but not sure why. Yep, this is actually a regression introduced by: commit 99c6f5221800a48e8ce0b1e070e97d1c26a0f90b Author: Luiz Augusto von Dentz <luiz.von.dentz@intel.com> Date: Tue Feb 19 12:54:55 2013 +0200 A2DP: Mark start flag if resume happen while in configured state If SEP is in configured state that means OPEN is about to happen so just mark start flag and send START once OPEN completes. As we don't have a start flag, perhaps because the endpoint don't try/delay the Acquire, the setup is free by finalize_config because there is no other operation pending. > The steps to run into this (although there may be some timing > involved) are simply scan, pair, trust and connect the device with > bluetoothctl. Device is a Bose SoundLink model 404600. Looks like there are some new devices that should try to get a hold, anyway this problem should be fixed ASAP so what about the following patch: diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c index 215f4db..c6973ae 100644 --- a/profiles/audio/a2dp.c +++ b/profiles/audio/a2dp.c @@ -723,16 +723,12 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep, if (err) { setup->stream = NULL; setup->err = err; + if (setup->start) + finalize_resume(setup); } finalize_config(setup); - if (!setup->start || !err) - return; - - setup->start = FALSE; - finalize_resume(setup); - return; } ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: a2dp.c: finalize_config(setup) can destroy setup 2013-05-16 11:02 ` Luiz Augusto von Dentz @ 2013-05-16 20:34 ` Alex Deymo 2013-05-17 6:47 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Alex Deymo @ 2013-05-16 20:34 UTC (permalink / raw) To: Luiz Augusto von Dentz; +Cc: keybuk, linux-bluetooth Hi Luiz, On Thu, May 16, 2013 at 4:02 AM, Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > Looks like there are some new devices that should try to get a hold, > anyway this problem should be fixed ASAP so what about the following > patch: > > diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c > index 215f4db..c6973ae 100644 > --- a/profiles/audio/a2dp.c > +++ b/profiles/audio/a2dp.c > @@ -723,16 +723,12 @@ static void open_cfm(struct avdtp *session, > struct avdtp_local_sep *sep, > if (err) { > setup->stream = NULL; > setup->err = err; > + if (setup->start) > + finalize_resume(setup); > } > > finalize_config(setup); > > - if (!setup->start || !err) > - return; > - > - setup->start = FALSE; > - finalize_resume(setup); > - > return; > } This patch looks good to me. I tried it in the same scenario and valgrind does not complain. Could you please push it to the repo? Thanks, Alex. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: a2dp.c: finalize_config(setup) can destroy setup 2013-05-16 20:34 ` Alex Deymo @ 2013-05-17 6:47 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 4+ messages in thread From: Luiz Augusto von Dentz @ 2013-05-17 6:47 UTC (permalink / raw) To: Alex Deymo; +Cc: keybuk, linux-bluetooth Hi Alex, On Thu, May 16, 2013 at 11:34 PM, Alex Deymo <deymo@chromium.org> wrote: > Hi Luiz, > > On Thu, May 16, 2013 at 4:02 AM, Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: >> Looks like there are some new devices that should try to get a hold, >> anyway this problem should be fixed ASAP so what about the following >> patch: >> >> diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c >> index 215f4db..c6973ae 100644 >> --- a/profiles/audio/a2dp.c >> +++ b/profiles/audio/a2dp.c >> @@ -723,16 +723,12 @@ static void open_cfm(struct avdtp *session, >> struct avdtp_local_sep *sep, >> if (err) { >> setup->stream = NULL; >> setup->err = err; >> + if (setup->start) >> + finalize_resume(setup); >> } >> >> finalize_config(setup); >> >> - if (!setup->start || !err) >> - return; >> - >> - setup->start = FALSE; >> - finalize_resume(setup); >> - >> return; >> } > > This patch looks good to me. I tried it in the same scenario and > valgrind does not complain. > Could you please push it to the repo? Pushed, thanks for testing it. -- Luiz Augusto von Dentz ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-17 6:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAGd9gwgKMt21TnGLJ09+gAkP-=Jn9mfoAThiBuqup_vTyCshuw@mail.gmail.com>
2013-05-16 4:06 ` Fwd: a2dp.c: finalize_config(setup) can destroy setup Alex Deymo
2013-05-16 11:02 ` Luiz Augusto von Dentz
2013-05-16 20:34 ` Alex Deymo
2013-05-17 6:47 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).