'audio-avrcp-control' has no need to be informed of connections or
disconnections, implied by the lack of .connect() and .disconnect()
callbacks. So no need to keep track of its connection status.
This also fixes this crash:
bluetoothd[22811]: profiles/audio/avrcp.c:session_ct_destroy() 0x6325370
bluetoothd[22811]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
==22811== Invalid read of size 8
==22811== at 0x466F10: dev_disconn_profile (device.c:976)
==22811== by 0x4E9307C: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x46B30B: device_request_disconnect (device.c:1005)
==22811== by 0x46B477: disconnect (device.c:1048)
==22811== by 0x40D050: process_message.isra.4 (object.c:258)
==22811== by 0x517C9E5: _dbus_object_tree_dispatch_and_unlock (in /usr/lib64/libdbus-1.so.3.7.2)
==22811== by 0x5167349: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
==22811== by 0x40ABD7: message_dispatch (mainloop.c:76)
==22811== by 0x4E77BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E77044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E77377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E77771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== Address 0x6256af0 is 0 bytes after a block of size 240 alloc'd
==22811== at 0x4C27816: memalign (vg_replace_malloc.c:727)
==22811== by 0x4C27907: posix_memalign (vg_replace_malloc.c:876)
==22811== by 0x4E49F80: slab_allocator_alloc_chunk (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E92094: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E9299D: g_slist_prepend (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E75790: g_source_add_poll (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4EB4612: g_io_unix_create_watch (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x4E6A443: g_io_add_watch_full (in /usr/lib64/libglib-2.0.so.0.3400.2)
==22811== by 0x44CD15: bt_io_listen (btio.c:283)
==22811== by 0x42EBDD: server_register (server.c:764)
==22811== by 0x45DD9E: probe_profile (adapter.c:2650)
==22811== by 0x466BCB: btd_profile_foreach (profile.c:599)
==22811==
This is caused because 'audio_control_disconnected()' which removes two elements
from the device->connected_profiles list, is called from inside a GFunc of a
g_slist_foreach() (src/device.c:1005) leaving the list corrupted.
---
The correct solution may be to make the "list" more robust against this kind of
change in the first place.
profiles/audio/manager.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
index 64665d7..f7b8559 100644
--- a/profiles/audio/manager.c
+++ b/profiles/audio/manager.c
@@ -422,13 +422,11 @@ void audio_source_disconnected(struct btd_device *dev, int err)
void audio_control_connected(struct btd_device *dev, int err)
{
device_profile_connected(dev, &avrcp_target_profile, err);
- device_profile_connected(dev, &avrcp_remote_profile, err);
}
void audio_control_disconnected(struct btd_device *dev, int err)
{
device_profile_disconnected(dev, &avrcp_target_profile, err);
- device_profile_disconnected(dev, &avrcp_remote_profile, err);
}
int audio_manager_init(GKeyFile *conf)
--
1.8.1.5
Hi Johan,
On 15:31 Thu 04 Apr, Johan Hedberg wrote:
> Hi Vinicius,
>
> On Wed, Mar 13, 2013, Vinicius Costa Gomes wrote:
> > 'audio-avrcp-control' has no need to be informed of connections or
> > disconnections, implied by the lack of .connect() and .disconnect()
> > callbacks. So no need to keep track of its connection status.
> >
> > This also fixes this crash:
> >
> > bluetoothd[22811]: profiles/audio/avrcp.c:session_ct_destroy() 0x6325370
> > bluetoothd[22811]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
> > ==22811== Invalid read of size 8
> > ==22811== at 0x466F10: dev_disconn_profile (device.c:976)
> > ==22811== by 0x4E9307C: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x46B30B: device_request_disconnect (device.c:1005)
> > ==22811== by 0x46B477: disconnect (device.c:1048)
> > ==22811== by 0x40D050: process_message.isra.4 (object.c:258)
> > ==22811== by 0x517C9E5: _dbus_object_tree_dispatch_and_unlock (in /usr/lib64/libdbus-1.so.3.7.2)
> > ==22811== by 0x5167349: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
> > ==22811== by 0x40ABD7: message_dispatch (mainloop.c:76)
> > ==22811== by 0x4E77BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E77044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E77377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E77771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== Address 0x6256af0 is 0 bytes after a block of size 240 alloc'd
> > ==22811== at 0x4C27816: memalign (vg_replace_malloc.c:727)
> > ==22811== by 0x4C27907: posix_memalign (vg_replace_malloc.c:876)
> > ==22811== by 0x4E49F80: slab_allocator_alloc_chunk (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E92094: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E9299D: g_slist_prepend (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E75790: g_source_add_poll (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4EB4612: g_io_unix_create_watch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x4E6A443: g_io_add_watch_full (in /usr/lib64/libglib-2.0.so.0.3400.2)
> > ==22811== by 0x44CD15: bt_io_listen (btio.c:283)
> > ==22811== by 0x42EBDD: server_register (server.c:764)
> > ==22811== by 0x45DD9E: probe_profile (adapter.c:2650)
> > ==22811== by 0x466BCB: btd_profile_foreach (profile.c:599)
> > ==22811==
> >
[snip]
>
> Is this patch still needed? We're planning to push 5.4 out soonish, so
> if there's a known crash it should be fixed.
Yes. I can still reproduce this problem with BlueZ master. It should be noted
that Luiz has a patch that also solves this problem: "[PATCH BlueZ v2] audio:
Track connections to AVRCP roles separately".
>
> The patch you sent doesn't quite look good (which is why it's not yet
> applied) since it seems unintuitive to me that a "global" AVRCP
> connection state notification should only affect one specific role even
> though both are represented as individual btd_profile objects.
> Especially if/when we start exposing these states through D-Bus you
> really do need to have the correct state for both.
>
> So preliminarily it seems to me like this is something needing fixing in
> the bluetoothd core, i.e. src/device.c. Could you send a patch for that?
I agree that even if this patch (or Luiz patch) works, the core should be
prepared to deal with this kind of situation. Looking into it.
>
> Johan
Cheers,
--
Vinicius
Hi Vinicius,
On Wed, Mar 13, 2013, Vinicius Costa Gomes wrote:
> 'audio-avrcp-control' has no need to be informed of connections or
> disconnections, implied by the lack of .connect() and .disconnect()
> callbacks. So no need to keep track of its connection status.
>
> This also fixes this crash:
>
> bluetoothd[22811]: profiles/audio/avrcp.c:session_ct_destroy() 0x6325370
> bluetoothd[22811]: profiles/audio/avctp.c:avctp_set_state() AVCTP Disconnected
> ==22811== Invalid read of size 8
> ==22811== at 0x466F10: dev_disconn_profile (device.c:976)
> ==22811== by 0x4E9307C: g_slist_foreach (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x46B30B: device_request_disconnect (device.c:1005)
> ==22811== by 0x46B477: disconnect (device.c:1048)
> ==22811== by 0x40D050: process_message.isra.4 (object.c:258)
> ==22811== by 0x517C9E5: _dbus_object_tree_dispatch_and_unlock (in /usr/lib64/libdbus-1.so.3.7.2)
> ==22811== by 0x5167349: dbus_connection_dispatch (in /usr/lib64/libdbus-1.so.3.7.2)
> ==22811== by 0x40ABD7: message_dispatch (mainloop.c:76)
> ==22811== by 0x4E77BCA: g_timeout_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E77044: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E77377: g_main_context_iterate.isra.24 (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E77771: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== Address 0x6256af0 is 0 bytes after a block of size 240 alloc'd
> ==22811== at 0x4C27816: memalign (vg_replace_malloc.c:727)
> ==22811== by 0x4C27907: posix_memalign (vg_replace_malloc.c:876)
> ==22811== by 0x4E49F80: slab_allocator_alloc_chunk (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E92094: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E9299D: g_slist_prepend (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E75790: g_source_add_poll (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4EB4612: g_io_unix_create_watch (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x4E6A443: g_io_add_watch_full (in /usr/lib64/libglib-2.0.so.0.3400.2)
> ==22811== by 0x44CD15: bt_io_listen (btio.c:283)
> ==22811== by 0x42EBDD: server_register (server.c:764)
> ==22811== by 0x45DD9E: probe_profile (adapter.c:2650)
> ==22811== by 0x466BCB: btd_profile_foreach (profile.c:599)
> ==22811==
>
> This is caused because 'audio_control_disconnected()' which removes two elements
> from the device->connected_profiles list, is called from inside a GFunc of a
> g_slist_foreach() (src/device.c:1005) leaving the list corrupted.
> ---
>
> The correct solution may be to make the "list" more robust against this kind of
> change in the first place.
>
>
> profiles/audio/manager.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/profiles/audio/manager.c b/profiles/audio/manager.c
> index 64665d7..f7b8559 100644
> --- a/profiles/audio/manager.c
> +++ b/profiles/audio/manager.c
> @@ -422,13 +422,11 @@ void audio_source_disconnected(struct btd_device *dev, int err)
> void audio_control_connected(struct btd_device *dev, int err)
> {
> device_profile_connected(dev, &avrcp_target_profile, err);
> - device_profile_connected(dev, &avrcp_remote_profile, err);
> }
>
> void audio_control_disconnected(struct btd_device *dev, int err)
> {
> device_profile_disconnected(dev, &avrcp_target_profile, err);
> - device_profile_disconnected(dev, &avrcp_remote_profile, err);
> }
>
> int audio_manager_init(GKeyFile *conf)
Is this patch still needed? We're planning to push 5.4 out soonish, so
if there's a known crash it should be fixed.
The patch you sent doesn't quite look good (which is why it's not yet
applied) since it seems unintuitive to me that a "global" AVRCP
connection state notification should only affect one specific role even
though both are represented as individual btd_profile objects.
Especially if/when we start exposing these states through D-Bus you
really do need to have the correct state for both.
So preliminarily it seems to me like this is something needing fixing in
the bluetoothd core, i.e. src/device.c. Could you send a patch for that?
Johan