Hi Tanu, On Tue, Feb 12, 2013 at 9:36 PM, Tanu Kaskinen <tanuk at iki.fi> wrote: > Luiz reported today a bug revealed by Valgrind: > > ==16165== Invalid read of size 1 > ==16165== at 0x5170C2E: pa_idxset_string_hash_func (idxset.c:67) > ==16165== by 0x516FC77: remove_entry (hashmap.c:93) > ==16165== by 0x516FE0B: pa_hashmap_free (hashmap.c:110) > ==16165== by 0x4C77B75: device_port_free (device-port.c:60) > ==16165== by 0x4C43C7F: pa_object_unref (object.c:64) > ==16165== by 0x4C7785E: pa_device_port_unref (device-port.h:61) > ==16165== by 0x4C77D9C: pa_device_port_hashmap_free (device-port.c:96) > ==16165== by 0x4C860BE: source_free (source.c:680) > ==16165== by 0x4C43C7F: pa_object_unref (object.c:64) > ==16165== by 0x155AD610: pa_source_unref (source.h:237) > ==16165== by 0x155B1965: pa_dbusiface_stream_free (iface-stream.c:917) > ==16165== by 0x1559CE89: subscription_cb (iface-core.c:1746) > ==16165== Address 0x10dc1ec0 is 0 bytes inside a block of size 12 free'd > ==16165== at 0x4A077A6: free (vg_replace_malloc.c:446) > ==16165== by 0x4F31E3D: pa_xfree (xmalloc.c:131) > ==16165== by 0x4C376A8: pa_card_profile_free (card.c:62) > ==16165== by 0x4C38788: pa_card_free (card.c:254) > ==16165== by 0x159D6E6C: module_bluetooth_device_LTX_pa__done (module-bluetooth-device.c:2627) > ==16165== by 0x4C41C02: pa_module_free (module.c:162) > ==16165== by 0x4C41DE8: pa_module_unload (module.c:188) > ==16165== by 0x159D6391: discovery_hook_cb (module-bluetooth-device.c:2439) > ==16165== by 0x4C3EF55: pa_hook_fire (hook-list.c:106) > ==16165== by 0x13528C21: run_callback (bluetooth-util.c:668) > ==16165== by 0x1352E504: endpoint_clear_configuration (bluetooth-util.c:1859) > ==16165== by 0x1352F23B: endpoint_handler (bluetooth-util.c:2077) > > The keys of the profiles hashmap of pa_device_port were getting freed > too early. The reason was that module-dbus-protocol held a reference > to a bluetooth source, and that source in turn held a reference to a > port. When the associated card was freed, the port was not. When the > port then eventually was freed, it was referencing profile memory that > was already freed. > > I started by modifying pa_card_free(), and I found myself wanting > a pa_hashmap_remove_all() function, so I started writing that. I > wanted the free_cb of that function to match the free_cb type of > pa_hashmap_free(), but pa_hashmap_free() used pa_free2_cb_t, which was > stupid, so I changed that first. Then I made the same changes to > idxset. Patches 1-7 implement this refactoring. > > Patches 8-13 then fix the actual problem by removing refcounting from > ports and making sure that there won't be any other reference holders > when the sink or source implementor calls pa_sink/source_unref(). This > ensures that the ports and profiles are freed in the expected order > when a card is removed. > > FWIW, three of the patches are from an older patch set that I haven't > finished (the intention has been to completely fix > https://bugs.freedesktop.org/show_bug.cgi?id=45817 before submitting > the patch set, but I haven't worked on that lately). > > Tanu Kaskinen (13): > gconf: Add userdata pointer to struct module_info > gconf: Remove needless userdata function arguments > hashmap: Use pa_free_cb_t instead of pa_free2_cb_t > device-port: Remove pa_device_port_hashmap_free() > idxset: Use pa_free_cb_t instead of pa_free2_cb_t > hashmap: Add pa_hashmap_remove_all() > idxset: Add pa_idxset_remove_all() > core: Add hooks for default device changes > dbus: Track the default sink and source with hooks > dbus: Track streams with hooks > dbus: Track stream moves with hooks > bluetooth: Fix thread teardown code ordering > Remove refcounting from ports, sinks and sources > > src/modules/alsa/alsa-mixer.c | 68 +--- > src/modules/alsa/alsa-sink.c | 4 +- > src/modules/alsa/alsa-ucm.c | 12 +- > src/modules/alsa/module-alsa-card.c | 18 +- > src/modules/bluetooth/bluetooth-util.c | 4 +- > src/modules/bluetooth/module-bluetooth-device.c | 22 +- > src/modules/bluetooth/module-bluetooth-discover.c | 2 +- > src/modules/bluetooth/module-bluetooth-proximity.c | 10 +- > src/modules/dbus/iface-card.c | 10 +- > src/modules/dbus/iface-core.c | 416 +++++++++----------- > src/modules/dbus/iface-device.c | 23 +- > src/modules/dbus/iface-stream.c | 96 ++--- > src/modules/dbus/module-dbus-protocol.c | 9 +- > src/modules/gconf/module-gconf.c | 37 +- > src/modules/macosx/module-bonjour-publish.c | 2 +- > src/modules/module-augment-properties.c | 10 +- > src/modules/module-card-restore.c | 4 +- > src/modules/module-combine-sink.c | 11 +- > src/modules/module-console-kit.c | 9 +- > src/modules/module-device-manager.c | 10 +- > src/modules/module-device-restore.c | 6 +- > src/modules/module-filter-apply.c | 2 +- > src/modules/module-role-cork.c | 18 +- > src/modules/module-role-ducking.c | 19 +- > src/modules/module-stream-restore.c | 14 +- > src/modules/module-suspend-on-idle.c | 15 +- > src/modules/module-systemd-login.c | 14 +- > src/modules/module-udev-detect.c | 10 +- > src/modules/module-zeroconf-discover.c | 2 +- > src/modules/module-zeroconf-publish.c | 18 +- > src/modules/raop/module-raop-discover.c | 2 +- > src/modules/rtp/headerlist.c | 7 +- > src/modules/rtp/module-rtp-recv.c | 17 +- > src/pulse/context.c | 4 +- > src/pulse/format.c | 4 - > src/pulse/internal.h | 2 - > src/pulse/proplist.c | 7 +- > src/pulsecore/card.c | 38 +- > src/pulsecore/client.c | 4 +- > src/pulsecore/core-scache.c | 5 +- > src/pulsecore/core.c | 20 +- > src/pulsecore/core.h | 2 + > src/pulsecore/database-simple.c | 7 +- > src/pulsecore/device-port.c | 30 +- > src/pulsecore/device-port.h | 7 +- > src/pulsecore/hashmap.c | 25 +- > src/pulsecore/hashmap.h | 7 +- > src/pulsecore/idxset.c | 25 +- > src/pulsecore/idxset.h | 10 +- > src/pulsecore/memblock.c | 4 +- > src/pulsecore/modargs.c | 6 +- > src/pulsecore/module.c | 4 +- > src/pulsecore/mutex-win32.c | 2 +- > src/pulsecore/namereg.c | 2 + > src/pulsecore/protocol-cli.c | 2 +- > src/pulsecore/protocol-dbus.c | 66 +--- > src/pulsecore/protocol-esound.c | 2 +- > src/pulsecore/protocol-http.c | 2 +- > src/pulsecore/protocol-native.c | 18 +- > src/pulsecore/protocol-simple.c | 2 +- > src/pulsecore/sink-input.c | 27 +- > src/pulsecore/sink.c | 33 +- > src/pulsecore/source-output.c | 10 +- > src/pulsecore/source.c | 17 +- > 64 files changed, 521 insertions(+), 794 deletions(-) > > -- > 1.7.10.4 It looks pretty good to me. -- Luiz Augusto von Dentz
