Re: [Xen-devel] [PATCH 2/2] tools/libxl/libxl_qmp.c: Close fds on error path in qmp_open

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Wed, Mar 18, 2015 at 06:50:35AM +0000, PRAMOD DEVENDRA wrote:
> From: Pramod Devendra <pramod.devendra@xxxxxxxxxx>
> 
> Signed-off-by: Pramod Devendra <pramod.devendra@xxxxxxxxxx>
> CC: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> CC: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> CC: Ian Campbell <ian.campbell@xxxxxxxxxx>
> CC: Wei Liu <wei.liu2@xxxxxxxxxx>

Thanks.

The patch content looks good.

However the patch title and commit message are not descriptive enough.

Can you change the title to:

  libxl/libxl_qmp.c: fix qmp_open

And then in commit message:

  This patch does following things:
  
  1. Make sure sun_path does not overflow.
  2. Close qmp_fd on error.

For more general information on patch submission, please refer to

  http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches

Wei.

> ---
>  tools/libxl/libxl_qmp.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index c7324e6..992c1ee 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -365,14 +365,24 @@ static int qmp_open(libxl__qmp_handler *qmp, const char 
> *qmp_socket_path,
>          return -1;
>      }
>      ret = libxl_fd_set_nonblock(qmp->ctx, qmp->qmp_fd, 1);
> -    if (ret) return -1;
> +    if (ret) {
> +        close(qmp->qmp_fd);
> +        return -1;
> +    }
>      ret = libxl_fd_set_cloexec(qmp->ctx, qmp->qmp_fd, 1);
> -    if (ret) return -1;
> +    if (ret) {
> +        close(qmp->qmp_fd);
> +        return -1;
> +    }
>  
> +    if(sizeof (qmp->addr.sun_path) <= strlen(qmp_socket_path)) {
> +        close(qmp->qmp_fd);
> +        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,
> @@ -384,6 +394,7 @@ static int qmp_open(libxl__qmp_handler *qmp, const char 
> *qmp_socket_path,
>               * ECONNREFUSED : Leftover socket hasn't been removed yet */
>              continue;
>          }
> +        close(qmp->qmp_fd);
>          return -1;
>      } while ((++i / 5 <= timeout) && (usleep(200 * 1000) <= 0));
>  
> -- 
> 1.7.10.4

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel