[pulseaudio-discuss] [PATCH 00/13] Fix reading of freed memory
Luiz Augusto von Dentz
luiz.dentz at gmail.com
Wed Feb 13 05:01:35 PST 2013
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
More information about the pulseaudio-discuss
mailing list