Re: [libvirt] [PATCH v2] daemon: initialize GnuTLS

On Thu, Aug 18, 2011 at 10:48:35AM +0200, Michal Privoznik wrote: > 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. > --- > diff to v1: > - moved from qemu to daemon > - created special init function > > daemon/libvirtd.c | 2 ++ > src/rpc/virnettlscontext.c | 25 ++++++++++++++++++++++--- > src/rpc/virnettlscontext.h | 3 +++ > 3 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c > index b99c637..0530ba5 100644 > --- a/daemon/libvirtd.c > +++ b/daemon/libvirtd.c > @@ -1516,6 +1516,7 @@ int main(int argc, char **argv) { > virHookCall(VIR_HOOK_DRIVER_DAEMON, "-", VIR_HOOK_DAEMON_OP_START, > 0, "start", NULL); > > + virNetTLSInit(); > if (daemonSetupNetworking(srv, config, > sock_file, sock_file_ro, > ipsock, privileged) < 0) { > @@ -1554,6 +1555,7 @@ cleanup: > virNetServerProgramFree(qemuProgram); > virNetServerClose(srv); > virNetServerFree(srv); > + virNetTLSDeinit(); > if (statuswrite != -1) { > if (ret != 0) { > /* Tell parent of daemon what failed */ > diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c > index 19a9b25..8482eaf 100644 > --- a/src/rpc/virnettlscontext.c > +++ b/src/rpc/virnettlscontext.c > @@ -679,9 +679,6 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, > > ctxt->refs = 1; > > - /* Initialise GnuTLS. */ > - gnutls_global_init(); > - > if ((gnutlsdebug = getenv("LIBVIRT_GNUTLS_DEBUG")) != NULL) { > int val; > if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0) > @@ -1399,3 +1396,25 @@ void virNetTLSSessionFree(virNetTLSSessionPtr sess) > virMutexDestroy(&sess->lock); > VIR_FREE(sess); > } > + > +/* > + * This function MUST be called before any > + * virNetTLS* because it initializes > + * underlying GnuTLS library. According to > + * it's documentation, it's safe to be called > + * many times, but is not thread safe. Each > + * call SHOULD be later followed by > + * virNetTLSContextDeinit. > + */ > +void virNetTLSInit(void) > +{ > + gnutls_global_init(); > +} > + > +/* > + * See virNetTLSInit > + */ > +void virNetTLSDeinit(void) > +{ > + gnutls_global_deinit(); > +} > diff --git a/src/rpc/virnettlscontext.h b/src/rpc/virnettlscontext.h > index 641d67e..99f31b9 100644 > --- a/src/rpc/virnettlscontext.h > +++ b/src/rpc/virnettlscontext.h > @@ -30,6 +30,9 @@ typedef struct _virNetTLSSession virNetTLSSession; > typedef virNetTLSSession *virNetTLSSessionPtr; > > > +void virNetTLSInit(void); > +void virNetTLSDeinit(void); > + > virNetTLSContextPtr virNetTLSContextNewServerPath(const char *pkipath, > bool tryUserPkiPath, > const char *const*x509dnWhitelist, I wonder about virNetTLSDeinit(), it basically call gnutls and tell the library that we don't use it anymore. While gnutls_global_init() might be safe, if they don't do reference counting gnutls_global_deinit() may free up data still used by another library under the hood. The comment seems to imply that gnutls_global_(de)init do reference counting and ACK in this case but if not I would just drop the virNetTLSDeinit() part, we were not calling gnutls_global_deinit before anyway. ACK if ref counting in gnutls confirmed Daniel