After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
Problem with this approach is in the NSS module because the
module calls some internal APIs which occasionally want to log
something. This results in virLogInitialize() to be called which
in turn ends up calling virGetHostnameQuiet() and effectively the
control gets to NSS plugin again which calls some internal APIs
which occasionally want to log something. You can see the
deadlock now.

One way out of this is to call only gethostname() and not whole
virGetHostnameQuiet() machinery.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/util/virlog.c        |  2 +-
 src/util/virutil.c       | 46 ++++++++++++++++++++++++++++++++++------------
 src/util/virutil.h       |  1 +
 4 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 3b14d7d15..3ef55f809 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -3016,6 +3016,7 @@ virGetGroupList;
 virGetGroupName;
 virGetHostname;
 virGetHostnameQuiet;
+virGetHostnameSimple;
 virGetListenFDs;
 virGetSelfLastChanged;
 virGetSystemPageSize;
diff --git a/src/util/virlog.c b/src/util/virlog.c
index 8f1e4800d..fc854ffeb 100644
--- a/src/util/virlog.c
+++ b/src/util/virlog.c
@@ -276,7 +276,7 @@ virLogOnceInit(void)
      * it might not be possible to load NSS modules via getaddrinfo()
      * (e.g. at container startup the host filesystem will not be
      * accessible anymore. */
-    virLogHostname = virGetHostnameQuiet();
+    virLogHostname = virGetHostnameSimple();
 
     virLogUnlock();
     return 0;
diff --git a/src/util/virutil.c b/src/util/virutil.c
index cd6fbf2f3..22adecd53 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -684,26 +684,23 @@ static char *
 virGetHostnameImpl(bool quiet)
 {
     int r;
-    char hostname[HOST_NAME_MAX+1], *result = NULL;
+    char *result;
     struct addrinfo hints, *info;
 
-    r = gethostname(hostname, sizeof(hostname));
-    if (r == -1) {
+    if (!(result = virGetHostnameSimple())) {
         if (!quiet)
             virReportSystemError(errno,
                                  "%s", _("failed to determine host name"));
         return NULL;
     }
-    NUL_TERMINATE(hostname);
 
-    if (STRPREFIX(hostname, "localhost") || strchr(hostname, '.')) {
+    if (STRPREFIX(result, "localhost") || strchr(result, '.')) {
         /* in this case, gethostname returned localhost (meaning we can't
          * do any further canonicalization), or it returned an FQDN (and
          * we don't need to do any further canonicalization).  Return the
          * string as-is; it's up to callers to check whether "localhost"
          * is allowed.
          */
-        ignore_value(VIR_STRDUP_QUIET(result, hostname));
         goto cleanup;
     }
 
@@ -714,12 +711,11 @@ virGetHostnameImpl(bool quiet)
     memset(&hints, 0, sizeof(hints));
     hints.ai_flags = AI_CANONNAME|AI_CANONIDN;
     hints.ai_family = AF_UNSPEC;
-    r = getaddrinfo(hostname, NULL, &hints, &info);
+    r = getaddrinfo(result, NULL, &hints, &info);
     if (r != 0) {
         if (!quiet)
             VIR_WARN("getaddrinfo failed for '%s': %s",
-                     hostname, gai_strerror(r));
-        ignore_value(VIR_STRDUP_QUIET(result, hostname));
+                     result, gai_strerror(r));
         goto cleanup;
     }
 
@@ -727,15 +723,16 @@ virGetHostnameImpl(bool quiet)
     sa_assert(info);
 
     if (info->ai_canonname == NULL ||
-        STRPREFIX(info->ai_canonname, "localhost"))
+        STRPREFIX(info->ai_canonname, "localhost")) {
         /* in this case, we tried to canonicalize and we ended up back with
          * localhost.  Ignore the canonicalized name and just return the
          * original hostname
          */
-        ignore_value(VIR_STRDUP_QUIET(result, hostname));
-    else
+    } else {
         /* Caller frees this string. */
+        VIR_FREE(result);
         ignore_value(VIR_STRDUP_QUIET(result, info->ai_canonname));
+    }
 
     freeaddrinfo(info);
 
@@ -760,6 +757,31 @@ virGetHostnameQuiet(void)
 }
 
 
+/**
+ * virGetHostnameSimple:
+ *
+ * Plain wrapper over gethostname(). The difference to
+ * virGetHostname() is that this function doesn't try to
+ * canonicalize the hostname.
+ *
+ * Returns: hostname string (caller must free),
+ *          NULL on error.
+ */
+char *
+virGetHostnameSimple(void)
+{
+    char hostname[HOST_NAME_MAX+1];
+    char *ret;
+
+    if (gethostname(hostname, sizeof(hostname)) == -1)
+        return NULL;
+
+    NUL_TERMINATE(hostname);
+    ignore_value(VIR_STRDUP_QUIET(ret, hostname));
+    return ret;
+}
+
+
 char *
 virGetUserDirectory(void)
 {
diff --git a/src/util/virutil.h b/src/util/virutil.h
index be0f6b0ea..57148374b 100644
--- a/src/util/virutil.h
+++ b/src/util/virutil.h
@@ -136,6 +136,7 @@ static inline int pthread_sigmask(int how,
 
 char *virGetHostname(void);
 char *virGetHostnameQuiet(void);
+char *virGetHostnameSimple(void);
 
 char *virGetUserDirectory(void);
 char *virGetUserDirectoryByUID(uid_t uid);
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Mon, Feb 12, 2018 at 11:29:21AM +0100, Michal Privoznik wrote:
> After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
> Problem with this approach is in the NSS module because the
> module calls some internal APIs which occasionally want to log
> something. This results in virLogInitialize() to be called which
> in turn ends up calling virGetHostnameQuiet() and effectively the
> control gets to NSS plugin again which calls some internal APIs
> which occasionally want to log something. You can see the
> deadlock now.
> 
> One way out of this is to call only gethostname() and not whole
> virGetHostnameQuiet() machinery.

The extra bits in virGetHostname() only exist for the sake of
the QEMU migration code. The source call gethostname() on the
target host and wants to make sure it doesn't return "localhost"
or something that resolves to "127.0.0.1", otherwise the source
host would end up migrating to itself instead of the actual
target host.  We should really just move that extra stuff into
the migration code and leave virGetHostname() simple, instead
of having a virGetHostnameSimple(). That's more than I would
want todo for this CVE fix though, as it would complicate the
backporting. So I feel my patch to inline hostname() call in
the logging code is more suitable in short term, but after
that we could do a big refactor.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 02/12/2018 11:42 AM, Daniel P. Berrangé wrote:
> On Mon, Feb 12, 2018 at 11:29:21AM +0100, Michal Privoznik wrote:
>> After 759b4d1b0fe5f we are getting hostname in virLogOnceInit().
>> Problem with this approach is in the NSS module because the
>> module calls some internal APIs which occasionally want to log
>> something. This results in virLogInitialize() to be called which
>> in turn ends up calling virGetHostnameQuiet() and effectively the
>> control gets to NSS plugin again which calls some internal APIs
>> which occasionally want to log something. You can see the
>> deadlock now.
>>
>> One way out of this is to call only gethostname() and not whole
>> virGetHostnameQuiet() machinery.
> 
> The extra bits in virGetHostname() only exist for the sake of
> the QEMU migration code. The source call gethostname() on the
> target host and wants to make sure it doesn't return "localhost"
> or something that resolves to "127.0.0.1", otherwise the source
> host would end up migrating to itself instead of the actual
> target host.  We should really just move that extra stuff into
> the migration code and leave virGetHostname() simple, instead
> of having a virGetHostnameSimple(). That's more than I would
> want todo for this CVE fix though, as it would complicate the
> backporting. So I feel my patch to inline hostname() call in
> the logging code is more suitable in short term, but after
> that we could do a big refactor.

Okay, lets got with your version then.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list