[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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-global-init


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]