The virNetDaemon code now only concerns itself with preventing auto
shutdown of the local daemon. Logind is now handled by the new
virInhibitor object, for QEMU, LXC and LibXL. This fixes two notable
bugs
* Running virtual networks would prevent system shutdown
* Loaded ephemeral secrets would prevent system shutdown
Fixes 9e3cc0ff5e
Fixes 37800af9a4
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This function return value is invariant since VIR_EXPAND_N check
removal in 7d2fd6e, so change its type and remove all dependent checks.
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Pavel Nekrasov <p.nekrasov@fobos-nt.ru>
Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This function return value is invariant since VIR_EXPAND_N check
removal in 7d2fd6e, so change its type and remove all dependent checks.
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Pavel Nekrasov <p.nekrasov@fobos-nt.ru>
Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This function return value is invariant since VIR_EXPAND_N check
removal in 7d2fd6e, so change its type and remove all dependent checks.
Found by Linux Verification Center (linuxtesting.org) with Svace.
Reported-by: Pavel Nekrasov <p.nekrasov@fobos-nt.ru>
Signed-off-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When moving function and/or renaming them sometimes corresponding
change to corresponding header file is not done. This leaves us
with functions that are declared in header files, but nowhere
implemented. Drop such declarations.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function the comment is referring to is
virNetServerClientPrivNew() not virNetServerClintPrivNew(). The
latter doesn't even exist.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
openssh 8.4p1 released in Sep 2020 added a feature to force use
of SSH_ASKPASS
https://man.openbsd.org/ssh.1#SSH_ASKPASS_REQUIRE
Don't strip it from the environment
Signed-off-by: Cole Robinson <crobinso@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
As of libssh commit of libssh-0.11.0~70 [1] the
ssh_channel_get_exit_status() function is deprecated and a new
one is introduced instead: ssh_channel_get_exit_state().
It's not a drop-in replacement, but it's simple enough.
Adapt our libssh handling code to this change.
1: https://git.libssh.org/projects/libssh.git/commit/?id=04d86aeeae73c78af8b3dcdabb2e588cd31a8923
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
The TLS cert validation logic will be reused for the new impl of the
virt-pki-validate tool.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This will facilitate moving much of the code into a new file in the
subsequent commit.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We'll want to access these paths from outside the TLS context code,
so split them into a standalone file.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
We never release the reference on the GSource created for
interrupting the main loop, nor do we remove it from the
main context if our thread is woken up prior to the wakeup
callback firing.
This can result in a leak of GSource objects, along with an
ever growing list of GSources attached to the main context,
which will gradually slow down execution of the loop, as
several operations are O(N) for the number of attached GSource
objects.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Users are seeing periodic segfaults from libvirt client apps,
especially thread heavy ones like virt-manager. A typical
stack trace would end up in the virNetClientIOEventFD method,
with illegal access to stale stack data. eg
==238721==ERROR: AddressSanitizer: stack-use-after-return on address 0x75cd18709788 at pc 0x75cd3111f907 bp 0x75cd181ff550 sp 0x75cd181ff548
WRITE of size 4 at 0x75cd18709788 thread T11
#0 0x75cd3111f906 in virNetClientIOEventFD /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:1634:15
#1 0x75cd3210d198 (/usr/lib/libglib-2.0.so.0+0x5a198) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#2 0x75cd3216c3be (/usr/lib/libglib-2.0.so.0+0xb93be) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#3 0x75cd3210ddc6 in g_main_loop_run (/usr/lib/libglib-2.0.so.0+0x5adc6) (BuildId: 0a2311dfbbc6c215dc36f4b6bdd2b4b6fbae55a2)
#4 0x75cd3111a47c in virNetClientIOEventLoop /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:1722:9
#5 0x75cd3111a47c in virNetClientIO /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2002:10
#6 0x75cd3111a47c in virNetClientSendInternal /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2170:11
#7 0x75cd311198a8 in virNetClientSendWithReply /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclient.c:2198:11
#8 0x75cd31111653 in virNetClientProgramCall /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/rpc/virnetclientprogram.c:318:9
#9 0x75cd31241c8f in callFull /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/remote/remote_driver.c:6054:10
#10 0x75cd31241c8f in call /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/remote/remote_driver.c:6076:12
#11 0x75cd31241c8f in remoteNetworkGetXMLDesc /usr/src/debug/libvirt/libvirt-10.2.0/build/src/remote/remote_client_bodies.h:5959:9
#12 0x75cd31410ff7 in virNetworkGetXMLDesc /usr/src/debug/libvirt/libvirt-10.2.0/build/../src/libvirt-network.c:952:15
The root cause is a bad assumption in the virNetClientIOEventLoop
method. This method is run by whichever thread currently owns the
buck, and is responsible for handling I/O. Inside a for(;;) loop,
this method creates a temporary GSource, adds it to the event loop
and runs g_main_loop_run(). When I/O is ready, the GSource callback
(virNetClientIOEventFD) will fire and call g_main_loop_quit(), and
return G_SOURCE_REMOVE which results in the temporary GSource being
destroyed. A g_autoptr() will then remove the last reference.
What was overlooked, is that a second thread can come along and
while it can't enter virNetClientIOEventLoop, it will register an
idle source that uses virNetClientIOWakeup to interrupt the
original thread's 'g_main_loop_run' call. When this happens the
virNetClientIOEventFD callback never runs, and so the temporary
GSource is not destroyed. The g_autoptr() will remove a reference,
but by virtue of still being attached to the event context, there
is an extra reference held causing GSource to be leaked. The
next time 'g_main_loop_run' is called, the original GSource will
trigger its callback, and access data that was allocated on the
stack by the previous thread, and likely SEGV.
To solve this, the thread calling 'g_main_loop_run' must call
g_source_destroy, immediately upon return, to guarantee that
the temporary GSource is removed.
CVE-2024-4418
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Reported-by: Martin Shirokov <shirokovmartin@gmail.com>
Tested-by: Martin Shirokov <shirokovmartin@gmail.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
While the C API entry points will validate non-negative lengths
for various parameters, the RPC server de-serialization code
will need to allocate memory for arrays before entering the C
API. These allocations will thus happen before the non-negative
length check is performed.
Passing a negative length to the g_new0 function will usually
result in a crash due to the negative length being treated as
a huge positive number.
This was found and diagnosed by ALT Linux Team with AFLplusplus.
CVE-2024-2494
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Found-by: Alexandr Shashkin <dutyrok@altlinux.org>
Co-developed-by: Alexander Kuznetsov <kuznetsovam@altlinux.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
meson wraps python scripts already on win32, so we end up with these
failing commands:
[185/868] Generating src/rpc/virnetprotocol.h with a custom command
FAILED: src/rpc/virnetprotocol.h
"sh" "libvirt/scripts/meson-python.sh" "F:/msys64/ucrt64/bin/python3.EXE" "F:/msys64/ucrt64/bin/python.exe" "libvirt/scripts/rpcgen/main.py" "--mode=header" "../src/rpc/virnetprotocol.x" "src/rpc/virnetprotocol.h"
SyntaxError: Non-UTF-8 code starting with '\x90' in file F:/msys64/ucrt64/bin/python.exe on line 1, but no encoding declared; see https://peps.python.org/pep-0263/ for details
The issue was introduced in a62486b95f commit.
These changes are similar as e06beacec2 commit.
Signed-off-by: Biswapriyo Nath <nathbappai@gmail.com>
The first thread to issue a client RPC request will own the event
loop execution, sitting in the virNetClientIOEventLoop function.
It releases the client lock while running:
virNetClientUnlock()
g_main_loop_run()
virNetClientLock()
If a second thread arrives with an RPC request, it will queue it
for the first thread to process. To inform the first thread that
there's a new request it calls g_main_loop_quit() to break it out
of the main loop.
This works if the first thread is in g_main_loop_run() at that
time. There is a small window of opportunity, however, where
the first thread has released the client lock, but not yet got
into g_main_loop_run(). If that happens, the wakeup from the
second thread is lost.
This patch deals with that by changing the way the wakeup is
performed. Instead of directly calling g_main_loop_quit(), the
second thread creates an idle source to run the quit function
from within the first thread. This guarantees that the first
thread will see the wakeup.
Tested by: Fima Shevrin <efim.shevrin@virtuozzo.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Denis V. Lunev <den@openvz.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
virNetServerAddService() return value is invariant, so change it
type and remove all dependent checks.
Signed-off-by: Artem Chernyshev <artem.chernyshev@red-soft.ru>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
If libvirt is built in client only mode, the libvirtd/virtqemud/etc
daemons won't exist. If the client is told to connect to a local
hypervisor, it'll see the socket doesn't exist, try to spawn the
daemon and then re-try connecting to the socket for a few seconds.
Ultimately this will fail because the daemon doesn't exist and the
user gets an error message
error: Failed to connect socket to '/run/user/1000/libvirt/virtqemud-sock': No such file or directory
technically this is accurate, but it doesn't help identify the root
cause. With this change it will now report
error: binary 'virtqemud' does not exist in $PATH: No such file or directory
and will skip all the socket connect retries
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
gpg-agent can be used instead of ssh-agent to authenticate
against an SSH server, but in order to do so the GPG_TTY and
TERM environment variables need to be passed through.
For obvious reasons, we avoid doing that when no_tty=1 is found
in the connection URI.
https://bugs.debian.org/843863https://gitlab.com/libvirt/libvirt/-/merge_requests/290
Thanks: Guilhem Moulin <guilhem@guilhem.org>
Thanks: Kunwu Chan <chentao@kylinos.cn>
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Currently some, but not all, methods have a call to the
xdr_free function, for the 'ret' variable. This is done
on methods where there are complex structs containing
allocated memory. In other cases the structs contain
allocated memory, but the pointer is stolen, so xdr_free
is not called. In other cases no allocated memory is
present, so xdr_free.
This is hard to reason about, because the definition of
the struct is not visible in the client stubs.
Switch to use g_auto() for the 'ret' variable, which
means 'xdr_free' is always going to be called. Some
places now need to use g_steal_pointer as a result.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This replaces use of 'rpcgen' with our new python impl of
the RPC code generator. Since the new impl generates code
that matches our style/coding rules, and does not contain
long standing bugs, we no longer need to post-process the
output.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The current RPC code is post-processed to introduce an
intermediate variable, rather than casting directly
to char ** at time of use. This is said to be a workaround
for type-puning warnings that the compiler emitted.
Neither GCC or CLang emit any warnings for the code in
question today, across any of the architectures we
test in CI. Thus it is presumed that somewhere in the
15 years since the workaround was done, the compilers
have got smarter and do the right thing.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This is the default for the version of rpcgen shipped with
Linux distributions, but the one in macOS and possibly others
default to K&R C, which modern compilers don't appreciate.
Luckily, all versions of rpcgen shipped with our target
platforms seem to support the -C option.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Error messages are exempt from the 80 columns rule. Move them
onto one line.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
There are some cases left after previous commit which were not
picked up by coccinelle. Mostly, becuase the spatch was not
generic enough. We are left with cases like: two variables
declared on one line, a variable declared in #ifdef-s (there are
notoriously difficult for coccinelle), arrays, macro definitions,
etc.
Finish what coccinelle started, by hand.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
This is a more concise approach and guarantees there is
no time window where the struct is uninitialized.
Generated using the following semantic patch:
@@
type T;
identifier X;
@@
- T X;
+ T X = { 0 };
... when exists
(
- memset(&X, 0, sizeof(X));
|
- memset(&X, 0, sizeof(T));
)
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Ideally, these would be fixed by coccinelle (see next commit),
but because of various reasons they aren't. Fix them manually.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Instead of suggesting to zero structs out using memset() we
should suggest initializing structs with zero initializer.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
There are couple of variables that are declared at function
beginning but then used solely within a block (either for() loop
or if() statement). And just before their use they are zeroed
explicitly using memset(). Decrease their scope, use struct zero
initializer and drop explicit memset().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Claudio Fontana <cfontana@suse.de>
Helped to debug next patch use-after-free.
Signed-off-by: Oleg Vasilev <oleg.vasilev@virtuozzo.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
None of our daemons use select(), so it is safe to raise the max file
limit to its maximum on startup.
https://gitlab.com/libvirt/libvirt/-/issues/489
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In one of its commits [1] libssh2 changed the 'text' member of
LIBSSH2_USERAUTH_KBDINT_PROMPT struct from 'char' to 'unsigned
char'. But we g_strdup() the member in order to fill 'prompt'
member of virConnectCredential struct. Typecast the value to
avoid warnings. Also, drop @prompt variable, as it's needless.
1: 83853f8aea
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
The function does not exist on win32.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
The platform check which determines when to apply the fixups mentions
all officially supported build targets (per docs/platforms.rst) thus
it's not really necessary.
Additionally while not explicitly written as supported the check does
not work properly when building with the MinGW toolchain on Windows as
it does not apply the needed transformations. They are necessary
there the same way as with MinGW on Linux.
https://gitlab.com/libvirt/libvirt/-/issues/453
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The separate API perms XML is no longer used. Remove the support for
generating it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Both the object name and permission name in ACL use '-' instead of '_'
separator when referring to them in the docs or even when used inside of
polkit. Unfortunately the generators used for generating our docs don't
honour this in certain cases which would result in broken names in the
API docs (once they will be generated).
Rename both object and permission name to use dash and reflect that in
the anchor names in the documentation.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The warning about max_client_requests is hit inside virtlogd every time
a VM starts which spams the logs.
Emit the warning only when the client request limit is not 1 and add a
warning into the daemon config to not configure it too low instead.
Fixes: 031878c236
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2145188
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
virNetServerClientDispatchRead checked the return value but it's not
necessary any more as it can't return NULL nowadays.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
In a recent commit of v9.0.0-104-g0211e430a8 I've turned all args
vars in src/remote/remote_driver.c to be initialized wit {0}.
What I've missed was the generated code.
Do what we've done in v9.0.0-13-g1c656836e3 and init also args,
not just ret.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Commit f007940cb2 tried to change the error message so that it is unified
later in 35afa1d2d6, but various rewrites missed this particular error message
which does not make sense. Fix it so that it is the same as the other two
messages checking the same thing in this file.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2033879
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function can't return NULL to the callers so it doesn't make sense
to check it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'sess->authPath' is modified before locking the 'sess' object.
Additionally on failure of 'virAuthGetConfigFilePathURI' 'sess' would be
unlocked even when it was not yet locked.
Fixes: 6917467c2b
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
The function can't return NULL to the callers so it doesn't make sense
to check it.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
'sess->authPath' is modified before locking the 'sess' object.
Additionally on failure of 'virAuthGetConfigFilePathURI' 'sess' would be
unlocked even when it was not yet locked.
Fixes: 273745b431
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
virAuthGetPasswordPath can return the same password over and over if
it's configured in the config. We rather want to try that only the first
time and then ask the user instead.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
