[libvirt] [PATCH v2] daemon: initialize GnuTLS
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, -- 1.7.3.4
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 -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 19.08.2011 11:03, Daniel Veillard wrote:
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
According to GnuTLS documentation, they do ref counting. I was concerned about the same as you are, but after reading the documentation I've created also Deinit. From gnutls_global_init [1]: "This function increment a global counter, so that gnutls_global_deinit() only releases resources when it has been called as many times as gnutls_global_init()." So pushed as-is. Thanks. [1] http://www.gnu.org/software/gnutls/reference/gnutls-gnutls.html#gnutls-globa...
participants (2)
-
Daniel Veillard -
Michal Privoznik