Re: [libvirt] [PATCH v3 2/3] conf: add check if migration_host is a localhost address

On 10/07/2014 06:07 AM, Chen Fan wrote: > Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; > --- > src/libvirt_private.syms | 1 + > src/qemu/qemu_conf.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++ > src/qemu/qemu_conf.h | 2 ++ > src/util/virsocketaddr.c | 24 +++++++++++++++++++++++ > src/util/virsocketaddr.h | 2 ++ > 5 files changed, 79 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 8ab1394..a104bc6 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1911,6 +1911,7 @@ virSocketAddrGetIpPrefix; > virSocketAddrGetPort; > virSocketAddrGetRange; > virSocketAddrIsNetmask; > +virSocketAddrIsNumericLocalhost; > virSocketAddrIsPrivate; > virSocketAddrIsWildcard; > virSocketAddrMask; > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index adc6caf..6b0ac5c 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -707,6 +707,15 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, > GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox); > > GET_VALUE_STR("migration_host", cfg->migrateHost); > + if (cfg->migrateHost && > + qemuCheckLocalhost(cfg->migrateHost)) { > + virReportError(VIR_ERR_CONF_SYNTAX, > + _("migration_host must not be the address of" > + " the local machine: %s"), > + cfg->migrateHost); > + goto cleanup; > + } > + > GET_VALUE_STR("migration_address", cfg->migrationAddress); > > GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); > @@ -1371,3 +1380,44 @@ qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, > > return qemuGetHugepagePath(&hugetlbfs[i]); > } > + > +bool > +qemuCheckLocalhost(const char *addrStr) > +{ > + virSocketAddr addr; > + char *hostname, *tmp; > + bool encloseAddress = false; > + int family; > + bool ret = true; > + > + if (VIR_STRDUP(hostname, addrStr) < 0) > + return false; > + > + tmp = hostname; > + > + if (STRPREFIX(hostname, "[")) { > + char *end = strchr(hostname, ']'); > + if (end) { > + *end = '\0'; > + hostname++; > + encloseAddress = true; > + } > + } We don't format the qemu.conf back and we don't need the brackets for anything. We can just store the migration host without them in cfg->migrationHost > + > + if (STRPREFIX(hostname, "localhost")) > + goto cleanup; > + > + family = virSocketAddrNumericFamily(hostname); > + if ((family == AF_INET && !encloseAddress) || > + family == AF_INET6) { > + if (virSocketAddrParse(&addr, hostname, family) > 0 && > + virSocketAddrIsNumericLocalhost(&addr)) { > + goto cleanup; > + } > + } There's no need to check for family upfront. > + > + ret = false; > +cleanup: > + VIR_FREE(tmp); > + return ret; > +} > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index cb01fb6..c9ce53c 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -322,4 +322,6 @@ int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn, > char * qemuGetHugepagePath(virHugeTLBFSPtr hugepage); > char * qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, > size_t nhugetlbfs); > + > +bool qemuCheckLocalhost(const char *addrStr); > #endif /* __QEMUD_CONF_H */ > diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c > index 7fe7a15..6d36689 100644 > --- a/src/util/virsocketaddr.c > +++ b/src/util/virsocketaddr.c > @@ -878,3 +878,27 @@ virSocketAddrNumericFamily(const char *address) > freeaddrinfo(res); > return family; > } > + > +/** > + * virSocketAddrIsNumericLocalhost: > + * @address: address to check > + * > + * Check if passed address is a numeric 'localhost' address. > + * > + * Returns: true if @address is a numeric 'localhost' address, > + * false otherwise > + */ > +bool > +virSocketAddrIsNumericLocalhost(const virSocketAddr *addr) I've rewritten this function to take a 'const char *' argument. Along with the virStringStripIPv6Brackets function I've sent for review separately, this removes the need for a separate qemuCheckLocalhost function and it can be inlined. > +{ > + struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) }; > + switch (addr->data.stor.ss_family) { > + case AF_INET: > + return memcmp(&addr->data.inet4.sin_addr.s_addr, &tmp.s_addr, > + sizeof(addr->data.inet4.sin_addr.s_addr)) == 0; > + case AF_INET6: > + return IN6_IS_ADDR_LOOPBACK(&addr->data.inet6.sin6_addr); > + } > + return false; > + > +} The diff I'll squash into this patch before pushing (after virStringStripIPv6Brackets is sorted out):

--- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -707,8 +707,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_LONG("seccomp_sandbox", cfg->seccompSandbox); GET_VALUE_STR("migration_host", cfg->migrateHost); + virStringStripIPv6Brackets(cfg->migrateHost); if (cfg->migrateHost && - qemuCheckLocalhost(cfg->migrateHost)) { + (STRPREFIX(cfg->migrateHost, "localhost") || + virSocketAddrIsNumericLocalhost(cfg->migrateHost))) { virReportError(VIR_ERR_CONF_SYNTAX, _("migration_host must not be the address of" " the local machine: %s"), @@ -1380,44 +1382,3 @@ qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, return qemuGetHugepagePath(&hugetlbfs[i]); } - -bool -qemuCheckLocalhost(const char *addrStr) -{ - virSocketAddr addr; - char *hostname, *tmp; - bool encloseAddress = false; - int family; - bool ret = true; - - if (VIR_STRDUP(hostname, addrStr) < 0) - return false; - - tmp = hostname; - - if (STRPREFIX(hostname, "[")) { - char *end = strchr(hostname, ']'); - if (end) { - *end = '\0'; - hostname++; - encloseAddress = true; - } - } - - if (STRPREFIX(hostname, "localhost")) - goto cleanup; - - family = virSocketAddrNumericFamily(hostname); - if ((family == AF_INET && !encloseAddress) || - family == AF_INET6) { - if (virSocketAddrParse(&addr, hostname, family) > 0 && - virSocketAddrIsNumericLocalhost(&addr)) { - goto cleanup; - } - } - - ret = false; -cleanup: - VIR_FREE(tmp); - return ret; -} diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index c9ce53c..cb01fb6 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -322,6 +322,4 @@ int qemuTranslateSnapshotDiskSourcePool(virConnectPtr conn, char * qemuGetHugepagePath(virHugeTLBFSPtr hugepage); char * qemuGetDefaultHugepath(virHugeTLBFSPtr hugetlbfs, size_t nhugetlbfs); - -bool qemuCheckLocalhost(const char *addrStr); #endif /* __QEMUD_CONF_H */ diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 6d36689..a19e3af 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -889,15 +889,24 @@ virSocketAddrNumericFamily(const char *address) * false otherwise */ bool -virSocketAddrIsNumericLocalhost(const virSocketAddr *addr) +virSocketAddrIsNumericLocalhost(const char *addr) { + struct addrinfo *res; struct in_addr tmp = { .s_addr = htonl(INADDR_LOOPBACK) }; - switch (addr->data.stor.ss_family) { + struct sockaddr_in *inet4; + struct sockaddr_in6 *inet6; + + if (virSocketAddrParseInternal(&res, addr, AF_UNSPEC, false) < 0) + return false; + + switch (res->ai_addr->sa_family) { case AF_INET: - return memcmp(&addr->data.inet4.sin_addr.s_addr, &tmp.s_addr, - sizeof(addr->data.inet4.sin_addr.s_addr)) == 0; + inet4 = (struct sockaddr_in*) res->ai_addr; + return memcmp(&inet4->sin_addr.s_addr, &tmp.s_addr, + sizeof(inet4->sin_addr.s_addr)) == 0; case AF_INET6: - return IN6_IS_ADDR_LOOPBACK(&addr->data.inet6.sin6_addr); + inet6 = (struct sockaddr_in6*) res->ai_addr; + return IN6_IS_ADDR_LOOPBACK(&(inet6->sin6_addr)); } return false; diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index fa9e98b..053855b 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -127,5 +127,5 @@ bool virSocketAddrIsWildcard(const virSocketAddr *addr); int virSocketAddrNumericFamily(const char *address); -bool virSocketAddrIsNumericLocalhost(const virSocketAddr *addr); +bool virSocketAddrIsNumericLocalhost(const char *addr); #endif /* __VIR_SOCKETADDR_H__ */ Jan