[PATCH 00/13] Fix reading of freed memory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Audio Users]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux