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

2014-10-15 Thread John Ferlan 
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.f...@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

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

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

2014-10-15 Thread Chen, Fan 
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.f...@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


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

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

2014-10-08 Thread Ján Tomko 
On 10/07/2014 06:07 AM, Chen Fan wrote:
  Signed-off-by: Chen Fan chen.fan.f...@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 
-

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

2014-10-08 Thread Chen, Fan 
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.f...@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