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

Re: [libvirt] PATCH: 19/25: Remove use of non-threadsafe POSIX apis



Remove use of various non thread safe functions, specifically

  strtok getmntent getgrnam getpwuid gethostbyname

I thought we also had to remove use of readdir(), but it turns
out that *is* threadsafe provided you only use each DIR* object
from one thread at a time, which is fine for our needs. The
readdir_r() function is absolutely horrific to use safely so
its just as well we don't need to.

For the gethostbyname removal in Xen, I took the opportunity
to switch to getaddrinfo(), since even gethostbyname_r is
deprecated these days - not IPv6 aware.

 configure.in             |    2 
 proxy/libvirt_proxy.c    |    3 
 qemud/qemud.c            |    9 +-
 src/lxc_container.c      |    9 +-
 src/network_driver.c     |    7 +-
 src/openvz_driver.c      |    3 
 src/qemu_driver.c        |    7 +-
 src/remote_internal.c    |    4 -
 src/storage_backend_fs.c |    7 +-
 src/storage_driver.c     |    7 +-
 src/uml_driver.c         |    6 +
 src/xen_unified.c        |    3 
 src/xen_unified.h        |   10 +--
 src/xend_internal.c      |  147 +++++++++++++++++++++++++----------------------
 14 files changed, 123 insertions(+), 101 deletions(-)

Daniel

diff --git a/configure.in b/configure.in
--- a/configure.in
+++ b/configure.in
@@ -75,7 +75,7 @@ dnl Availability of various common funct
 AC_CHECK_FUNCS([cfmakeraw regexec uname sched_getaffinity getuid getgid])
 
 dnl Availability of various not common threadsafe functions
-AC_CHECK_FUNCS([strerror_r])
+AC_CHECK_FUNCS([strerror_r strtok_r getmntent_r getgrnam_r getpwuid_r])
 
 dnl Availability of various common headers (non-fatal if missing).
 AC_CHECK_HEADERS([pwd.h paths.h regex.h sys/syslimits.h sys/utsname.h sys/wait.h winsock2.h sched.h termios.h sys/poll.h syslog.h])
diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
--- a/proxy/libvirt_proxy.c
+++ b/proxy/libvirt_proxy.c
@@ -76,9 +76,6 @@ proxyInitXen(void) {
 
     priv->handle = -1;
     priv->xendConfigVersion = -1;
-    priv->type = -1;
-    priv->len = -1;
-    priv->addr = NULL;
     priv->xshandle = NULL;
     priv->proxy = -1;
 
diff --git a/qemud/qemud.c b/qemud/qemud.c
--- a/qemud/qemud.c
+++ b/qemud/qemud.c
@@ -689,9 +689,11 @@ static int qemudInitPaths(struct qemud_s
         if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX)
             goto snprintf_error;
     } else {
+        char buf[1024];
+        struct passwd pwbuf;
         struct passwd *pw;
 
-        if (!(pw = getpwuid(uid))) {
+        if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
             VIR_ERROR(_("Failed to find user record for uid '%d': %s"),
                      uid, strerror(errno));
             return -1;
@@ -2376,8 +2378,9 @@ remoteReadConfigFile (struct qemud_serve
         if (getuid() != 0) {
             VIR_WARN0(_("Cannot set group when not running as root"));
         } else {
-            struct group *grp = getgrnam(unix_sock_group);
-            if (!grp) {
+            char buf[1024];
+            struct group grpdata, *grp;
+            if (getgrnam_r(unix_sock_group, &grpdata, buf, sizeof(buf), &grp) != 0 || !grp) {
                 VIR_ERROR(_("Failed to lookup group '%s'"), unix_sock_group);
                 goto free_and_fail;
             }
diff --git a/src/lxc_container.c b/src/lxc_container.c
--- a/src/lxc_container.c
+++ b/src/lxc_container.c
@@ -414,19 +414,20 @@ static int lxcContainerMountNewFS(virDom
 
 static int lxcContainerUnmountOldFS(void)
 {
-    struct mntent *mntent;
+    struct mntent mntent;
     char **mounts = NULL;
     int nmounts = 0;
     FILE *procmnt;
     int i;
+    char mntbuf[1024];
 
     if (!(procmnt = setmntent("/proc/mounts", "r"))) {
         virReportSystemError(NULL, errno, "%s",
                              _("failed to read /proc/mounts"));
         return -1;
     }
-    while ((mntent = getmntent(procmnt)) != NULL) {
-        if (!STRPREFIX(mntent->mnt_dir, "/.oldroot"))
+    while (getmntent_r(procmnt, &mntent, mntbuf, sizeof(mntbuf)) != NULL) {
+        if (!STRPREFIX(mntent.mnt_dir, "/.oldroot"))
             continue;
 
         if (VIR_REALLOC_N(mounts, nmounts+1) < 0) {
@@ -434,7 +435,7 @@ static int lxcContainerUnmountOldFS(void
             lxcError(NULL, NULL, VIR_ERR_NO_MEMORY, NULL);
             return -1;
         }
-        if (!(mounts[nmounts++] = strdup(mntent->mnt_dir))) {
+        if (!(mounts[nmounts++] = strdup(mntent.mnt_dir))) {
             endmntent(procmnt);
             lxcError(NULL, NULL, VIR_ERR_NO_MEMORY, NULL);
             return -1;
diff --git a/src/network_driver.c b/src/network_driver.c
--- a/src/network_driver.c
+++ b/src/network_driver.c
@@ -131,7 +131,6 @@ networkAutostartConfigs(struct network_d
 static int
 networkStartup(void) {
     uid_t uid = geteuid();
-    struct passwd *pw;
     char *base = NULL;
 
     if (VIR_ALLOC(driverState) < 0)
@@ -151,7 +150,11 @@ networkStartup(void) {
         if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL)
             goto out_of_memory;
     } else {
-        if (!(pw = getpwuid(uid))) {
+        char buf[1024];
+        struct passwd pwbuf;
+        struct passwd *pw;
+
+        if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
             networkLog(NETWORK_ERR, _("Failed to find user record for uid '%d': %s\n"),
                      uid, strerror(errno));
             goto out_of_memory;
diff --git a/src/openvz_driver.c b/src/openvz_driver.c
--- a/src/openvz_driver.c
+++ b/src/openvz_driver.c
@@ -448,11 +448,12 @@ openvzGenerateContainerVethName(int veid
     if ( (ret = openvzReadConfigParam(veid, "NETIF", temp, sizeof(temp))) <= 0) {
         snprintf(temp, sizeof(temp), "eth0");
     } else {
+        char *saveptr;
         char   *s;
         int     max = 0;
 
         /* get maximum interface number (actually, it is the last one) */
-        for (s=strtok(temp, ";"); s; s=strtok(NULL, ";")) {
+        for (s=strtok_r(temp, ";", &saveptr); s; s=strtok_r(NULL, ";", &saveptr)) {
             int x;
 
             if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL;
diff --git a/src/qemu_driver.c b/src/qemu_driver.c
--- a/src/qemu_driver.c
+++ b/src/qemu_driver.c
@@ -265,7 +265,6 @@ cleanup:
 static int
 qemudStartup(void) {
     uid_t uid = geteuid();
-    struct passwd *pw;
     char *base = NULL;
     char driverConf[PATH_MAX];
 
@@ -304,7 +303,11 @@ qemudStartup(void) {
                       "%s/run/libvirt/qemu/", LOCAL_STATE_DIR) == -1)
             goto out_of_memory;
     } else {
-        if (!(pw = getpwuid(uid))) {
+        char buf[1024];
+        struct passwd pwbuf;
+        struct passwd *pw;
+
+        if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
             qemudLog(QEMUD_ERR, _("Failed to find user record for uid '%d': %s\n"),
                      uid, strerror(errno));
             goto error;
diff --git a/src/remote_internal.c b/src/remote_internal.c
--- a/src/remote_internal.c
+++ b/src/remote_internal.c
@@ -603,10 +603,12 @@ doRemoteOpen (virConnectPtr conn,
     case trans_unix: {
         if (!sockname) {
             if (flags & VIR_DRV_OPEN_REMOTE_USER) {
+                char buf[1024];
+                struct passwd pwbuf;
                 struct passwd *pw;
                 uid_t uid = getuid();
 
-                if (!(pw = getpwuid(uid))) {
+                if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
                     virReportSystemError(conn, errno,
                                          _("unable to lookup user '%d'"),
                                          uid);
diff --git a/src/storage_backend_fs.c b/src/storage_backend_fs.c
--- a/src/storage_backend_fs.c
+++ b/src/storage_backend_fs.c
@@ -385,7 +385,8 @@ static int
 virStorageBackendFileSystemIsMounted(virConnectPtr conn,
                                      virStoragePoolObjPtr pool) {
     FILE *mtab;
-    struct mntent *ent;
+    struct mntent ent;
+    char buf[1024];
 
     if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) {
         virReportSystemError(conn, errno,
@@ -394,8 +395,8 @@ virStorageBackendFileSystemIsMounted(vir
         return -1;
     }
 
-    while ((ent = getmntent(mtab)) != NULL) {
-        if (STREQ(ent->mnt_dir, pool->def->target.path)) {
+    while ((getmntent_r(mtab, &ent, buf, sizeof(buf))) != NULL) {
+        if (STREQ(ent.mnt_dir, pool->def->target.path)) {
             fclose(mtab);
             return 1;
         }
diff --git a/src/storage_driver.c b/src/storage_driver.c
--- a/src/storage_driver.c
+++ b/src/storage_driver.c
@@ -108,7 +108,6 @@ storageDriverAutostart(virStorageDriverS
 static int
 storageDriverStartup(void) {
     uid_t uid = geteuid();
-    struct passwd *pw;
     char *base = NULL;
     char driverConf[PATH_MAX];
 
@@ -125,7 +124,11 @@ storageDriverStartup(void) {
         if ((base = strdup (SYSCONF_DIR "/libvirt")) == NULL)
             goto out_of_memory;
     } else {
-        if (!(pw = getpwuid(uid))) {
+        char buf[1024];
+        struct passwd pwbuf;
+        struct passwd *pw;
+
+        if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
             storageLog("Failed to find user record for uid '%d': %s",
                        uid, strerror(errno));
             goto out_of_memory;
diff --git a/src/uml_driver.c b/src/uml_driver.c
--- a/src/uml_driver.c
+++ b/src/uml_driver.c
@@ -309,9 +309,11 @@ cleanup:
 static int
 umlStartup(void) {
     uid_t uid = geteuid();
-    struct passwd *pw;
     char *base = NULL;
     char driverConf[PATH_MAX];
+    char buf[1024];
+    struct passwd pwbuf;
+    struct passwd *pw;
 
     if (VIR_ALLOC(uml_driver) < 0)
         return -1;
@@ -325,7 +327,7 @@ umlStartup(void) {
     /* Don't have a dom0 so start from 1 */
     uml_driver->nextvmid = 1;
 
-    if (!(pw = getpwuid(uid))) {
+    if (getpwuid_r(uid, &pwbuf, buf, sizeof(buf), &pw) != 0) {
         umlLog(VIR_LOG_ERROR, _("Failed to find user record for uid '%d': %s\n"),
                uid, strerror(errno));
         goto error;
diff --git a/src/xen_unified.c b/src/xen_unified.c
--- a/src/xen_unified.c
+++ b/src/xen_unified.c
@@ -258,9 +258,6 @@ xenUnifiedOpen (virConnectPtr conn, virC
 
     priv->handle = -1;
     priv->xendConfigVersion = -1;
-    priv->type = -1;
-    priv->len = -1;
-    priv->addr = NULL;
     priv->xshandle = NULL;
     priv->proxy = -1;
 
diff --git a/src/xen_unified.h b/src/xen_unified.h
--- a/src/xen_unified.h
+++ b/src/xen_unified.h
@@ -142,13 +142,11 @@ struct _xenUnifiedPrivate {
 
     int xendConfigVersion;      /* XenD config version */
 
-    /* XXX This code is not IPv6 aware. */
     /* connection to xend */
-    int type;                   /* PF_UNIX or PF_INET */
-    int len;                    /* length of addr */
-    struct sockaddr *addr;      /* type of address used */
-    struct sockaddr_un addr_un; /* the unix address */
-    struct sockaddr_in addr_in; /* the inet address */
+    struct sockaddr_storage addr;
+    socklen_t addrlen;
+    int addrfamily;
+    int addrprotocol;
 
     /* Keep track of the drivers which opened.  We keep a yes/no flag
      * here for each driver, corresponding to the array drivers in
diff --git a/src/xend_internal.c b/src/xend_internal.c
--- a/src/xend_internal.c
+++ b/src/xend_internal.c
@@ -61,30 +61,6 @@
 
 #endif /* PROXY */
 
-/**
- * xend_connection_type:
- *
- * The connection to the Xen Daemon can be done either though a normal TCP
- * socket or a local domain direct connection.
- */
-enum xend_connection_type {
-    XEND_DOMAIN,
-    XEND_TCP,
-};
-
-/**
- * xend:
- *
- * Structure associated to a connection to a Xen daemon
- */
-struct xend {
-    int len;
-    int type;
-    struct sockaddr *addr;
-    struct sockaddr_un addr_un;
-    struct sockaddr_in addr_in;
-};
-
 
 #ifndef PROXY
 static int
@@ -132,7 +108,7 @@ do_connect(virConnectPtr xend)
     int no_slow_start = 1;
     xenUnifiedPrivatePtr priv = (xenUnifiedPrivatePtr) xend->privateData;
 
-    s = socket(priv->type, SOCK_STREAM, 0);
+    s = socket(priv->addrfamily, SOCK_STREAM, priv->addrprotocol);
     if (s == -1) {
         virXendError(xend, VIR_ERR_INTERNAL_ERROR,
                      "%s", _("failed to create a socket"));
@@ -146,7 +122,7 @@ do_connect(virConnectPtr xend)
                sizeof(no_slow_start));
 
 
-    if (connect(s, priv->addr, priv->len) == -1) {
+    if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) {
         serrno = errno;
         close(s);
         errno = serrno;
@@ -804,18 +780,16 @@ xenDaemonOpen_unix(virConnectPtr conn, c
     if ((conn == NULL) || (path == NULL))
         return (-1);
 
-    addr = &priv->addr_un;
+    memset(&priv->addr, 0, sizeof(priv->addr));
+    priv->addrfamily = AF_UNIX;
+    priv->addrprotocol = PF_UNIX;
+    priv->addrlen = sizeof(struct sockaddr_un);
+
+    addr = (struct sockaddr_un *)&priv->addr;
     addr->sun_family = AF_UNIX;
     memset(addr->sun_path, 0, sizeof(addr->sun_path));
     strncpy(addr->sun_path, path, sizeof(addr->sun_path));
 
-    priv->len = sizeof(addr->sun_family) + strlen(addr->sun_path);
-    if ((unsigned int) priv->len > sizeof(addr->sun_path))
-        priv->len = sizeof(addr->sun_path);
-
-    priv->addr = (struct sockaddr *) addr;
-    priv->type = PF_UNIX;
-
     return (0);
 }
 
@@ -832,38 +806,71 @@ xenDaemonOpen_unix(virConnectPtr conn, c
  * Returns 0 in case of success, -1 in case of error.
  */
 static int
-xenDaemonOpen_tcp(virConnectPtr conn, const char *host, int port)
-{
-    struct in_addr ip;
-    struct hostent *pent;
-    xenUnifiedPrivatePtr priv;
-
-    if ((conn == NULL) || (host == NULL) || (port <= 0))
-        return (-1);
-
-    priv = (xenUnifiedPrivatePtr) conn->privateData;
-
-    pent = gethostbyname(host);
-    if (pent == NULL) {
-        if (inet_aton(host, &ip) == 0) {
-            virXendError(NULL, VIR_ERR_UNKNOWN_HOST,
-                         _("gethostbyname failed: %s"), host);
-            errno = ESRCH;
-            return (-1);
-        }
-    } else {
-        memcpy(&ip, pent->h_addr_list[0], sizeof(ip));
-    }
-
-    priv->len = sizeof(struct sockaddr_in);
-    priv->addr = (struct sockaddr *) &priv->addr_in;
-    priv->type = PF_INET;
-
-    priv->addr_in.sin_family = AF_INET;
-    priv->addr_in.sin_port = htons(port);
-    memcpy(&priv->addr_in.sin_addr, &ip, sizeof(ip));
-
-    return (0);
+xenDaemonOpen_tcp(virConnectPtr conn, const char *host, const char *port)
+{
+    xenUnifiedPrivatePtr priv;
+    struct addrinfo *res, *r;
+    struct addrinfo hints;
+    int saved_errno = EINVAL;
+    int ret;
+
+    if ((conn == NULL) || (host == NULL) || (port == NULL))
+        return (-1);
+
+    priv = (xenUnifiedPrivatePtr) conn->privateData;
+
+    priv->addrlen = 0;
+    memset(&priv->addr, 0, sizeof(priv->addr));
+
+    // http://people.redhat.com/drepper/userapi-ipv6.html
+    memset (&hints, 0, sizeof hints);
+    hints.ai_socktype = SOCK_STREAM;
+    hints.ai_flags = AI_ADDRCONFIG;
+
+    ret = getaddrinfo (host, port, &hints, &res);
+    if (ret != 0) {
+        virXendError(NULL, VIR_ERR_UNKNOWN_HOST,
+                     _("unable to resolve hostname '%s': %s"),
+                     host, gai_strerror (ret));
+        return -1;
+    }
+
+    /* Try to connect to each returned address in turn. */
+    for (r = res; r; r = r->ai_next) {
+        int sock;
+
+        sock = socket (r->ai_family, SOCK_STREAM, r->ai_protocol);
+        if (sock == -1) {
+            saved_errno = errno;
+            continue;
+        }
+
+        if (connect (sock, r->ai_addr, r->ai_addrlen) == -1) {
+            saved_errno = errno;
+            close (sock);
+            continue;
+        }
+
+        priv->addrlen = r->ai_addrlen;
+        priv->addrfamily = r->ai_family;
+        priv->addrprotocol = r->ai_protocol;
+        memcpy(&priv->addr,
+               r->ai_addr,
+               r->ai_addrlen);
+        close(sock);
+        break;
+    }
+
+    freeaddrinfo (res);
+
+    if (!priv->addrlen) {
+        virReportSystemError(conn, saved_errno,
+                             _("unable to connect to '%s:%s'"),
+                             host, port);
+        return -1;
+    }
+
+    return 0;
 }
 
 
@@ -2765,14 +2772,18 @@ xenDaemonOpen(virConnectPtr conn,
         /*
          * try though http on port 8000
          */
-        ret = xenDaemonOpen_tcp(conn, "localhost", 8000);
+        ret = xenDaemonOpen_tcp(conn, "localhost", "8000");
         if (ret < 0)
             goto failed;
         ret = xend_detect_config_version(conn);
         if (ret == -1)
             goto failed;
     } else if (STRCASEEQ (conn->uri->scheme, "http")) {
-        ret = xenDaemonOpen_tcp(conn, conn->uri->server, conn->uri->port);
+        char *port;
+        if (virAsprintf(&port, "%d", conn->uri->port) == -1)
+            goto failed;
+        ret = xenDaemonOpen_tcp(conn, conn->uri->server, port);
+        VIR_FREE(port);
         if (ret < 0)
             goto failed;
         ret = xend_detect_config_version(conn);

-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|


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