The bufferOffset has been initialized to zero in virNetMessageEncodePayloadRaw(),
so, we use bufferLength to represent the length of message which is going to be
sent to client side.
From: Matthias Bolte <matthias.bolte@googlemail.com>
Tested-by: Jim Fehlig <jfehlig@novell.com>
Matthias provided this patch to fix an issue I encountered in the
generator with APIs containing call-by-ref long type, e.g.
int virDomainMigrateGetMaxSpeed(virDomainPtr domain,
unsigned long *bandwidth,
unsigned int flags);
In case we add a new program in the future (we did that in the past and
we are going to do it again soon) current daemon will behave badly with
new client that wants to use the new program. Before the RPC rewrite we
used to just send an error reply to any request with unknown program.
With the RPC rewrite in 0.9.3 the daemon just closes the connection
through which such request was sent. This patch fixes this regression.
When spice_tls is set but listen_tls is not, we don't initialize
GnuTLS library. So any later gnutls call (e.g. during migration,
where we initialize a certificate) will access uninitialized GnuTLS
internal structs and throws an error.
Although, we might now initialize GnuTLS twice, it is safe according
to the documentation:
This function can be called many times,
but will only do something the first time.
This patch creates 2 functions: virNetTLSInit and virNetTLSDeinit
with respect to written above.
If a client had initiated a stream abort, it will have a call
waiting for a reply in the queue. If more data continues to
arrive on the stream, the abort command could mistakenly get
signalled as complete. Remove the code from async data processing
that looked for waiting calls. Add a sanity check to ensure no
async call can ever be marked as needing a reply
* src/rpc/virnetclient.c: Ensure async data packets can't
trigger a reply
If a stream gets a server initiated abort, the client may still
send an abort request before it receives the server side abort.
This causes the server to send back another abort for the
stream. Since the protocol defines that abort is the last thing
to be sent, the client gets confused by this second abort from
the server. If the stream is already shutdown, just drop any
client requested abort, rather than sending back another message.
This fixes the regression from previous versions.
Tested as follows
In one virsh session
virsh # start foo
virsh # console foo
In other virsh session
virsh # destroy foo
The first virsh session should be able to continue issuing
commands without error. Prior to this patch it saw
virsh # list
error: Failed to list active domains
error: An error occurred, but the cause is unknown
virsh # list
error: Failed to list active domains
error: no call waiting for reply with prog 536903814 vers 1 serial 9
* src/rpc/virnetserverprogram.c: Drop abort requests
for streams which no longer exist
Every active stream results in a reference being held on the
virNetServerClientPtr object. This meant that if a client quit
with any streams active, although all I/O was stopped the
virNetServerClientPtr object would leak. This causes libvirtd
to leak any file handles associated with open streams when a
client quit
To fix this, when we call virNetServerClientClose there is a
callback invoked which lets the daemon release the streams
and thus the extra references
* daemon/remote.c: Add a hook to close all streams
* daemon/stream.c, daemon/stream.h: Add API for releasing
all streams
* src/rpc/virnetserverclient.c, src/rpc/virnetserverclient.h:
Allow registration of a hook to trigger when closing client
When trying to use any SASL authentication for TCP sockets by
setting auth_tls = "sasl" in libvirtd.conf on server side, the
client will hang because of the sasl session relocking other than
dropping the lock when exiting virNetSASLSessionExtKeySize()
* src/rpc/virnetsaslcontext.c: virNetSASLSessionExtKeySize drop the
lock on exit
This patch introduces a internal RPC API "virNetServerClose", which
is standalone with "virNetServerFree". it closes all the socket fds,
and unlinks the unix socket paths, regardless of whether the socket
is still referenced or not.
This is to address regression bug:
https://bugzilla.redhat.com/show_bug.cgi?id=725702
In virNetServerNew, Coverity didn't realize that srv->mdsnGroupName
can only be non-NULL if mdsnGroupName was non-NULL.
In virNetServerRun, Coverity didn't realize that the array is non-NULL
if the array count is non-zero.
* src/rpc/virnetserver.c (virNetServerNew): Use alternate pointer.
(virNetServerRun): Give coverity a hint.
Detected by Coverity. Freeing the wrong variable results in both
a memory leak and the likelihood of the caller dereferencing through
a freed pointer.
* src/rpc/virnettlscontext.c (virNetTLSSessionNew): Free correct
variable.
Detected by Coverity. We want to compare the result of fnmatch 'rv',
not our pre-set return value 'ret'.
* src/rpc/virnetsaslcontext.c (virNetSASLContextCheckIdentity):
Check correct variable.
Spotted by Coverity. Gnutls documents that buffer must be NULL
if gnutls_x509_crt_get_key_purpose_oid is to be used to determine
the correct size needed for allocating a buffer.
* src/rpc/virnettlscontext.c
(virNetTLSContextCheckCertKeyPurpose): Initialize buffer.
Spotted by coverity. If pipe2 fails, then we attempt to close
uninitialized fds, which may result in a double-close.
* src/rpc/virnetserver.c (virNetServerSignalSetup): Initialize fds.
Steps to reproduce this problem (vm1 is not running):
for i in `seq 50`; do virsh managedsave vm1& done; killall virsh
Pre-patch, virNetServerClientClose could end up setting client->sock
to NULL prior to other cleanup functions trying to use client->sock.
This fixes things by checking for NULL in more places, and by deferring
the cleanup until after all queued messages have been served.
* src/rpc/virnetserverclient.c (virNetServerClientRegisterEvent)
(virNetServerClientGetFD, virNetServerClientIsSecure)
(virNetServerClientLocalAddrString)
(virNetServerClientRemoteAddrString): Check for closed socket.
(virNetServerClientClose): Rearrange close sequence.
Analysis from Wen Congyang.
Without this, cygwin failed to compile:
In file included from ../src/rpc/virnetmessage.h:24,
from ../src/rpc/virnetclient.h:27,
from remote/remote_driver.c:31:
../src/rpc/virnetprotocol.h:9:21: error: rpc/rpc.h: No such file or directory
With that fixed, compilation warned:
rpc/virnetsocket.c: In function 'virNetSocketNewListenUNIX':
rpc/virnetsocket.c:347: warning: format '%d' expects type 'int', but argument 8 has type 'gid_t' [-Wformat]
rpc/virnetsocket.c: In function 'virNetSocketGetLocalIdentity':
rpc/virnetsocket.c:743: warning: pointer targets in passing argument 5 of 'getsockopt' differ in signedness
* src/Makefile.am (libvirt_driver_remote_la_CFLAGS)
(libvirt_net_rpc_client_la_CFLAGS)
(libvirt_net_rpc_server_la_CFLAGS): Include XDR_CFLAGS, for rpc
headers on cygwin.
* src/rpc/virnetsocket.c (virNetSocketNewListenUNIX)
(virNetSocketGetLocalIdentity): Avoid compiler warnings.
On RHEL 5, with gcc 4.1.2:
rpc/virnetsaslcontext.c: In function 'virNetSASLSessionUpdateBufSize':
rpc/virnetsaslcontext.c:396: warning: dereferencing type-punned pointer will break strict-aliasing rules [-Wstrict-aliasing]
* src/rpc/virnetsaslcontext.c (virNetSASLSessionUpdateBufSize):
Use a union to work around gcc warning.
When an incoming RPC message is ready for processing,
virNetServerClientDispatchRead()
will invoke the 'dispatchFunc' callback. This is set to
virNetServerDispatchNewMessage
This function puts the message + client in a queue for processing by the thread
pool. The thread pool worker function is
virNetServerHandleJob
The first thing this does is acquire an extra reference on the 'client'.
Unfortunately, between the time the message+client are put on the thread pool
queue, and the time the worker runs, the client object may have had its last
reference removed.
We clearly need to add the reference to the client object before putting the
client on the processing queue
* src/rpc/virnetserverclient.c: Add a reference to the client when
invoking the dispatch function
* src/rpc/virnetserver.c: Don't acquire a reference to the client
when in the worker thread
The virNetSASLContext, virNetSASLSession, virNetTLSContext and
virNetTLSSession classes previously relied in their owners
(virNetClient / virNetServer / virNetServerClient) to provide
locking protection for concurrent usage. When virNetSocket
gained its own locking code, this invalidated the implicit
safety the SASL/TLS modules relied on. Thus we need to give
them all explicit locking of their own via new mutexes.
* src/rpc/virnetsaslcontext.c, src/rpc/virnettlscontext.c: Add
a mutex per object
When setting up a server socket, we must skip EADDRINUSE errors
from bind, since the IPv6 socket bind may have already bound to
the IPv4 socket too. If we don't manage to bind to any sockets
at all though, we should then report the EADDRINUSE error as
normal.
This fixes the case where libvirtd would not exit if some other
program was listening on its TCP/TLS ports.
* src/rpc/virnetsocket.c: Report EADDRINUSE
When libvirtd starts it it will sanity check its own certs,
and before libvirt clients connect to a remote server they
will sanity check their own certs. This patch allows such
sanity checking to be skipped. There is no strong reason to
need to do this, other than to bypass possible libvirt bugs
in sanity checking, or for testing purposes.
libvirt.conf gains tls_no_sanity_certificate parameter to
go along with tls_no_verify_certificate. The remote driver
client URIs gain a no_sanity URI parameter
* daemon/test_libvirtd.aug, daemon/libvirtd.conf,
daemon/libvirtd.c, daemon/libvirtd.aug: Add parameter to
allow cert sanity checks to be skipped
* src/remote/remote_driver.c: Add no_sanity parameter to
skip cert checks
* src/rpc/virnettlscontext.c, src/rpc/virnettlscontext.h:
Add new parameter for skipping sanity checks independantly
of skipping session cert validation checks
There is some commonality between the code for sanity checking
certs when initializing libvirt and the code for validating
certs during a live TLS session handshake. This patchset splits
up the sanity checking function into several smaller functions
each doing a specific type of check. The cert validation code
is then updated to also call into these functions
* src/rpc/virnettlscontext.c: Refactor cert validation code
The gnutls_certificate_type_set_priority method is deprecated.
Since we already set the default gnutls priority, it was not
serving any useful purpose and can be removed
* src/rpc/virnettlscontext.c: Remove gnutls_certificate_type_set_priority
call
If the virStateInitialize call fails we must shutdown libvirtd
since drivers will not be available. Just free'ing the virNetServer
is not sufficient, we must send a SIGTERM to ourselves so that
we interrupt the event loop and trigger a orderly shutdown
* daemon/libvirtd.c: Kill ourselves if state init fails
* src/rpc/virnetserver.c: Add some debugging to event loop
The generator can handle everything except virDomainGetBlockJobInfo().
* src/remote/remote_protocol.x: provide defines for the new entry points
* src/remote/remote_driver.c daemon/remote.c: implement the client and
server side for virDomainGetBlockJobInfo.
* src/remote_protocol-structs: structure definitions for protocol verification
* src/rpc/gendispatch.pl: Permit some unsigned long parameters
The only 'void name(void)' style procedure in the protocol is 'close' that
is handled special, but also programming errors like a missing _args or
_ret suffix on the structs in the .x files can create such a situation by
accident. Making the generator aware of this avoids bogus errors from the
generator such as:
Use of uninitialized value in exists at ./rpc/gendispatch.pl line 967.
Also this allows to get rid of the -c option and the special case code for
the 'close' procedure, as the generator handles it now correctly.
Reported by Michal Privoznik
Though we prefer users to have SSH keys setup, virt-manager users still
depend on remote SSH connections to launch a password dialog. This fixes
launch ssh-askpass
Fix suggested by danpb
If a key purpose or usage field is marked as non-critical in the
certificate, then a data mismatch is not (ordinarily) a cause for
rejecting the connection
* src/rpc/virnettlscontext.c: Honour key usage/purpose criticality
If key usage or purpose data is not present in the cert, the
RFC recommends that access be allowed. Also fix checking of
key usage to include requirements for client/server certs,
and fix key purpose checking to treat data as a list of bits
Gnutls requires that certificates have basic constraints present
to be used as a CA certificate. OpenSSL doesn't add this data
by default, so add a sanity check to catch this situation. Also
validate that the key usage and key purpose constraints contain
correct data
* src/rpc/virnettlscontext.c: Add sanity checking of certificate
constraints
If the libvirt daemon or libvirt client is configured with bogus
certificates, it is very unhelpful to only find out about this
when a TLS connection is actually attempted. Not least because
the error messages you get back for failures are incredibly
obscure.
This adds some basic sanity checking of certificates at the
time the virNetTLSContext object is created. This is at libvirt
startup, or when creating a virNetClient instance.
This checks that the certificate expiry/start dates are valid
and that the certificate is actually signed by the CA that is
loaded.
* src/rpc/virnettlscontext.c: Add certificate sanity checks
Since the I/O callback registered against virNetSocket will
hold a reference on the virNetClient, we can't rely on the
virNetClientFree to be able to close the network connection.
The last reference will only go away when the event callback
fires (likely due to EOF from the server).
This is sub-optimal and can potentially cause a leak of the
virNetClient object if the server were to not explicitly
close the socket itself
* src/remote/remote_driver.c: Explicitly close the client
object when disconnecting
* src/rpc/virnetclient.c, src/rpc/virnetclient.h: Add a
virNetClientClose method
When unregistering an I/O callback from a virNetSocket object,
there is still a chance that an event may come in on the callback.
In this case it is possible that the virNetSocket might have been
freed already. Make use of a virFreeCallback when registering
the I/O callbacks and hold a reference for the entire time the
callback is set.
* src/rpc/virnetsocket.c: Register a free function for the
file handle watch
* src/rpc/virnetsocket.h, src/rpc/virnetserverservice.c,
src/rpc/virnetserverclient.c, src/rpc/virnetclient.c: Add
a free function for the socket I/O watches
Remove the need for a virNetSocket object to be protected by
locks from the object using it, by introducing its own native
locking and reference counting
* src/rpc/virnetsocket.c: Add locking & reference counting
