1. 03 Jan, 2017 2 commits
  2. 29 Dec, 2016 1 commit
  3. 20 Dec, 2016 2 commits
    • Ahmed S. Darwish's avatar
      pacat: Write to stream only in frame-sized chunks · f7b8df50
      Ahmed S. Darwish authored and Tanu Kaskinen's avatar Tanu Kaskinen committed
      Current pacat code reads whatever available from STDIN and writes
      it directly to the playback stream. A minimal buffer is created
      for each read operation; no further reads are then allowed unless
      earlier read buffer has been fully consumed by a stream write.
      
      While quite simple, this model breaks upon the new requirements of
      writing only frame-aligned data to the stream (commits #1 and #2).
      The kernel read syscall can return a length much smaller than the
      frame-aligned size requested, leading to invalid unaligned writes.
      
      This can easily be reproduced by choosing a starved STDIN backend:
      
        pacat /dev/random    pa_stream_write() failed: EINVAL
        echo 1234 | pacat    pa_stream_write() failed: EINVAL
      
      or by playing an incomplete WAV file in raw, non-paplay, mode.
      
      So guard against such incomplete kernel reads by writing only in
      frame-aligned sizes, while caching any trailing partial frame for
      subsequent writes.
      
      Other operation modes are not affected. Non-raw paplay playback is
      handled by libsndfile, ensuring complete reads, and recording mode
      just writes to the STDOUT fd without any special needs.
      
      CommitReference #1: 22827a5e
      CommitReference #2: 150ace90
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=98475
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=77595
      
      
      
      Suggested-by: default avatarDavid Henningsson <diwic@ubuntu.com>
      Signed-off-by: default avatarAhmed S. Darwish <darwish.07@gmail.com>
      f7b8df50
    • Tanu Kaskinen's avatar
      build-sys: add LICENSE.WEBKIT to EXTRA_DIST · 65958ca8
      Tanu Kaskinen authored
      The file was missing from release tarballs.
      65958ca8
  4. 19 Dec, 2016 9 commits
    • Tanu Kaskinen's avatar
      refactor stream attaching/detaching · f8252398
      Tanu Kaskinen authored
      Move repetitive code into convenience functions. No changes in
      behaviour.
      f8252398
    • Tanu Kaskinen's avatar
      sink, source: remove some assumptions about stream "attached" state · d404d8d1
      Tanu Kaskinen authored
      Streams are detached when they are removed or moved away from a device,
      or when a filter device that they're connected to is removed or moved.
      If these cases overlap, a crash will happen due to "double-detaching".
      This can happen if a filter sink is removed, and a stream connected to
      that filter sink removes itself when its sink goes away.
      
      Here are the steps in more detail: When a filter sink is unloaded, first
      it will unlink its own sink input. This will cause the filter sink's
      input to be detached. The filter sink propagates the detachment to all
      inputs connected to it using pa_sink_detach_within_thread(). After the
      filter sink is done unlinking its own sink input, it will unlink the
      sink. This will cause at least module-combine-sink to remove its sink
      input if it had one connected to the removed filter sink. When the
      combine sink removes its sink input, that input will get detached again,
      and a crash follows.
      
      We can relax the assertions a bit, and skip the detach() call if the
      sink input is already detached.
      
      I think a better fix would be to unlink the sink before the sink input
      when unloading a filter sink - that way we could avoid the
      double-detaching - but that would be a much more complicated change. I
      decided to go with this simple fix for now.
      
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=98617
      d404d8d1
    • Tanu Kaskinen's avatar
      sink, source: add missing stream "attached" flag handling · af45c0e3
      Tanu Kaskinen authored
      The functions that call attach()/detach() for all streams on a sink or
      source didn't update the "attached" flag accordingly. Since the flag is
      only used in assertions, this omission didn't cause any harm in normal
      use.
      af45c0e3
    • Tanu Kaskinen's avatar
      sink, source: unify stream "attached" flag checking · 539eb5c2
      Tanu Kaskinen authored
      The "attached" flag is only used for asserting that the stream is in the
      expected state when attaching or detaching.
      
      Sometimes the flag was checked and updated before calling the attach or
      detach callback, and sometimes after. I think it makes more sense to
      always check it before calling the callback.
      539eb5c2
    • Tanu Kaskinen's avatar
      sink-input, source-output: set sink/source to NULL before the "unlink post" hook · 74ff1153
      Tanu Kaskinen authored
      At the time the "unlink post" hook is fired, the stream is not any more
      connected to its old device, so it makes sense to reset the sink/source
      pointer to NULL before firing the hook. If this is not done, the pointer
      may become stale during the "unlink post" hook, because
      module-bluetooth-policy does a card profile change in its "unlink post"
      callback, so even if the pointer is valid when module-bluetooth-policy's
      callback is called, it will be invalid in subsequent callbacks.
      74ff1153
    • Tanu Kaskinen's avatar
      suspend-on-idle: use earlier (safer) hooks for stream unlink notifications · c3393d27
      Tanu Kaskinen authored
      In the "unlink post" hook it's not guaranteed that the stream's old
      device exists any more, so let's use the "unlink" hook that is safer.
      For example, module-bluetooth-policy does a card profile change in the
      source-output "unlink post" hook, which invalidates the source-output's
      source pointer.
      
      When the "unlink" hook is fired, the stream is still linked to its
      device, which affects the return values of the check_suspend()
      functions. The unlinked streams should be ignored by the check_suspend()
      functions, so I had to add extra parameters to those functions.
      c3393d27
    • Tanu Kaskinen's avatar
      bluetooth-policy: do A2DP profile restoring a bit later · 2250dbfd
      Tanu Kaskinen authored
      This fixes a crash that happens if the bluetooth headset is the only
      non-monitor source in the system and the last "phone" stream dies.
      When the stream dies, the native protocol calls pa_source_output_unlink()
      and would call pa_source_output_unref() next, but without this patch,
      things happen during the unlinking, and the unreffing ends up being
      performed on a stream that is already freed.
      
      pa_source_output_unlink() fires the "unlink" hook before doing anything
      else. module-bluetooth-policy then switches the headset profile from HSP
      to A2DP within that hook. The HSP source gets removed, and at this point
      the dying stream is still connected to it, and needs to be rescued.
      Rescuing fails, because there are no other sources in the system, so the
      stream gets killed. The native protocol has a kill callback, which again
      calls pa_source_output_unlink() and pa_source_output_unref(). This is
      the point where the native protocol drops its own reference to the
      stream, but another unref call is waiting to be executed once we return
      from the original unlink call.
      
      I first tried to avoid the double unreffing by making it safe to do
      unlinking recursively, but I found out that there's code that assumes
      that once unlink() returns, unlinking has actually occurred (a
      reasonable assumption), and at least with my implementation this was not
      guaranteed. I now think that we must avoid situations where unlinking
      happens recursively. It's just too hairy to deal with. This patch moves
      the bluetooth profile switch to happen at a time when the dead stream
      isn't any more connected to the source, so it doesn't have to be
      rescued or killed.
      
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97906
      2250dbfd
    • Tanu Kaskinen's avatar
      don't assume that pa_asyncq_new() always succeeds · 60695e3d
      Tanu Kaskinen authored
      Bug 96741 shows a case where an assertion is hit, because
      pa_asyncq_new() failed due to running out of file descriptors.
      pa_asyncq_new() is used in only one place (not counting the call in
      asyncq-test): pa_asyncmsgq_new(). Now pa_asyncmsgq_new() can fail too,
      which requires error handling in many places. One of those places is
      pa_thread_mq_init(), which can now fail too, and that needs additional
      error handling in many more places. Luckily there weren't any places
      where adding better error handling wouldn't have been easy, so there are
      many changes in this patch, but they are not complicated.
      
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96741
      60695e3d
    • Tanu Kaskinen's avatar
      rtp: don't use memblocks for non-audio data · 3e52972c
      Tanu Kaskinen authored
      pa_memblockq_push() expects all memchunks to be aligned to PCM frame
      boundaries, and that means both the index and length fields of
      pa_memchunk. pa_rtp_recv(), however, used a memblock for storing both
      the RTP packet metadata and the actual audio data. The metadata was
      "removed" from the audio data by setting the memchunk index
      appropriately, so the metadata stayed in the memblock, but it was not
      played back. The metadata length is not necessarily divisible by the PCM
      frame size, which caused pa_memblock_push() to crash in an assertion
      with some sample specs, because the memchunk index was not properly
      aligned. In my tests the metadata length was 12, so it was compatible
      with many configurations, but eight-channel audio didn't work.
      
      This patch adds a separate buffer for receiving the RTP packets. As a
      result, an extra memcpy is needed for moving the audio data from the
      receive buffer to the memblock buffer.
      
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96612
      3e52972c
  5. 12 Dec, 2016 1 commit
  6. 09 Dec, 2016 1 commit
  7. 28 Nov, 2016 1 commit
    • Juha Kuikka's avatar
      bluetooth: fix race condition in BlueZ5 device disconnection · 6a786c93
      Juha Kuikka authored and Tanu Kaskinen's avatar Tanu Kaskinen committed
      SW: Pulseaudio 8.0 / BlueZ 5.39
      
      Symptoms:
      While disconnecting/reconnecting a paired bluetooth headset (LG HBS750)
      audio fails roughly on every other connection.
      
      On a failed connection "pactl list cards" shows the bluetooth device's
      card but "Active Profile: off". Issuing "pacmd set-card-profile X
      a2dp_sink" makes audio work immediately.
      
      I realized that when this happened, the previous disconnection did not
      remove the card, instead it was only configured for "Active Profile:
      off" but otherwise left in place.
      
      Upon looking at PA debug logs I saw that the transport for the a2dp_sink
      was first set into disconnected state and then into idle state. In
      "device_connection_changed_cb()" this causes the
      "pa_bluetooth_device_any_transport_connected()" return true and the
      module-bluez5-device is not unloaded.
      
      Further investigation shows that this is caused by a race of
      module-bluez5-device.c:thread_func() and
      MediaPoint1::ClearConfiguration().
      
      When the FD in thread_func() is closed (POLLHUP) an
      BLUETOOTH_MESSAGE_STREAM_FD_HUP message is sent into the main thread.
      The handler of this message unconditionally sets the transport into IDLE
      state. This is a problem if it has already been set into DISCONNECTED
      state.
      6a786c93
  8. 27 Nov, 2016 2 commits
  9. 24 Nov, 2016 2 commits
  10. 19 Nov, 2016 1 commit
  11. 17 Nov, 2016 1 commit
    • Ahmed S. Darwish's avatar
      iochannel: Strictly specify PF_UNIX ancillary data boundaries · 451d1d67
      Ahmed S. Darwish authored and Tanu Kaskinen's avatar Tanu Kaskinen committed
      Users reported audio breakage for 32-bit pulse clients connected
      to a 64-bit server over memfds. Investigating the issue further,
      the problem is twofold:
      
      1. iochannel's file-descriptor passing code is liberal in what it
         issues: produced ancillary data object's "data" section exceeds
         length field. How such an extra space is handled is a grey area
         in the POSIX.1g spec, the IETF RFC #2292 "Advanced Sockets API
         for IPv6" memo, and the cmsg(3) manpage.
      
      2. A 64-bit kernel handling of such extra space differs by whether
         the app is 64-bit or 32-bit. For 64-bit apps, the kernel
         smartly ducks the issue. For 32-bit apps, an -EINVAL is
         directly returned; that's due to a kernel CMSG header traversal
         bug in the networking stack "32-bit sockets emulation layer".
      
         Compare Linux Kernel's socket.h cmsg_nxthdr() code and the
         32-bit emulation layer version of it at net/compat.c
         cmsg_compat_nxthdr() for further info. Notice how the former
         graciously ignores incomplete CMSGs while the latter _directly_
         complains about them -- as of kernel version 4.9-rc5.
      
         (A kernel patch is to be submitted)
      
      Details:
      
      iochannel typically uses sendmsg() for passing FDs & credentials.
      >From RFC 2292, sendmsg() control data is just a heterogeneous
      array of embedded ancillary objects that can differ in length.
      Linguistically, a "control message" is an ancillary data object.
      
      For example, below is a sendmsg() "msg_control" containing two
      ancillary objects:
      
      |<---------------------- msg_controllen---------------------->|
      |                                                             |
      |<--- ancillary data object -->|<----- ancillary data object->|
      |<------- CMSG_SPACE() ------->|<------- CMSG_SPACE() ------->|
      |                              |                              |
      |<-------- cmsg_len ------->|  |<-------- cmsg_len ------->|  |
      |<------- CMSG_LEN() ------>|  |<------- CMSG_LEN() ------>|  |
      |                           |  |                           |  |
      +-----+-----+-----+--+------+--+-----+-----+-----+--+------+--+
      |cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|cmsg_|cmsg_|cmsg_|XX|cmsg_ |XX|
      |len  |level|type |XX|data[]|XX|len  |level|type |XX|data[]|XX|
      +-----+-----+-----+--+------+--+-----+-----+-----+--+----+-+--+
       ^^^^^^^ Ancil Object #1        ^^^^^^^ Ancil Object #2
               (control message)              (control message)
      ^
      |
      +--- sendmsg() "msg_control" points here
      
      Problem is, while passing FDs, iochannel's code try to avoid
      variable-length arrays by creating a single cmsg object that can
      fit as much FDs as possible:
      
        union {
          struct cmsghdr hdr;
          uint8_t data[CMSG_SPACE(sizeof(int) * MAX_ANCIL_DATA_FDS)];
        } cmsg;                                 ^^^^^^^^^^^^^^^^^^
      
      Most of the time though the number of FDs to be passed is less
      than the maximum above, thus "cmsg_len" is set to the _actual_ FD
      array size:
      
        cmsg.hdr.cmsg_len = CMSG_LEN(sizeof(int) * nfd);
                                                   ^^^
      This inconsistency tricks the kernel into thinking that we have 2
      ancillay data objects instead of one! First cmsg is valid as
      intended, but the second is instantly _corrupt_ since it has a
      cmsg_len size of 0 -- thus failing kernel's CMSG_OK() tests.
      
      For 32-bit apps on a 32-bit kernel, and 64-bit apps over a 64-bit
      one, the kernel's own CMSG header traversal macros just ignore the
      second "incomplete" cmsg. For 32-bit apps over a 64-bit kernel
      though, the kernel 32-bit socket emulation macros does not forgive
      such incompleteness and directly complains of invalid args (due to
      a subtle bug).
      
      Avoid this ugly problem, which can also bite us in a pure 64-bit
      environment if MAX_ANCIL_DATA_FDS got extended to 5 FDs, by
      setting "cmsg_data[]" array size to "cmsg_len".
      
      BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97769
      
      
      
      Signed-off-by: default avatarAhmed S. Darwish <darwish.07@gmail.com>
      451d1d67
  12. 04 Nov, 2016 1 commit
  13. 26 Oct, 2016 1 commit
  14. 13 Oct, 2016 1 commit
  15. 05 Oct, 2016 1 commit
  16. 23 Sep, 2016 1 commit
  17. 21 Sep, 2016 1 commit
  18. 20 Sep, 2016 1 commit
  19. 18 Sep, 2016 1 commit
  20. 10 Sep, 2016 2 commits
  21. 09 Sep, 2016 1 commit
  22. 05 Sep, 2016 3 commits
    • Tanu Kaskinen's avatar
      zeroconf-publish: unref D-Bus connection · c73bbee8
      Tanu Kaskinen authored
      pa_dbus_bus_get() increments the bus connection refcount, but unreffing
      the connection was never done.
      c73bbee8
    • Tanu Kaskinen's avatar
      zeroconf-publish: fix uninitialized variable · de2f5601
      Tanu Kaskinen authored
      get_icon_name() returns the icon_name variable, and without this
      change the function might exit before icon_name is initialized.
      de2f5601
    • Sylvain Baubeau's avatar
      zeroconf: use local icon for shared devices · 963b3ea6
      Sylvain Baubeau authored and Tanu Kaskinen's avatar Tanu Kaskinen committed
      systemd-hostnamed provides an icon for the machine it is running on.
      If it is running, module-zeroconf-publish uses this icon for the
      'icon-name' attribute in the Avahi properties. module-zeroconf-discover
      passes this icon to module-tunnel using the module parameter
      {sink/source}_properties.
      
      This allows to display a portable, desktop or phone instead of
      the generic sound card icon.
      963b3ea6
  23. 04 Sep, 2016 1 commit
    • Peter Meerwald-Stadler's avatar
      sample: Assert validity of sample_spec · 83f0a34e
      Peter Meerwald-Stadler authored
      passing an invalid sample_spec to
      pa_sample_size_of_format(),
      pa_frame_size(),
      pa_bytes_per_second(),
      pa_bytes_to_usec(),
      pa_usec_to_bytes()
      currently gives a result of 0
      
      this is problematic as
      (a) it leads to many potential divide-by-zero issues flagged by Coverity,
      (b) pa_sample_spec_valid() is called often and the mostly unnecessary validation
      of the sample_spec cannot be optimized away due to pa_return_val_if_fail()
      (c) nobody checks the result for 0 and the behaviour is not documented
      
      this patch replaces pa_return_val_if_fail() with pa_assert()
      
      note that this commit changes the API!
      note that pa_return_val_if_fail() strangely logs an assertion, but then happily
      continues...
      
      fixes numerious CIDs
      83f0a34e
  24. 02 Sep, 2016 2 commits