[PATCH 1/3] tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open

[PATCH 1/3] tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open

[PRAMOD DEVENDRA]

From: Pramod Devendra <pramod.devendra@citrix.com>

Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_qmp.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index c7324e6..1080162 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -369,10 +369,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
     ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
     if (ret) return -1;
 
+    if(sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path))
+        return -1;
+
     memset(&qmp->addr, 0, sizeof (qmp->addr));
     qmp->addr.sun_family = AF_UNIX;
     strncpy(qmp->addr.sun_path, qmp_socket_path,
-            sizeof (qmp->addr.sun_path));
+            sizeof (qmp->addr.sun_path)-1);
 
     do {
         ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
-- 
1.7.10.4

Re: [PATCH 1/3] tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open

[Wei Liu]

On Mon, Mar 16, 2015 at 10:05:38AM +0000, PRAMOD DEVENDRA wrote:
> From: Pramod Devendra <pramod.devendra@citrix.com>
> 
> Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_qmp.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index c7324e6..1080162 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -369,10 +369,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
>      ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
>      if (ret) return -1;
>  
> +    if(sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path))
> +        return -1;
> +

I know this is not your fault, but the function seems to leak qmp_fd on
error path (qmp_fd is not closed). Do you fancy fixing that?

Wei.

>      memset(&qmp->addr, 0, sizeof (qmp->addr));
>      qmp->addr.sun_family = AF_UNIX;
>      strncpy(qmp->addr.sun_path, qmp_socket_path,
> -            sizeof (qmp->addr.sun_path));
> +            sizeof (qmp->addr.sun_path)-1);
>  
>      do {
>          ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
> -- 
> 1.7.10.4

Re: [PATCH 1/3] tools/libxl/libxl_qmp.c: Make sure sun_path is NULL terminated in qmp_open

[Ian Campbell]

On Mon, 2015-03-16 at 10:05 +0000, PRAMOD DEVENDRA wrote:

Please can you arrange in the future to chain your series into a single
thread. That means that each N/3 mail should have a References and/or
In-reply-to header pointing at either the 0/N mail or the (N-1)/N mail
in the series.

http://wiki.xen.org/wiki/Submitting_Xen_Patches#Sending_the_patches_to_the_list has some information on how to achieve this with git send-email.

Note that this also implies that you should include a 0/N mail which
ties the series together (i.e. explains the overall purpose of the
series). The wiki covers this too.

At the moment my mailbox contains 4 unthread mails from you, named 1/3,
2/3, 3/3 and 2/2 which is liable to get confusing if there are any
further revisions etc.

Ian.

[PATCH v2] libxl/libxl_qmp.c: fix qmp_open

[PRAMOD DEVENDRA]

From: Pramod Devendra <pramod.devendra@citrix.com>

Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
Changed since v1:
1. Make sure sun_path does not overflow.
2. Close qmp_fd on error.
---
 tools/libxl/libxl_qmp.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index c7324e6..316a93f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -357,22 +357,32 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid)
 static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
                     int timeout)
 {
-    int ret;
+    int ret = -1;
     int i = 0;
 
     qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0);
     if (qmp->qmp_fd < 0) {
-        return -1;
+        goto out;
     }
     ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1);
-    if (ret) return -1;
+    if (ret) {
+        ret = -1;
+        goto out;
+    }
     ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
-    if (ret) return -1;
+    if (ret) {
+        ret = -1;
+        goto out;
+    }
 
+    if (sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) {
+        ret = -1;
+        goto out;
+    }
     memset(&qmp->addr, 0, sizeof (qmp->addr));
     qmp->addr.sun_family = AF_UNIX;
     strncpy(qmp->addr.sun_path, qmp_socket_path,
-            sizeof (qmp->addr.sun_path));
+            sizeof (qmp->addr.sun_path)-1);
 
     do {
         ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
@@ -384,9 +394,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
              * ECONNREFUSED : Leftover socket hasn't been removed yet */
             continue;
         }
-        return -1;
+        ret = -1;
+        goto out;
     } while ((++i / 5 <= timeout) && (usleep(200 * 1000) <= 0));
 
+out:
+    if (ret == -1 && qmp->qmp_fd > -1) close(qmp->qmp_fd);
+
     return ret;
 }
 
-- 
1.7.10.4

Re: [PATCH v2] libxl/libxl_qmp.c: fix qmp_open

[Wei Liu]

On Thu, Mar 19, 2015 at 06:40:30AM +0000, PRAMOD DEVENDRA wrote:
> From: Pramod Devendra <pramod.devendra@citrix.com>
> 
> Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
> Changed since v1:
> 1. Make sure sun_path does not overflow.
> 2. Close qmp_fd on error.

These lines should be in the commit log itself. Putting them here makes
them get discarded when committing.

