[PATCH 0/3] tools: Miscellanous fixes for 4.4

[PATCH 0/3] tools: Miscellanous fixes for 4.4

[Ian Jackson]

Here are three bugfixes.  1/3 is quite important.  I'm CCing it to Jim
Fehlig in case he happens to see fallout from it with libvirt.

 1/3 libxl: events: Pass correct nfds to poll
 2/3 xl: Free optdata_begin when saving domain config
 3/3 xenstore: xs_suspend_evtchn_port: always free portstr

These are based on staging, but they will also apply without trouble
to my recent libxl fork series.

[PATCH 1/3] libxl: events: Pass correct nfds to poll

[Ian Jackson]

libxl_event.c:eventloop_iteration would pass the allocated pollfds
array size, rather than the used size, to poll (and to
afterpoll_internal).

The effect is that if the number of fds to poll on reduces, libxl will
poll on stale entries.  Because of the way the return value from poll
is processed these stale entries are often harmless because any events
coming back from poll ignored by libxl.  However, it could cause
malfunctions:

It could result in unwanted SIGTTIN/SIGTTOU/SIGPIPE, for example, if
the fd has been reused to refer to an object which can generate those
signals.  Alternatively, it could result in libxl spinning if the
stale entry refers to an fd which happens now to be ready for the
previously-requested operation.

I have tested this with a localhost migration and inspected the strace
output.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Jim Fehlig <jfehlig@suse.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/libxl_event.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
index bdef7ac..1c48fee 100644
--- a/tools/libxl/libxl_event.c
+++ b/tools/libxl/libxl_event.c
@@ -1386,7 +1386,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
      * can unlock it when it polls.
      */
     EGC_GC;
-    int rc;
+    int rc, nfds;
     struct timeval now;
     
     rc = libxl__gettimeofday(gc, &now);
@@ -1395,7 +1395,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
     int timeout;
 
     for (;;) {
-        int nfds = poller->fd_polls_allocd;
+        nfds = poller->fd_polls_allocd;
         timeout = -1;
         rc = beforepoll_internal(gc, poller, &nfds, poller->fd_polls,
                                  &timeout, now);
@@ -1413,7 +1413,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
     }
 
     CTX_UNLOCK;
-    rc = poll(poller->fd_polls, poller->fd_polls_allocd, timeout);
+    rc = poll(poller->fd_polls, nfds, timeout);
     CTX_LOCK;
 
     if (rc < 0) {
@@ -1428,8 +1428,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
     rc = libxl__gettimeofday(gc, &now);
     if (rc) goto out;
 
-    afterpoll_internal(egc, poller,
-                       poller->fd_polls_allocd, poller->fd_polls, now);
+    afterpoll_internal(egc, poller, nfds, poller->fd_polls, now);
 
     rc = 0;
  out:
-- 
1.7.10.4

Re: [PATCH 1/3] libxl: events: Pass correct nfds to poll

[Ian Campbell]

On Fri, 2014-01-17 at 17:34 +0000, Ian Jackson wrote:
> libxl_event.c:eventloop_iteration would pass the allocated pollfds
> array size, rather than the used size, to poll (and to
> afterpoll_internal).
> 
> The effect is that if the number of fds to poll on reduces, libxl will
> poll on stale entries.  Because of the way the return value from poll
> is processed these stale entries are often harmless because any events
> coming back from poll ignored by libxl.  However, it could cause
> malfunctions:
> 
> It could result in unwanted SIGTTIN/SIGTTOU/SIGPIPE, for example, if
> the fd has been reused to refer to an object which can generate those
> signals.  Alternatively, it could result in libxl spinning if the
> stale entry refers to an fd which happens now to be ready for the
> previously-requested operation.
> 
> I have tested this with a localhost migration and inspected the strace
> output.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Jim Fehlig <jfehlig@suse.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> ---
>  tools/libxl/libxl_event.c |    9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index bdef7ac..1c48fee 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -1386,7 +1386,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
>       * can unlock it when it polls.
>       */
>      EGC_GC;
> -    int rc;
> +    int rc, nfds;
>      struct timeval now;
>      
>      rc = libxl__gettimeofday(gc, &now);
> @@ -1395,7 +1395,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
>      int timeout;
>  
>      for (;;) {
> -        int nfds = poller->fd_polls_allocd;
> +        nfds = poller->fd_polls_allocd;
>          timeout = -1;
>          rc = beforepoll_internal(gc, poller, &nfds, poller->fd_polls,
>                                   &timeout, now);
> @@ -1413,7 +1413,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
>      }
>  
>      CTX_UNLOCK;
> -    rc = poll(poller->fd_polls, poller->fd_polls_allocd, timeout);
> +    rc = poll(poller->fd_polls, nfds, timeout);
>      CTX_LOCK;
>  
>      if (rc < 0) {
> @@ -1428,8 +1428,7 @@ static int eventloop_iteration(libxl__egc *egc, libxl__poller *poller) {
>      rc = libxl__gettimeofday(gc, &now);
>      if (rc) goto out;
>  
> -    afterpoll_internal(egc, poller,
> -                       poller->fd_polls_allocd, poller->fd_polls, now);
> +    afterpoll_internal(egc, poller, nfds, poller->fd_polls, now);
>  
>      rc = 0;
>   out:

[PATCH 2/3] xl: Free optdata_begin when saving domain config

[Ian Jackson]

This makes valgrind a bit happier.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d93e01b..aff6f90 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3442,6 +3442,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
                      ctx, fd, optdata_begin, hdr.optional_data_len,
                      source, "header"));
 
+    free(optdata_begin);
+
     fprintf(stderr, "Saving to %s new xl format (info"
             " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
             source, hdr.mandatory_flags, hdr.optional_flags,
-- 
1.7.10.4

Re: [PATCH 2/3] xl: Free optdata_begin when saving domain config

[Andrew Cooper]

On 17/01/14 17:34, Ian Jackson wrote:
> This makes valgrind a bit happier.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>

This is CID: 1055903

It is probably worth noting that there are 11 other resource leaks
Coverity has found in this file.

~Andrew

> ---
>  tools/libxl/xl_cmdimpl.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d93e01b..aff6f90 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3442,6 +3442,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
>                       ctx, fd, optdata_begin, hdr.optional_data_len,
>                       source, "header"));
>  
> +    free(optdata_begin);
> +
>      fprintf(stderr, "Saving to %s new xl format (info"
>              " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
>              source, hdr.mandatory_flags, hdr.optional_flags,

Re: [PATCH 2/3] xl: Free optdata_begin when saving domain config

[Ian Campbell]

On Fri, 2014-01-17 at 17:34 +0000, Ian Jackson wrote:
> This makes valgrind a bit happier.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> ---
>  tools/libxl/xl_cmdimpl.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index d93e01b..aff6f90 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3442,6 +3442,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
>                       ctx, fd, optdata_begin, hdr.optional_data_len,
>                       source, "header"));
>  
> +    free(optdata_begin);
> +
>      fprintf(stderr, "Saving to %s new xl format (info"
>              " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
>              source, hdr.mandatory_flags, hdr.optional_flags,

[PATCH 3/3] xenstore: xs_suspend_evtchn_port: always free portstr

[Ian Jackson]

If portstr!=NULL but plen==0 this function would leak portstr.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/xenstore/xs.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
index a636498..9cd99eb 100644
--- a/tools/xenstore/xs.c
+++ b/tools/xenstore/xs.c
@@ -1095,12 +1095,15 @@ int xs_suspend_evtchn_port(int domid)
     portstr = xs_read(xs, XBT_NULL, path, &plen);
     xs_daemon_close(xs);
 
-    if (!portstr || !plen)
-        return -1;
+    if (!portstr || !plen) {
+        port = -1;
+	goto out;
+    }
 
     port = atoi(portstr);
-    free(portstr);
 
+out:
+    free(portstr);
     return port;
 }
 
