During a move sink_input->sink is not valid. This leads to a crash when
sink_input_set_rate() is called from the moving() callback. This patch
fixes the problem.
The previous patch assumed constant port latency offsets. The offsets can
however be changed by the user, therefore these changes need to be tracked
as well. This patch adds the necessary hooks.
Also the print_msg argument was removed from update_minimum_latency() and
update_latency_boundaries() because the message should always be logged.
The old code makes no sense to me. Why would multiple references mean
that a previously read-only memblock is suddenly writable? I'm pretty
sure that the original intention was to treat multi-referenced blocks
as read-only. I don't have any examples where the old code would have
caused bad behaviour, however.
Currently internal > speaker > headphone and pci > usb > bluetooth.
Invert both of these sets, with the reasoning that a headphone and
speakers are something that a user has actively attached and should
therefore get a higher priority. The same reasoning is applied for
the bus type, i.e. bluetooth and usb should be higher than pci,
because they most likely have been actively attached be a user.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=99222
If pa_sink_input_cork() or pa_source_output_cork() were called without a sink
or source attached, the calls would crash pulseaudio.
This patch fixes the problem, so that a source output or sink input can still
be corked or uncorked while source or sink are invalid. This is needed to
correct the corking logic in module-loopback.
document behaviour of pa_shared_remove() in case name does not exist
Coverity ID: #1380672
thanks to Georg Chini for suggesting to swap patch title and commit message
Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
for example, in case HAVE_MEMFD is #undef, checking with #if HAVE_MEMFD
gives a warning (gcc 5.4.1, Ubuntu)
pulsecore/shm.c: In function 'sharedmem_create':
pulsecore/shm.c:208:5: warning: "HAVE_MEMFD" is not defined [-Wundef]
#if HAVE_MEMFD
use #ifdef or #if defined() to check for presence of a #define
Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Clang didn't like the variable length array:
pulsecore/iochannel.c:358:17: error: fields must have a constant size:
'variable length array in structure' extension will never be supported
uint8_t data[CMSG_SPACE(sizeof(int) * nfd)];
^
Commit 451d1d6762 introduced the variable length array in order to have
the correct value in msg_controllen. This patch reverts that commit and
uses a different way to achieve the same goal.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=99458
the modeling file help to avoid false positives and increase scanning
accuracy by explaining code Coverity can't see (out of tree libraries);
the model file must be uploaded by an admin to:
https://scan.coverity.com/projects/pulseaudio?tab=analysis_settings
the pa_assert_se() macro needs to be rewritten for Coverity so that
the assignment is not declared a side-effect
Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
This serves to explicitly document the various cases we deal with in
pa_sink_update_rate()/pa_source_update_rate() rather than have some of
them hidden behind the initialisation of desired_rate.
This adds an "avoid-resampling" option to daemon.conf that makes the
daemon try to use the stream sample rate if possible (the device needs
to support it, which currently only ALSA does), and there should not be
any other stream connected).
This should enable some of the "audiophile" use-cases where users wish
to play high sample rate audio files without resampling.
We still will do conversion if sample formats don't match, though. This
means that if you want to play 96 kHz/24 bit audio without any
modification the default format will need to be set to be 24-bit as
well. This will force all streams to be upconverted, which, other than
the wasted resources, should be relatively harmless.
This macro compares if the given two strings, with the maximum length
of n, are equal. Useful for strings that are not NULL-terminated.
Reviewed-by: Anton Lundin <glance@acc.umu.se>
pa_socket_client_new_string() did not work as expected when an IPv6
address string like "2001:db8::1" is passed as the "name" parameter.
This is because the name parameter is then passed to pa_parse_address(),
which thinks the last colon as a separator between hostname (or address)
and a port number. To prevent pa_parse_address() from doing this, an IPv6
address must be bracketed with "[]" (e.g. "[2001:db8::1]"). [1]
This patch fixes pa_socket_client_new_string() so that it internally
adds brackets to an IPv6 address. This decision is based on a
discussion at [2].
[1]: http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-October/022010.html
[2]: http://lists.freedesktop.org/archives/pulseaudio-discuss/2014-November/022401.html
Reviewed-by: Anton Lundin <glance@acc.umu.se>
The function isn't used anywhere else than memblockq-test. Also, the
function is confusing, because it defines "missing" differently than
pa_memblockq_pop_missing(). pa_memblockq_missing() calculated the
missing amount like this:
missing = tlength - length,
where "length" is the current queue length. pa_memblockq_pop_missing(),
on the other hand, calculates the missing amount like this:
missing = tlength - length - requested,
where "requested" is an internal variable that keeps track of how much
the server has requested data from the client and how much of the
requests are yet to be fulfilled by the client.
memblockq-test is broken at the moment, because it assumes that
pa_memblockq_pop_missing() calculates "missing" the same way that
pa_memblockq_missing() used to calculate it. A patch for fixing that
will follow.
This reverts commit 74251f0786.
The reverted commit was not intended to make any behavioral changes, but
it broke at least the case where a client writes more data than the
server has requested.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=99211
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
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.
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.
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.
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.
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
When unloading a module, lt_dlclose() may remove the module from memory.
If a module unloads itself, it's not safe to call lt_dlclose()
synchronously from pa_module_unload(), because the execution may return
to the module code that was removed from memory. To avoid this
situation, let's postpone lt_dlclose() until it's safe to call it.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96831
Although such 9.0 clients support memfd transport, they have an
iochannel bug that would break memfd audio if they're run in 32
bit mode over a 64-bit kernel. Influence them to use the POSIX
shared memory model instead.
Also bump the protocol version to exclusively mark such v9.0
libraries. Check commit 451d1d6762 for further details.
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=97769
Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
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: Ahmed S. Darwish <darwish.07@gmail.com>
PA_PAGE_SIZE using sysconf() may return a negative number
CID 1137925, CID 1137926, CID 1138485
instead of calling sysconf() directly, add function pa_page_size()
which uses the guestimate 4096 in case sysconf(_SC_PAGE_SIZE) fails
using PA_ONCE to only evaluate sysconf() once
pa_ncpu() is supposed to report the number of processors available on
the system. For that, it currently calls sysconf(_SC_NPROCESSORS_CONF).
However, since the operating system can disable individual processors,
we should call sysconf(_SC_NPROCESSORS_ONLN) to determine the number
of processors currently available [1]. Consequently, the once-test will
fail since pthread_setaffinity_np() is called with CPUs that are
currently not available.
It might also be advisable to change the code in the future to use CPU
sets on Linux as even the suggested change is not 100% safe but at least
it improves over the existing code. If PulseAudio was to be run in a CPU
set [2], the number of processors available to PulseAudio could be even
less than the number of CPUs currently online (_SC_NPROCESSORS_CONF).
[1] https://www.gnu.org/software/libc/manual/html_node/Processor-Resources.html
[2] http://man7.org/linux/man-pages/man7/cpuset.7.html
BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=96809
Signed-off-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
Doesn't really affect logic, but Coverity reports this as dead-code, and
I figure it makes sense to be consistent about our use of HAVE_MEMFD.
CID: 1352045
It was a very confusing state variable that required a lot of
fiddling. It was also redundant in that it can be computed from
the other variables, removing any risk of it getting out of sync.
In the same spirit, make sure "requested" also always contains a
sane value, even though it may not be used by every caller.
Having it handled in the callers proved to be a poor fit as it
became difficult to handle a shrinking minreq sanely. It could end
up in a state where the request was never sent downstream to the
client.
The behaviour is to leave the value unchanged. The idea is to init the value
with a default before the call and not treat a missing value as error. That
way, only parsing errors or validating errors actually return error codes.
I want module-alsa-card to set the availability of unavailable
profiles before the initial card profile gets selected, so that the
selection logic can use correct availability information.
module-alsa-card initializes the jack state after calling
pa_card_new(), however, and the profile selection happens in
pa_card_new(). This patch solves that by moving parts of pa_card_new()
to pa_card_choose_initial_profile() and pa_card_put().
pa_card_choose_initial_profile() applies the profile selection policy,
so module-alsa-card can first call pa_card_new(), then initialize the
jack state, and then call pa_card_choose_initial_profile(). After that
module-alsa-card can still override the profile selection policy, in
case module-alsa-card was loaded with the "profile" argument. Finally,
pa_card_put() finalizes the card creation.
An alternative solution would have been to move the jack
initialization to happen before pa_card_new() and use pa_card_new_data
instead of pa_card in the jack initialization code, but I disliked
that idea (I want to get rid of the "new data" pattern eventually).
The order in which the initial profile policy is applied is reversed
in this patch. Previously the first one to set it won, now the last
one to set it wins. I think this is better, because if you have N
parties that want to set the profile, we avoid checking N times
whether someone else has already set the profile.
There is currently no use for allowing modules to cancel card creation,
and I don't see need for that in the future either. Let's simplify
things by removing the failure handling code.
This allows us to parse an extra set of modargs to tack on to an
existing set. Duplicates in the second set are ignored, since this fits
our use best. In the future, this could be extended to support different
merge modes (ignore dupes vs. replace with dupes), but I've left this
out since there isn't a clear need and it would be dead code for now.
