[libvirt] [PATCH v3 0/3] Check migration configuration
add some check in migration configuration. Chen Fan (3): migration: add migration_host support for Ipv6 address without brackets conf: add check if migration_host is a localhost address conf: Check migration_address whether is localhost src/libvirt_private.syms | 3 +- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 58 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_migration.c | 49 ++++++++++++++++---------------- src/qemu/test_libvirtd_qemu.aug.in | 2 +- src/util/virsocketaddr.c | 43 +++++++++++++++++++++------- src/util/virsocketaddr.h | 4 ++- tests/sockettest.c | 2 +- 9 files changed, 125 insertions(+), 40 deletions(-) -- 1.9.3
if specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it. Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++------------------------ src/util/virsocketaddr.c | 19 +++++++++--------- src/util/virsocketaddr.h | 2 +- tests/sockettest.c | 2 +- 5 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7cbc35b..8ab1394 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1911,11 +1911,11 @@ virSocketAddrGetIpPrefix; virSocketAddrGetPort; virSocketAddrGetRange; virSocketAddrIsNetmask; -virSocketAddrIsNumeric; virSocketAddrIsPrivate; virSocketAddrIsWildcard; virSocketAddrMask; virSocketAddrMaskByPrefix; +virSocketAddrNumericFamily; virSocketAddrParse; virSocketAddrParseIPv4; virSocketAddrParseIPv6; diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 284cd5a..e135249 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,7 +2605,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, if (VIR_STRDUP(migrateFrom, "stdio") < 0) goto cleanup; } else { - virSocketAddr listenAddressSocket; bool encloseAddress = false; bool hostIPv6Capable = false; bool qemuIPv6Capable = false; @@ -2627,28 +2626,21 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, virObjectUnref(qemuCaps); if (listenAddress) { - if (virSocketAddrIsNumeric(listenAddress)) { - /* listenAddress is numeric IPv4 or IPv6 */ - if (virSocketAddrParse(&listenAddressSocket, listenAddress, AF_UNSPEC) < 0) + if (virSocketAddrNumericFamily(listenAddress) == AF_INET6) { + if (!qemuIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("qemu isn't capable of IPv6")); goto cleanup; - - /* address parsed successfully */ - if (VIR_SOCKET_ADDR_IS_FAMILY(&listenAddressSocket, AF_INET6)) { - if (!qemuIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("qemu isn't capable of IPv6")); - goto cleanup; - } - if (!hostIPv6Capable) { - virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", - _("host isn't capable of IPv6")); - goto cleanup; - } - /* IPv6 address must be escaped in brackets on the cmd line */ - encloseAddress = true; } + if (!hostIPv6Capable) { + virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", + _("host isn't capable of IPv6")); + goto cleanup; + } + /* IPv6 address must be escaped in brackets on the cmd line */ + encloseAddress = true; } else { - /* listenAddress is a hostname */ + /* listenAddress is a hostname or IPv4 */ } } else if (qemuIPv6Capable && hostIPv6Capable) { /* Listen on :: instead of 0.0.0.0 if QEMU understands it @@ -2950,15 +2942,17 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * to be a correct hostname which refers to the target machine). */ if (uri_in == NULL) { + bool encloseAddress = false; + const char *incFormat; + if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0) goto cleanup; if (migrateHost != NULL) { - if (virSocketAddrIsNumeric(migrateHost) && - virSocketAddrParse(NULL, migrateHost, AF_UNSPEC) < 0) - goto cleanup; + if (virSocketAddrNumericFamily(migrateHost) == AF_INET6) + encloseAddress = true; - if (VIR_STRDUP(hostname, migrateHost) < 0) + if (VIR_STRDUP(hostname, migrateHost) < 0) goto cleanup; } else { if ((hostname = virGetHostname()) == NULL) @@ -2977,7 +2971,12 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, * compatibility with old targets. We at least make the * new targets accept both syntaxes though. */ - if (virAsprintf(uri_out, "tcp:%s:%d", hostname, port) < 0) + if (encloseAddress) + incFormat = "%s:[%s]:%d"; + else + incFormat = "%s:%s:%d"; + + if (virAsprintf(uri_out, incFormat, "tcp", hostname, port) < 0) goto cleanup; } else { bool well_formed_uri; diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 7cc4bde..7fe7a15 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -856,26 +856,25 @@ virSocketAddrGetIpPrefix(const virSocketAddr *address, } /** - * virSocketAddrIsNumeric: + * virSocketAddrNumericFamily: * @address: address to check * - * Check if passed address is an IP address in numeric format. For - * instance, for 0.0.0.0 true is returned, for 'examplehost" - * false is returned. + * Check if passed address is an IP address in numeric format. and + * return the address family, otherwise return 0. * - * Returns: true if @address is an IP address, - * false otherwise + * Returns: AF_INET or AF_INET6 if @address is an numeric IP address, + * -1 otherwise. */ -bool -virSocketAddrIsNumeric(const char *address) +int +virSocketAddrNumericFamily(const char *address) { struct addrinfo *res; unsigned short family; if (virSocketAddrParseInternal(&res, address, AF_UNSPEC, false) < 0) - return false; + return -1; family = res->ai_addr->sa_family; freeaddrinfo(res); - return family == AF_INET || family == AF_INET6; + return family; } diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 27defa0..35c9c1a 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -125,5 +125,5 @@ bool virSocketAddrIsPrivate(const virSocketAddr *addr); bool virSocketAddrIsWildcard(const virSocketAddr *addr); -bool virSocketAddrIsNumeric(const char *address); +int virSocketAddrNumericFamily(const char *address); #endif /* __VIR_SOCKETADDR_H__ */ diff --git a/tests/sockettest.c b/tests/sockettest.c index 68b0536..abf7350 100644 --- a/tests/sockettest.c +++ b/tests/sockettest.c @@ -229,7 +229,7 @@ testIsNumericHelper(const void *opaque) { const struct testIsNumericData *data = opaque; - if (virSocketAddrIsNumeric(data->addr)) + if (virSocketAddrNumericFamily(data->addr) != -1) return data->pass ? 0 : -1; return data->pass ? -1 : 0; } -- 1.9.3
On 10/07/2014 06:07 AM, Chen Fan wrote:
if specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++------------------------ src/util/virsocketaddr.c | 19 +++++++++--------- src/util/virsocketaddr.h | 2 +- tests/sockettest.c | 2 +- 5 files changed, 36 insertions(+), 38 deletions(-)
ACK Jan
On Tue, 2014-10-07 at 11:08 +0200, Ján Tomko wrote:
On 10/07/2014 06:07 AM, Chen Fan wrote:
if specifying migration_host to an Ipv6 address without brackets, it was resolved to an incorrect address, such as: tcp:2001:0DB8::1428:4444, but the correct address should be: tcp:[2001:0DB8::1428]:4444 so we should add brackets when parsing it.
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/libvirt_private.syms | 2 +- src/qemu/qemu_migration.c | 49 +++++++++++++++++++++++------------------------ src/util/virsocketaddr.c | 19 +++++++++--------- src/util/virsocketaddr.h | 2 +- tests/sockettest.c | 2 +- 5 files changed, 36 insertions(+), 38 deletions(-)
ACK
Thanks.
Jan
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- 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; + } + } + + 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 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) +{ + 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; + +} diff --git a/src/util/virsocketaddr.h b/src/util/virsocketaddr.h index 35c9c1a..fa9e98b 100644 --- a/src/util/virsocketaddr.h +++ b/src/util/virsocketaddr.h @@ -126,4 +126,6 @@ bool virSocketAddrIsPrivate(const virSocketAddr *addr); bool virSocketAddrIsWildcard(const virSocketAddr *addr); int virSocketAddrNumericFamily(const char *address); + +bool virSocketAddrIsNumericLocalhost(const virSocketAddr *addr); #endif /* __VIR_SOCKETADDR_H__ */ -- 1.9.3
On 10/07/2014 06:07 AM, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- 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
On Wed, 2014-10-08 at 12:33 +0200, Ján Tomko wrote:
On 10/07/2014 06:07 AM, Chen Fan wrote:
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- 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): I had tested this diff and it works fine.
Thanks, Chen
--- 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
This patch has triggered a Coverity RESOURCE_LEAK (3 actually)
On 10/08/2014 09:54 PM, Chen, Fan wrote:
> On Wed, 2014-10-08 at 12:33 +0200, Ján Tomko wrote:
>> On 10/07/2014 06:07 AM, Chen Fan wrote:
>>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
>> 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;
>> +
'res' allocates something that must be free'd
>> + 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;
Leak #1
>> + 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;
Leak #2
>> + return IN6_IS_ADDR_LOOPBACK(&(inet6->sin6_addr));
>> }
Leak #3
>> return false;
>>
Other callers will call 'freeaddrinfo(res);'
In order to resolve - you probably need to create a local ret = false,
then assign ret = to either memcmp/IN6_IS_ADDR_LOOPBACK return, then
call freeaddrinfo() before return ret
John
On Wed, 2014-10-15 at 04:46 -0400, John Ferlan wrote:
> This patch has triggered a Coverity RESOURCE_LEAK (3 actually)
Right, I will make a patch to fix it.
Thank you for catching that.
>
> On 10/08/2014 09:54 PM, Chen, Fan wrote:
> > On Wed, 2014-10-08 at 12:33 +0200, Ján Tomko wrote:
> >> On 10/07/2014 06:07 AM, Chen Fan wrote:
> >>> Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
> >> 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;
> >> +
>
> 'res' allocates something that must be free'd
>
>
> >> + 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;
>
> Leak #1
>
> >> + 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;
>
> Leak #2
>
> >> + return IN6_IS_ADDR_LOOPBACK(&(inet6->sin6_addr));
> >> }
>
> Leak #3
> >> return false;
> >>
>
> Other callers will call 'freeaddrinfo(res);'
>
> In order to resolve - you probably need to create a local ret = false,
> then assign ret = to either memcmp/IN6_IS_ADDR_LOOPBACK return, then
> call freeaddrinfo() before return ret
>
> John
When enabling the migration_address option, by default it is set to "127.0.0.1", but it's not a valid address for migration. so we should add verification and set the default migration_address to "0.0.0.0". Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com> --- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 8 ++++++++ src/qemu/test_libvirtd_qemu.aug.in | 2 +- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index 92ca715..c6db568 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -467,7 +467,7 @@ # Override the listen address for all incoming migrations. Defaults to # 0.0.0.0, or :: if both host and qemu are capable of IPv6. -#migration_address = "127.0.0.1" +#migration_address = "0.0.0.0" # The default hostname or IP address which will be used by a migration diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 6b0ac5c..f34fa06 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -717,6 +717,14 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, } GET_VALUE_STR("migration_address", cfg->migrationAddress); + if (cfg->migrationAddress && + qemuCheckLocalhost(cfg->migrationAddress)) { + virReportError(VIR_ERR_CONF_SYNTAX, + _("migration_address must not be the address of" + " the local machine: %s"), + cfg->migrationAddress); + goto cleanup; + } GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp); diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index d2bc2c0..30fd27e 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -69,7 +69,7 @@ module Test_libvirtd_qemu = { "keepalive_interval" = "5" } { "keepalive_count" = "5" } { "seccomp_sandbox" = "1" } -{ "migration_address" = "127.0.0.1" } +{ "migration_address" = "0.0.0.0" } { "migration_host" = "host.example.com" } { "migration_port_min" = "49152" } { "migration_port_max" = "49215" } -- 1.9.3
On 10/07/2014 06:07 AM, Chen Fan wrote:
add some check in migration configuration.
Chen Fan (3): migration: add migration_host support for Ipv6 address without brackets conf: add check if migration_host is a localhost address conf: Check migration_address whether is localhost
src/libvirt_private.syms | 3 +- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 58 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_migration.c | 49 ++++++++++++++++---------------- src/qemu/test_libvirtd_qemu.aug.in | 2 +- src/util/virsocketaddr.c | 43 +++++++++++++++++++++------- src/util/virsocketaddr.h | 4 ++- tests/sockettest.c | 2 +- 9 files changed, 125 insertions(+), 40 deletions(-)
Now pushed. Jan
On Thu, 2014-10-09 at 14:54 +0200, Ján Tomko wrote:
On 10/07/2014 06:07 AM, Chen Fan wrote:
add some check in migration configuration.
Chen Fan (3): migration: add migration_host support for Ipv6 address without brackets conf: add check if migration_host is a localhost address conf: Check migration_address whether is localhost
src/libvirt_private.syms | 3 +- src/qemu/qemu.conf | 2 +- src/qemu/qemu_conf.c | 58 ++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_conf.h | 2 ++ src/qemu/qemu_migration.c | 49 ++++++++++++++++---------------- src/qemu/test_libvirtd_qemu.aug.in | 2 +- src/util/virsocketaddr.c | 43 +++++++++++++++++++++------- src/util/virsocketaddr.h | 4 ++- tests/sockettest.c | 2 +- 9 files changed, 125 insertions(+), 40 deletions(-)
Now pushed.
Thanks, Chen
Jan
participants (4)
-
Chen Fan -
Chen, Fan
-
John Ferlan -
Ján Tomko