* src/rpc/virnettlscontext.c: fix memory leak on
virNetTLSContextValidCertificate.
* Detected in valgrind run:
==25667==
==25667== 6,085 (44 direct, 6,041 indirect) bytes in 1 blocks are definitely
lost in loss record 326 of 351
==25667== at 0x4005447: calloc (vg_replace_malloc.c:467)
==25667== by 0x4F2791F3: _asn1_add_node_only (structure.c:53)
==25667== by 0x4F27997A: _asn1_copy_structure3 (structure.c:421)
==25667== by 0x4F276A50: _asn1_append_sequence_set (element.c:144)
==25667== by 0x4F2743FF: asn1_der_decoding (decoding.c:1194)
==25667== by 0x4F22B9CC: gnutls_x509_crt_import (x509.c:229)
==25667== by 0x805274B: virNetTLSContextCheckCertificate
(virnettlscontext.c:1009)
==25667== by 0x804DE32: testTLSSessionInit (virnettlscontexttest.c:693)
==25667== by 0x804F14D: virtTestRun (testutils.c:140)
==25667==
==25667== 23,188 (88 direct, 23,100 indirect) bytes in 11 blocks are definitely
lost in loss record 346 of 351
==25667== at 0x4005447: calloc (vg_replace_malloc.c:467)
==25667== by 0x4F22B841: gnutls_x509_crt_init (x509.c:50)
==25667== by 0x805272B: virNetTLSContextCheckCertificate
(virnettlscontext.c:1003)
==25667== by 0x804DDD1: testTLSSessionInit (virnettlscontexttest.c:673)
==25667== by 0x804F14D: virtTestRun (testutils.c:140)
* How to reproduce?
% cd libvirt && ./configure && make && make -C tests valgrind
or
% valgrind -v --leak-check=full ./tests/virnettlscontexttest
Signed-off-by: Alex Jia <ajia@redhat.com>
This patch annotates APIs with low or high priority.
In low set MUST be all APIs which might eventually access monitor
(and thus block indefinitely). Other APIs may be marked as high
priority. However, some must be (e.g. domainDestroy).
For high priority calls (HPC), there are some high priority workers
(HPW) created in the pool. HPW can execute only HPC, although normal
worker can process any call regardless priority. Therefore, only those
APIs which are guaranteed to end in reasonable small amount of time
can be marked as HPC.
The size of this HPC pool is static, because HPC are expected to end
quickly, therefore jobs assigned to this pool will be served quickly.
It can be configured in libvirtd.conf via prio_workers variable.
Default is set to 5.
To mark API with low or high priority, append priority:{low|high} to
it's comment in src/remote/remote_protocol.x. This is similar to
autogen|skipgen. If not marked, the generator assumes low as default.
Commit 2c85644b0b51fbe5b6244e6773531af29933a727 attempted to
fix a problem with tracking RPC messages from streams by doing
- if (msg->header.type == VIR_NET_REPLY) {
+ if (msg->header.type == VIR_NET_REPLY ||
+ (msg->header.type == VIR_NET_STREAM &&
+ msg->header.status != VIR_NET_CONTINUE)) {
client->nrequests--;
In other words any stream packet, with status NET_OK or NET_ERROR
would cause nrequests to be decremented. This is great if the
packet from from a synchronous virStreamFinish or virStreamAbort
API call, but wildly wrong if from a server initiated abort.
The latter resulted in 'nrequests' being decremented below zero.
This then causes all I/O for that client to be stopped.
Instead of trying to infer whether we need to decrement the
nrequests field, from the message type/status, introduce an
explicit 'bool tracked' field to mark whether the virNetMessagePtr
object is subject to tracking.
Also add a virNetMessageClear function to allow a message
contents to be cleared out, without adversely impacting the
'tracked' field as a naive memset() would do
* src/rpc/virnetmessage.c, src/rpc/virnetmessage.h: Add
a 'bool tracked' field and virNetMessageClear() API
* daemon/remote.c, daemon/stream.c, src/rpc/virnetclientprogram.c,
src/rpc/virnetclientstream.c, src/rpc/virnetserverclient.c,
src/rpc/virnetserverprogram.c: Switch over to use
virNetMessageClear() and pass in the 'bool tracked' value
when creating messages.
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