You also need to write about your third change, which was requested by
Ian. That is 

  3. Use "goto out" error handling idiom.

Wei.

Re: [PATCH v2] libxl/libxl_qmp.c: fix qmp_open

[Ian Campbell]

On Thu, 2015-03-19 at 10:03 +0000, Wei Liu wrote:
> On Thu, Mar 19, 2015 at 06:40:30AM +0000, PRAMOD DEVENDRA wrote:
> > From: Pramod Devendra <pramod.devendra@citrix.com>
> > 
> > Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
> > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> > CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: Ian Campbell <ian.campbell@citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Changed since v1:
> > 1. Make sure sun_path does not overflow.
> > 2. Close qmp_fd on error.
> 
> These lines should be in the commit log itself. Putting them here makes
> them get discarded when committing.
> 
> You also need to write about your third change, which was requested by
> Ian. That is 
> 
>   3. Use "goto out" error handling idiom.

Please also use a subject more descriptive than "Fix foo", describe the
fix somehow, since this combines multiple fixes perhaps "fix error
handling in qmp_open" and then describe the various fixes it in the main
bit of the message (i.e. the stuff about the path overflow and closing
the fd).

In general it is easier for everyone to split such changes into two,
then you can give each a useful and distinct subject.

> 
> Wei.

[PATCH v3] libxl/libxl_qmp.c: fix error handling in qmp_open

[PRAMOD DEVENDRA]

From: Pramod Devendra <pramod.devendra@citrix.com>

1. Make sure sun_path does not overflow
2. Close qmp_fd on error
3. Use goto out for error handling

Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxl/libxl_qmp.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index c7324e6..316a93f 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -357,22 +357,32 @@ static libxl__qmp_handler *qmp_init_handler(libxl__gc *gc, uint32_t domid)
 static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
                     int timeout)
 {
-    int ret;
+    int ret = -1;
     int i = 0;
 
     qmp->qmp_fd = socket(AF_UNIX, SOCK_STREAM, 0);
     if (qmp->qmp_fd < 0) {
-        return -1;
+        goto out;
     }
     ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1);
-    if (ret) return -1;
+    if (ret) {
+        ret = -1;
+        goto out;
+    }
     ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
-    if (ret) return -1;
+    if (ret) {
+        ret = -1;
+        goto out;
+    }
 
+    if (sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) {
+        ret = -1;
+        goto out;
+    }
     memset(&qmp->addr, 0, sizeof (qmp->addr));
     qmp->addr.sun_family = AF_UNIX;
     strncpy(qmp->addr.sun_path, qmp_socket_path,
-            sizeof (qmp->addr.sun_path));
+            sizeof (qmp->addr.sun_path)-1);
 
     do {
         ret = connect(qmp->qmp_fd, (struct sockaddr *) &qmp->addr,
@@ -384,9 +394,13 @@ static int qmp_open(libxl__qmp_handler *qmp, const char *qmp_socket_path,
              * ECONNREFUSED : Leftover socket hasn't been removed yet */
             continue;
         }
-        return -1;
+        ret = -1;
+        goto out;
     } while ((++i / 5 <= timeout) && (usleep(200 * 1000) <= 0));
 
+out:
+    if (ret == -1 && qmp->qmp_fd > -1) close(qmp->qmp_fd);
+
     return ret;
 }
 
-- 
1.7.10.4

Re: [PATCH v3] libxl/libxl_qmp.c: fix error handling in qmp_open

[Wei Liu]

On Thu, Mar 19, 2015 at 12:55:12PM +0000, PRAMOD DEVENDRA wrote:
> From: Pramod Devendra <pramod.devendra@citrix.com>
> 
> 1. Make sure sun_path does not overflow
> 2. Close qmp_fd on error
> 3. Use goto out for error handling
> 
> Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v3] libxl/libxl_qmp.c: fix error handling in qmp_open

[Ian Campbell]

On Fri, 2015-03-20 at 10:08 +0000, Wei Liu wrote:
> On Thu, Mar 19, 2015 at 12:55:12PM +0000, PRAMOD DEVENDRA wrote:
> > From: Pramod Devendra <pramod.devendra@citrix.com>
> > 
> > 1. Make sure sun_path does not overflow
> > 2. Close qmp_fd on error
> > 3. Use goto out for error handling
> > 
> > Signed-off-by: Pramod Devendra <pramod.devendra@citrix.com>
> > CC: Ian Jackson <ian.jackson@eu.citrix.com>
> > CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > CC: Ian Campbell <ian.campbell@citrix.com>
> > CC: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied, thanks.

Pramod, my queue folder contains several patches from you with a
confusing variety of versions ("2/2", "1/3", "v2"), however I think they
are all precursors of this one and so I have moved them to my done
folder. If there was another patch in there then please resend.