-- 
1.7.10.4

Re: [PATCH 3/3] xenstore: xs_suspend_evtchn_port: always free portstr

[Andrew Cooper]

On 17/01/14 17:34, Ian Jackson wrote:
> If portstr!=NULL but plen==0 this function would leak portstr.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Cc: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  tools/xenstore/xs.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index a636498..9cd99eb 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -1095,12 +1095,15 @@ int xs_suspend_evtchn_port(int domid)
>      portstr = xs_read(xs, XBT_NULL, path, &plen);
>      xs_daemon_close(xs);
>  
> -    if (!portstr || !plen)
> -        return -1;
> +    if (!portstr || !plen) {
> +        port = -1;
> +	goto out;

You have mixed tabs/spaces here.

~Andrew

> +    }
>  
>      port = atoi(portstr);
> -    free(portstr);
>  
> +out:
> +    free(portstr);
>      return port;
>  }
>  

Re: [PATCH 3/3] xenstore: xs_suspend_evtchn_port: always free portstr

[Ian Campbell]

On Fri, 2014-01-17 at 17:34 +0000, Ian Jackson wrote:
> If portstr!=NULL but plen==0 this function would leak portstr.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

With the whitespace fixed:

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>

> ---
>  tools/xenstore/xs.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/xenstore/xs.c b/tools/xenstore/xs.c
> index a636498..9cd99eb 100644
> --- a/tools/xenstore/xs.c
> +++ b/tools/xenstore/xs.c
> @@ -1095,12 +1095,15 @@ int xs_suspend_evtchn_port(int domid)
>      portstr = xs_read(xs, XBT_NULL, path, &plen);
>      xs_daemon_close(xs);
>  
> -    if (!portstr || !plen)
> -        return -1;
> +    if (!portstr || !plen) {
> +        port = -1;
> +	goto out;
> +    }
>  
>      port = atoi(portstr);
> -    free(portstr);
>  
> +out:
> +    free(portstr);
>      return port;
>  }
>  

[PATCH v2 0/3] tools: Fixes for 4.4

[Ian Jackson]

I think these three bugfixes are clear 4.4 material.

 a 1/3 libxl: events: Pass correct nfds to poll
 a 2/3 xl: Free optdata_begin when saving domain config
 a 3/3 xenstore: xs_suspend_evtchn_port: always free portstr

They have all been acked by Ian Campbell.

Thanks,
Ian.

[PATCH 2/3] xl: Free optdata_begin when saving domain config

[Ian Jackson]

This makes valgrind a bit happier.

It is also
Coverity-CID: 1055903

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: Ian Campbell <Ian.Campbell@citrix.com>
Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index d93e01b..aff6f90 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3442,6 +3442,8 @@ static void save_domain_core_writeconfig(int fd, const char *source,
                      ctx, fd, optdata_begin, hdr.optional_data_len,
                      source, "header"));
 
+    free(optdata_begin);
+
     fprintf(stderr, "Saving to %s new xl format (info"
             " 0x%"PRIx32"/0x%"PRIx32"/%"PRIu32")\n",
             source, hdr.mandatory_flags, hdr.optional_flags,
-- 
1.7.10.4