Now that the memory disposal is handled automatically we can simplify
the cleanup paths. In this case it's not as simple as sometimes the
value of the called function is returned.
While at it fix the initialization value of the returned variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Refactor code paths which clear strings on cleanup paths to use the
automatic helper.
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
If 2 threads call abort for example then one of them
will hang because client will send 2 abort messages and
server will reply only on first of them, the second will be
ignored. And on server reply client changes the state only
one of abort message to complete, the second will hang forever.
There are other similar issues.
We should complete all messages waiting reply if we got
error or expected abort/finish reply from server. Also if one
thread send finish and another abort one of them will win
the race and server will either abort or finish stream. If
stream is aborted then thread requested finishing should report
error. In order to archive this let's keep stream closing reason
in @closed field. If we receive VIR_NET_OK message for stream
then stream is finished if oldest (closest to queue end) message
in stream queue is finish message and stream is aborted if oldest
message is abort message. Otherwise it is protocol error.
By the way we need to fix case of receiving VIR_NET_CONTINUE
message. Now we take oldest message in queue and check if
this is dummy message. If one thread first sends abort and
second thread then receives data then oldest message is abort
message and second thread won't be notified when data arrives.
Let's find oldest dummy message instead.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
If we call virStreamFinish and virStreamAbort from 2 distinct
threads for example we can have access to freed memory.
Because when virStreamFinish finishes for example virStreamAbort
yet to be finished and it access virNetClientStreamPtr object
in stream->privateData.
Also it does not make sense to clear @driver field. After
stream is finished/aborted it is better to have appropriate
error message instead of "unsupported error".
This commit reverts [1] or virNetClientStreamPtr and
virStreamPtr will never be unrefed due to cyclic dependency.
Before this patch we don't have leaks because all execution
paths we call virStreamFinish or virStreamAbort.
[1] 8b6ffe40 : virNetClientStreamNew: Track origin stream
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
This mixing errors and EOF condition in one flag is odd.
Instead let's check st->err.code where appropriate.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Checking virNetClientStreamRaiseError without client lock
is racy which is fixed in [1] for example. Thus let's remove such checks
when we are sending message to server. And in other cases
(like virNetClientStreamRecvHole for example) let's move the check
into client stream code.
virNetClientStreamRecvPacket already have stream lock so we could
introduce another error checking function like virNetClientStreamRaiseErrorLocked
but as error is set when both client and stream lock are hold we
can remove locking from virNetClientStreamRaiseError because all
callers hold either client or stream lock.
Also let's split virNetClientStreamRaiseErrorLocked into checking
state function and checking message send status function. They are
same yet.
[1] 1b6a29c21: rpc: fix race on stream abort/finish and server side abort
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Stream server error is not propagated if thread does not have the buck.
In case we have the buck we are ok due to the code added in [1].
Let's check for stream error on all paths. Now we don't need
to raise error in virNetClientCallDispatchStream.
Old code reported error only if the first message in wait
queue awaits reply. It is odd as depends on wait queue
situation. For example if we have only TX
message in queue and in one iteration loop both send the
message and receive error then thread sending TX message did
not receive the error. Next if we have RX message (first)
and TX message (second) in queue and in one iteration
loop both send the TX message and receive error then
thread sending TX message received error. In short
it was inconsistent. Let's report error whenever
we received it and for every type of message as it makes
sense to report errors as early as possible.
[1] 16c6e2b41: Fix propagation of RPC errors from streams
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
In next patches we'll add stream state checks to this
function that applicable to all call paths. This is handy
place because we hold client lock here.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Stream abort/finish can hang because we can receive abort message
from server and yet sent abort/finish message to server. The latter
will not be answered ever because after server sends abort message
it forgets the stream and messages for unknown stream are simply ignored.
We check for stream error at the very beginning of remoteStreamFinish/remoteStreamAbort
but stream error can be set after the check in another thread operating
on stream. Let's check for stream error under client lock similar
to what's done in [1].
[1] 833b901cb: stream: Check for stream EOF
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Missing semicolon at the end of macros can confuse some analyzers
(like cppcheck <filename>). VIR_ONCE_GLOBAL_INIT is almost
exclusively called without an ending semicolon, but let's
standardize on using one like the other macros.
Add a dummy struct definition at the end of the macro, so
the compiler will require callers to add a semicolon.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
if virNetClientNew finishes with error before sock is set
to client object then sock does not get unrefed. This is
unexpected by function clients like virNetClientNewUNIX.
Let's make sure sock gets unrefed on any error path.
Next some clients like virNetClientNewLibSSH2 try to unref
sock on virNetClientNew errors. This is not correct even
before this patch because in some cases virNetClientNew
unrefed sock on error path by itself. Let's give up
sock managment to virNetClientNew entirely.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Require that all headers are guarded by a symbol named
LIBVIRT_$FILENAME
where $FILENAME is the uppercased filename, with all characters
outside a-z changed into '_'.
Note we do not use a leading __ because that is technically a
namespace reserved for the toolchain.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This introduces a syntax-check script that validates header files use a
common layout:
/*
...copyright header...
*/
<one blank line>
#ifndef SYMBOL
# define SYMBOL
....content....
#endif /* SYMBOL */
For any file ending priv.h, before the #ifndef, we will require a
guard to prevent bogus imports:
#ifndef SYMBOL_ALLOW
# error ....
#endif /* SYMBOL_ALLOW */
<one blank line>
The many mistakes this script identifies are then fixed.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
The make_nonnull_XXX methods can all fail due to OOM but this was being
silently ignored and thus also not checked by callers. Make the methods
propagate errors and use ATTRIBUTE_RETURN_CHECK to force callers to deal
with it.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
In many files there are header comments that contain an Author:
statement, supposedly reflecting who originally wrote the code.
In a large collaborative project like libvirt, any non-trivial
file will have been modified by a large number of different
contributors. IOW, the Author: comments are quickly out of date,
omitting people who have made significant contribitions.
In some places Author: lines have been added despite the person
merely being responsible for creating the file by moving existing
code out of another file. IOW, the Author: lines give an incorrect
record of authorship.
With this all in mind, the comments are useless as a means to identify
who to talk to about code in a particular file. Contributors will always
be better off using 'git log' and 'git blame' if they need to find the
author of a particular bit of code.
This commit thus deletes all Author: comments from the source and adds
a rule to prevent them reappearing.
The Copyright headers are similarly misleading and inaccurate, however,
we cannot delete these as they have legal meaning, despite being largely
inaccurate. In addition only the copyright holder is permitted to change
their respective copyright statement.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
This is a simple removal of a duplicated check of the return of the
filter function. There is a nested conditional checking exactly the same
thing since commit c9ede1cf removed the (ret > 0) check condition.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1631606
Changes made to manage and utilize a secondary connection
driver to APIs outside the scope of the primary connection
driver have resulted in some confusion processing polkit rules
since the simple "access denied" error message doesn't provide
enough of a clue when combined with the "authentication failed:
access denied by policy" as to which connection driver refused
or failed the ACL check.
In order to provide some context, let's modify the existing
"access denied" error returned from the various vir*EnsureACL
API's to provide the connection driver name that is causing
the failure. This should provide the context for writing the
polkit rules that would allow access via the driver, but yet
still adhere to the virAccessManagerSanitizeError commentary
regarding not telling the user why access was denied.
Signed-off-by: John Ferlan <jferlan@redhat.com>
This reverts commit ccc72d5cbd.
Based on upstream comment to a follow-up patch, this didn't take the
right approach and the right thing to do is revert and rework.
Signed-off-by: John Ferlan <jferlan@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1631606
Changes made to manage and utilize a secondary connection
driver to APIs outside the scope of the primary connection
driver have resulted in some confusion processing polkit rules
since the simple "access denied" error message doesn't provide
enough of a clue when combined with the "authentication failed:
access denied by policy" as to which connection driver refused
or failed the ACL check.
In order to provide some context, let's modify the existing
"access denied" error returne from the various vir*EnsureACL
API's to provide the connection driver name that is causing
the failure. This should provide the context for writing the
polkit rules that would allow access via the driver.
Signed-off-by: John Ferlan <jferlan@redhat.com>
ACKed-by: Michal Privoznik <mprivozn@redhat.com>
The result of libssh2_userauth_password is being assigned to 'ret' in
one branch and 'rc' in the other branch. Checks are all done against the
'ret' variable, so one branch never does the correct check.
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
If the same source gets built twice ('build same source on different
hosts at different times') the resulting files may differ.
Fix this by sorting the hash keys before usage.
Signed-off-by: Olaf Hering <olaf@aepfle.de>
All of the ones being removed are pulled in by internal.h. The only
exception is sanlock which expects the application to include <stdint.h>
before sanlock's headers, because sanlock prototypes use fixed width
int, but they don't include stdint.h themselves, so we have to leave
that one in place.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
It doesn't really make sense for us to have stdlib.h and string.h but
not stdio.h in the internal.h header.
Signed-off-by: Erik Skultety <eskultet@redhat.com>
Acked-by: Michal Privoznik <mprivozn@redhat.com>
Now that the virAuthGet*Path API's generate all the error messages
we can remove them from the callers.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Marcos Paulo de Souza <marcos.souza.org@gmail.com>
Semantically, there is no difference between an uninitialized worker
pool and an initialized worker pool with zero workers. Let's allow the
worker pool to be initialized for max_workers=0 as well then which
makes the API more symmetric and simplifies code. Validity of the
worker pool is delegated to virThreadPoolGetMaxWorkers instead.
This patch fixes segmentation faults in
virNetServerGetThreadPoolParameters and
virNetServerSetThreadPoolParameters for the case when no worker pool
is actually initialized (max_workers=0).
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Consider creating a listener socket from a hostname that resolves to
multiple addresses. It might be the case that the hostname resolves to
both an IPv4 and IPv6 address because it is reachable over both
protocols, but the IPv6 connectivity is provided off-host. In such a
case no local NIC will have IPv6 and so bind() would fail with the
EADDRNOTAVAIL errno. Thus it should be treated as non-fatal as long as
at least one socket was succesfully bound.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When reporting socket/bind failures we want to ensure any fatal error
reported is as accurate as possible. We'll prefer reporting a bind()
errno over a socket() errno, because if socket() works but bind() fails
that is a more significant event.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently, the functions return a pointer to the
destination buffer on success or NULL on failure.
Not only does this kind of error handling look quite
alien in the context of libvirt, where most functions
return zero on success and a negative int on failure,
but it's also somewhat pointless because unless there's
been a failure the returned pointer will be the same
one passed in by the user, thus offering no additional
value.
Change the functions so that they return an int
instead.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The include guard should match the file name and comment.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
@srv must be unlocked for the call virNetServerProcessMsg otherwise a
deadlock can occur.
Since the pointer 'srv->workers' will never be changed after
initialization and the thread pool has it's own locking we can release
the lock of 'srv' earlier. This also fixes the deadlock.
Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Replace instances where we previously called virGetLastError just to
either get the code or to check if an error exists with
virGetLastErrorCode to avoid a validity pre-check.
Signed-off-by: Ramy Elkest <ramyelkest@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Now that GnuTLS is a requirement, we can drop a lot of
conditionally built code. However, not all ifdef-s can go because
we still want libvirt_setuid to build without gnutls.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Since GnuTLS is required there is no way to go with !WITH_GNUTLS
branch and just distribute these files.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
After version 0.7.5, libssh deprecated the function scope
ssh_get_publickey() and moved to ssh_get_server_publickey(). So, Libvirt
is failing to compile using this new function name.
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
Rather than have virJSONValueArraySize return a -1 when the input
is not an array and then splat an error message, let's check for
an array before calling and then change the return to be a size_t
instead of ssize_t.
That means using the helper virJSONValueIsArray as well as using a
more generic error message such as "Malformed <something> array".
In some cases we can remove stack variables and when we cannot,
those variables should be size_t not ssize_t. Alter a few references
of if (!value) to be if (value == 0) instead as well.
Some callers can already assume an array is being worked on based
on the previous call, so there's less to do.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
So far we are repeating the following lines over and over:
if (!(virSomeObjectClass = virClassNew(virClassForObject(),
"virSomeObject",
sizeof(virSomeObject),
virSomeObjectDispose)))
return -1;
While this works, it is impossible to do some checking. Firstly,
the class name (the 2nd argument) doesn't match the name in the
code in all cases (the 3rd argument). Secondly, the current style
is needlessly verbose. This commit turns example into following:
if (!(VIR_CLASS_NEW(virSomeObject,
virClassForObject)))
return -1;
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Whenever we declare a new object the first member of the struct
has to be virObject (or any other member of that family). Now, up
until now we did not care about the name of the struct member.
But lets unify it so that we can do some checks at compile time
later.
The unified name is 'parent'.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Future commits rely on the presence of this callback.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Calling a push_privconn method to directly push the connection object
name into the arg list is inconvenient. Refactor so that we acquire
the connection variable name upfront, and push it to the arg list
separately. This allows various hardcoded usage of "priv->conn" to
be parameterized.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
When preparing for migration, the libxl driver creates a new TCP listen
socket for the incoming migration by calling virNetSocketNewListenTCP,
passing the destination host name. virNetSocketNewListenTCP calls
virSocketAddrParse to check if the host name is a wildcard address, in
which case it avoids adding the AI_ADDRCONFIG flag to the hints passed to
getaddrinfo. If the host name is not an IP address, virSocketAddrParse
reports an error
error : virSocketAddrParseInternal:121 : Cannot parse socket address
'myhost.example.com': Name or service not known
But virNetSocketNewListenTCP succeeds regardless and the overall migration
operation succeeds.
Introduce virSocketAddrParseAny and use it when simply testing if a host
name/addr is parsable.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
If max_workers is set to zero, then the worker thread pool won't be
created, so when serializing state for pre-exec we must set various
parameters to zero.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently if the virNetServer instance is created with max_workers==0 to
request a non-threaded dispatch process, we deadlock during dispatch
#0 0x00007fb845f6f42d in __lll_lock_wait () from /lib64/libpthread.so.0
#1 0x00007fb845f681d3 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2 0x000055a6628bb305 in virMutexLock (m=<optimized out>) at util/virthread.c:89
#3 0x000055a6628a984b in virObjectLock (anyobj=<optimized out>) at util/virobject.c:435
#4 0x000055a66286fcde in virNetServerClientIsAuthenticated (client=client@entry=0x55a663a7b960)
at rpc/virnetserverclient.c:1565
#5 0x000055a66286cc17 in virNetServerProgramDispatchCall (msg=0x55a663a7bc50, client=0x55a663a7b960,
server=0x55a663a77550, prog=0x55a663a78020) at rpc/virnetserverprogram.c:407
#6 virNetServerProgramDispatch (prog=prog@entry=0x55a663a78020, server=server@entry=0x55a663a77550,
client=client@entry=0x55a663a7b960, msg=msg@entry=0x55a663a7bc50) at rpc/virnetserverprogram.c:307
#7 0x000055a662871d56 in virNetServerProcessMsg (msg=0x55a663a7bc50, prog=0x55a663a78020, client=0x55a663a7b960,
srv=0x55a663a77550) at rpc/virnetserver.c:148
#8 virNetServerDispatchNewMessage (client=0x55a663a7b960, msg=0x55a663a7bc50, opaque=0x55a663a77550)
at rpc/virnetserver.c:227
#9 0x000055a66286e4c0 in virNetServerClientDispatchRead (client=client@entry=0x55a663a7b960)
at rpc/virnetserverclient.c:1322
#10 0x000055a66286e813 in virNetServerClientDispatchEvent (sock=<optimized out>, events=1, opaque=0x55a663a7b960)
at rpc/virnetserverclient.c:1507
#11 0x000055a662899be0 in virEventPollDispatchHandles (fds=0x55a663a7bdc0, nfds=<optimized out>)
at util/vireventpoll.c:508
#12 virEventPollRunOnce () at util/vireventpoll.c:657
#13 0x000055a6628982f1 in virEventRunDefaultImpl () at util/virevent.c:327
#14 0x000055a6628716d5 in virNetDaemonRun (dmn=0x55a663a771b0) at rpc/virnetdaemon.c:858
#15 0x000055a662864c1d in main (argc=<optimized out>,
#argv=0x7ffd105b4838) at logging/log_daemon.c:1235
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Currently virNetServerClientDispatchFunc implementations are only
responsible for free'ing the "msg" parameter upon success. Simplify the
calling convention by making it their unconditional responsibility to
free the "msg", and close the client if desired.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
There's no reason why the virNetServerClientDispatchRead method needs to
acquire an extra reference on the "client" object. An extra reference is
only needed if the registered dispatch callback is going to keep hold of
the "client" for work in the background. Thus we can push reference
acquisition into virNetServerDispatchNewMessage.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Ensure all enum cases are listed in switch statements.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
No sense in calling ServiceToggle for all nservices during
ServiceDispose since ServerClose calls ServiceClose which
removes the IOCallback that's being toggled via ServiceToggle.
Signed-off-by: John Ferlan <jferlan@redhat.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
The position of various parameters changes depending on the WITH_GNUTLS
macro.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Since we annotate the APIs are having non-NULL parameters, we can remove
the checks for NULL in the code too.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
With the current code it is neccessary to call
virNetDaemonNewPostExecRestart()
and then for each server that needs restarting you are supposed
to call
virNetDaemonAddSeverPostExecRestart()
This is fine if there's only ever one server, but as soon as you
have two servers it is impossible to use this design. The code
has no idea which servers were recorded in the JSON state doc,
nor in which order the hash table serialized its keys.
So this patch changes things so that we only call
virNetDaemonNewPostExecRestart()
passing in a callback, which is invoked once for each server
found int he JSON state doc.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
It is not possible to blindly call virNetDaemonGetServer()
because in a post-exec restart scenario, some servers may
not exist and this method will pollute the error logs.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The server name and client data callbacks need to be non-NULL or the
system will crash at various times. This is particularly bad when some
of the crashes only occur post-exec restart.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virNetServer class is passing a pointer to itself to the
virNetServerClient as a 'void *' pointer. This is presumably due to fact
that the virnetserverclient.h file doesn't see the virNetServerPtr
typedef. The typedef is easily movable though, which lets us get
typesafe parameter passing, removing the confusion of passing two
distinct 'void *' pointers to one method.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The virNetSocketWriteSASL method has to encode the buffer it is given and then
write it to the underlying socket. This write is not guaranteed to send the
full amount of data that was encoded by SASL. We cache the SASL encoded data so
that on the next invocation of virNetSocketWriteSASL we carry on sending it.
The subtle problem is that the 'len' value passed into virNetSocketWriteSASL on
the 2nd call may be larger than the original value. So when we've completed
sending the SASL encoded data we previously cached, we must return the original
length we encoded, not the new length.
This flaw means we could potentially have been discarded queued data without
sending it. This would have exhibited itself as a libvirt client never receiving
the reply to a method it invokes, async events silently going missing, or worse
stream data silently getting dropped.
For this to be a problem libvirtd would have to be queued data to send to the
client, while at the same time the TCP socket send buffer is full (due to a very
slow client). This is quite unlikely so if this bug was ever triggered by a real
world user it would be almost impossible to reproduce or diagnose, if indeed it
was ever noticed at all.
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Replace virNetServerClientNeedAuth with
virNetServerClientIsAuthenticated because it makes it clearer what it
means.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
'Squash' virNetServerClientNeedAuthLocked into
virNetServerClientNeedAuth and remove virNetServerClientNeedAuthLocked
as it's not longer needed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
There is a race between virNetServerProcessClients (main thread) and
remoteDispatchAuthList/remoteDispatchAuthPolkit/remoteSASLFinish (worker
thread) that can lead to decrementing srv->nclients_unauth when it's
zero. Since virNetServerCheckLimits relies on the value
srv->nclients_unauth the underrun causes libvirtd to stop accepting
new connections forever.
Example race scenario (assuming libvirtd is using policykit and the
client is privileged):
1. The client calls the RPC remoteDispatchAuthList =>
remoteDispatchAuthList is executed on a worker thread (Thread
T1). We're assuming now the execution stops for some time before
the line 'virNetServerClientSetAuth(client, 0)'
2. The client closes the connection irregularly. This causes the
event loop to wake up and virNetServerProcessClient to be
called (on the main thread T0). During the
virNetServerProcessClients the srv lock is hold. The condition
virNetServerClientNeedAuth(client) will be checked and as the
authentication is not finished right now
virNetServerTrackCompletedAuthLocked(srv) will be called =>
--srv->nclients_unauth => 0
3. The Thread T1 continues, marks the client as authenticated, and
calls virNetServerTrackCompletedAuthLocked(srv) =>
--srv->nclients_unauth => --0 => wrap around as nclient_unauth is
unsigned
4. virNetServerCheckLimits(srv) will disable the services forever
To fix it, add an auth_pending field to the client struct so that it
is now possible to determine if the authentication process has already
been handled for this client.
Setting the authentication method to none for the client in
virNetServerProcessClients is not a proper way to indicate that the
counter has been decremented, as this would imply that the client is
authenticated.
Additionally, adjust the existing test cases for this new field.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Combine virNetServerClientSetAuth(client,
VIR_NET_SERVER_SERVICE_AUTH_NONE) and virNetServerTrackCompletedAuth
into one new function named virNetServerSetClientAuthenticated.
After using this new function the function
virNetServerTrackCompletedAuth was superfluous and is therefore
removed. In addition, it is not very common that a
'{{function}}' (virNetServerTrackCompletedAuth) does more than just
the locking compared to
'{{function}}Locked' (virNetServerTrackCompletedAuthLocked).
virNetServerTrackPendingAuth was already superfluous and therefore
it's also removed.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
The lock for @client must not only be held for the duration of
checking whether the client wants to close, but also for as long as
we're closing the client. The same applies to the tracking of
authentications.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Add virNetServerClientAuthMethodImpliesAuthenticated() for deciding
whether a authentication method implies that a client is automatically
authenticated or not. Use this new function in
virNetServerClientNeedAuthLocked().
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
This makes the code more efficient.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Be more precise in which cases the authentication is needed and
introduce *Locked.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Add typedef for the anonymous enum used for the authentication methods
and remove the default case. This allows the usage of the type in a
switch statement and taking advantage of the compilers feature to
detect uncovered cases.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi@linux.vnet.ibm.com>
Reviewed-by: John Ferlan <jferlan@redhat.com>
So far clients were closed when disposing the daemon, after the state
driver cleanup. This was leading to libvirtd crashing at shutdown due
to missing driver.
Moving the client close in virNetServerClose() fixes the problem.
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Prior to this change, we relied solely on the inherited readonly
attribute of a service's socket. This only worked for our UNIX sockets
(and only to some degree), but doesn't work for TCP sockets which are RW
by default, but such connections support RO as well. This patch forces
an update on the client object once we have established a connection to
reflect the nature of the connection itself rather than relying on the
underlying socket's attributes.
Clients connected to the admin server have always been connected as RW
only.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1524399
Signed-off-by: Erik Skultety <eskultet@redhat.com>
After the virNetDaemonAddServerPostExec call in virtlogd we should have
netserver refcount set to 2. One goes to netdaemon servers hashtable
and one goes to virt{logd,lock} own reference to netserver. Let's add
the missing increment in virNetDaemonAddServerPostExec itself while
holding the daemon lock.
Since lockd defers management of the @srv object by the presence
in the hash table, virLockDaemonNewPostExecRestart must Unref the
alloc'd Ref on the @srv object done as part of virNetDaemonAddServerPostExec
and virNetServerNewPostExecRestart processing. The virNetDaemonGetServer
in lock_daemon main will also take a reference which is Unref'd during
main cleanup.
Right-aligning backslashes when defining macros or using complex
commands in Makefiles looks cute, but as soon as any changes is
required to the code you end up with either distractingly broken
alignment or unnecessarily big diffs where most of the changes
are just pushing all backslashes a few characters to one side.
Generated using
$ git grep -El '[[:blank:]][[:blank:]]\\$' | \
grep -E '*\.([chx]|am|mk)$$' | \
while read f; do \
sed -Ei 's/[[:blank:]]*[[:blank:]]\\$/ \\/g' "$f"; \
done
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
The packet with passed FD has the following format:
--------------------------
| len | header | payload |
--------------------------
where "payload" has an additional count of FDs before the actual data:
------------------
| nfds | payload |
------------------
When the packet is received we parse the "header", which as a side
effect updates msg->bufferOffset to point to the beginning of "payload".
If the message call contains FDs, we need to also parse the count of
FDs, which also updates the msg->bufferOffset.
The issue here is that when we attempt to read the FDs data from the
socket and we receive EAGAIN we finish the reading and call poll()
to wait for the data the we need. When the data arrives we already have
the packet in our buffer so we read the "header" again but this time
we don't read the count of FDs because we already have it stored.
That means that the msg->bufferOffset is not updated to point to the
actual beginning of the payload data, but it points to the count of
FDs. After all FDs are processed we dispatch the message to process
it and decode the payload. Since the msg->bufferOffset points to wrong
data, we decode the wrong payload and the API call fails with
error messages:
Domain not found: no domain with matching uuid '67656e65-7269-6300-0c87-5003ca6941f2' ()
Broken by commit 133c511b52 which fixed a FD and memory leak.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
Seeing a log message saying 'flags=93' is ambiguous & confusing unless
you happen to know that libvirt always prints flags as hex. Change our
debug messages so that they always add a '0x' prefix when printing flags,
and '0' prefix when printing mode. A few other misc places gain a '0x'
prefix in error messages too.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
This is particularly useful on operating systems that don't ship
Perl as part of the base system (eg. FreeBSD) while still working
just as well as it did before on Linux.
In one case (src/rpc/genprotocol.pl) the interpreter path was
missing altogether.
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
https://bugzilla.redhat.com/show_bug.cgi?id=1487322
In ace45e67ab I tried to fix a problem that we get the reply to
a D-Bus call while we were sleeping. In that case the callback
was never set. So I changed the code that the callback is called
directly in this case. However, I hadn't realized that since the
callback is called out of order it locks the virNetDaemon.
Exactly the very same virNetDaemon object that we are dealing
with right now and that we have locked already (in
virNetDaemonAddShutdownInhibition())
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Inspired by the recent GIT / Mercurial security flaws
(http://blog.recurity-labs.com/2017-08-10/scm-vulns),
consider someone/something manages to feed libvirt a bogus
URI such as:
virsh -c qemu+ssh://-oProxyCommand=gnome-calculator/system
In this case, the hosname "-oProxyCommand=gnome-calculator"
will get interpreted as an argument to ssh, not a hostname.
Fortunately, due to the set of args we have following the
hostname, SSH will then interpret our bit of shell script
that runs 'nc' on the remote host as a cipher name, which is
clearly invalid. This makes ssh exit during argv parsing and
so it never tries to run gnome-calculator.
We are lucky this time, but lets be more paranoid, by using
'--' to explicitly tell SSH when it has finished seeing
command line options. This forces it to interpret
"-oProxyCommand=gnome-calculator" as a hostname, and thus
see a fail from hostname lookup.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
So there are couple of issues here. Firstly, we never unref the
@pendingReply and thus it leaks.
==13279== 144 (72 direct, 72 indirect) bytes in 1 blocks are definitely lost in loss record 1,095 of 1,259
==13279== at 0x4C2E080: calloc (vg_replace_malloc.c:711)
==13279== by 0x781FA97: _dbus_pending_call_new_unlocked (in /usr/lib64/libdbus-1.so.3.14.11)
==13279== by 0x7812A4C: dbus_connection_send_with_reply (in /usr/lib64/libdbus-1.so.3.14.11)
==13279== by 0x56BEDF3: virNetDaemonCallInhibit (virnetdaemon.c:514)
==13279== by 0x56BEF18: virNetDaemonAddShutdownInhibition (virnetdaemon.c:536)
==13279== by 0x12473B: daemonInhibitCallback (libvirtd.c:742)
==13279== by 0x1249BD: daemonRunStateInit (libvirtd.c:823)
==13279== by 0x554FBCF: virThreadHelper (virthread.c:206)
==13279== by 0x8F913D3: start_thread (in /lib64/libpthread-2.23.so)
==13279== by 0x928DE3C: clone (in /lib64/libc-2.23.so)
Secondly, while we send the message, we are suspended ('cos we're
talking to a UNIX socket). However, until we are resumed back
again the reply might have came therefore subsequent
dbus_pending_call_set_notify() has no effect and in fact the
virNetDaemonGotInhibitReply() callback is never called. Thirdly,
the dbus_connection_send_with_reply() has really stupid policy
for return values. To cite the man page:
Returns
FALSE if no memory, TRUE otherwise.
Yes, that's right. If anything goes wrong and it's not case of
OOM then TRUE is returned, i.e. you're trying to pass FDs and
it's not supported, or you're not connected, or anything else.
Therefore, checking for return value of
dbus_connection_send_with_reply() is not enoguh. We also have to
check if @pendingReply is not NULL before proceeding any further.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
This reverts commit e4b980c853.
When a binary links against a .a archive (as opposed to a shared library),
any symbols which are marked as 'weak' get silently dropped. As a result
when the binary later runs, those 'weak' functions have an address of
0x0 and thus crash when run.
This happened with virtlogd and virtlockd because they don't link to
libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The
virRandomBits symbols was weak and so left out of the virtlogd &
virtlockd binaries, despite being required by virHashTable functions.
Various other binaries like libvirt_lxc, libvirt_iohelper, etc also
link directly to .a files instead of libvirt.so, so are potentially
at risk of dropping symbols leading to a later runtime crash.
This is normal linker behaviour because a weak symbol is not treated
as undefined, so nothing forces it to be pulled in from the .a You
have to force the linker to pull in weak symbols using -u$SYMNAME
which is not a practical approach.
This risk is silent bad linkage that affects runtime behaviour is
not acceptable for a fix that was merely trying to fix the test
suite. So stop using __weak__ again.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
If we exceed a fixed limit in RPC code we get a horrible message
like this, if the parameter type is a 'string', because we forgot
to initialize the error message type field:
$ virsh snapshot-list ostack1
error: too many remote undefineds: 1329 > 1024
It would also be useful to know which RPC call and field was
exceeded. So this patch makes us report:
$ virsh snapshot-list ostack1
error: too many remote undefineds: 1329 > 1024,
in parameter 'names' for 'virDomainSnapshotListNames'
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Currently all mockable functions are annotated with the 'noinline'
attribute. This is insufficient to guarantee that a function can
be reliably mocked with an LD_PRELOAD. The C language spec allows
the compiler to assume there is only a single implementation of
each function. It can thus do things like propagating constant
return values into the caller at compile time, or creating
multiple specialized copies of the function body each optimized
for a different caller. To prevent these optimizations we must
also set the 'noclone' and 'weak' attributes.
This fixes the test suite when libvirt.so is built with CLang
with optimization enabled.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The log category for virnetdaemon.c was mistakenly set
to rpc.netserver. Some useful info about the inhibitor
file descriptor was also never logged.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
The DBus conditional was renamed way back:
commit da77f04ed5
Author: Daniel P. Berrange <berrange@redhat.com>
Date: Thu Sep 20 15:05:39 2012 +0100
Convert HAVE_DBUS to WITH_DBUS
but the shutdown inhibit code was not updated. Thus libvirt
was never inhibiting shutdown by a logged in user when VMs
are running.
Reviewed-by: Pavel Hrdina <phrdina@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Use ATTRIBUTE_FALLTHROUGH, introduced by commit
5d84f5961b, instead of comments to
indicate that the fall through is an intentional behavior.
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
Signed-off-by: Marc Hartmayer <mhartmay@linux.vnet.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy@linux.vnet.ibm.com>
Reviewed-by: Bjoern Walk <bwalk@linux.vnet.ibm.com>
While reworking client side of streams, I had to postpone payload
decoding so that stream holes and stream data can be
distinguished in virNetClientStreamRecvPacket. That's merely what
18944b7aea does. However, I accidentally removed one important
bit: when server sends us an empty STREAM packet (with no
payload) - meaning end of stream - st->incomingEOF flag needs to
be set. It used to be before I touched the code. After I removed
it, virNetClientStreamRecvPacket will try to fetch more data from
the stream, but it will never come.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
When increasing the buffer size up to VIR_NET_MESSAGE_MAX, we
currently quadruple it each time. This unfortunately means that we
cannot allow certain buffer sizes -- for example the current
VIR_NET_MESSAGE_MAX == 33554432 can never be "hit" since ‘newlen’
jumps from 16MB to 64MB.
Instead of quadrupling, double it each time.
Thanks: Daniel Berrange.
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
While most of the APIs are okay with 16M messages, the bulk stats API
can run into the limit in big configurations. Before we devise a new
plan for this, bump this limit slightly to accomodate some more configs.
