[PATCH 0/4] libxenstat bug fixes and cleanups

[PATCH 0/4] libxenstat bug fixes and cleanups

[Wei Liu]

Wei Liu (4):
  libxenstat: check xc_interface_open return value
  libxenstat: YAJL_GET_STRING may return NULL
  libxenstat: always free qmp_stats
  libxenstat: qmp_read fix and cleanup

 tools/xenstat/libxenstat/src/xenstat_qmp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
1.9.1

[PATCH 1/4] libxenstat: check xc_interface_open return value

[Wei Liu]

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 2cb99e9..f3aeec4 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -366,6 +366,8 @@ static xc_domaininfo_t *get_domain_ids(int *num_doms)
 	if (dominfo == NULL)
 		return NULL;
 	xc_handle = xc_interface_open(0,0,0);
+	if (xc_handle == NULL)
+		return NULL;
 	*num_doms = xc_domain_getinfolist(xc_handle, 0, 1024, dominfo);
 	xc_interface_close(xc_handle);
 	return dominfo;
-- 
1.9.1

Re: [PATCH 1/4] libxenstat: check xc_interface_open return value

[Andrew Cooper]

On 08/04/15 16:01, Wei Liu wrote:
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>

The xenstat handle passed around already has an open xc_handle.  That
should be reused rather than opening a new one every time we want to get
a list of domain ids.

~Andrew

> ---
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index 2cb99e9..f3aeec4 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -366,6 +366,8 @@ static xc_domaininfo_t *get_domain_ids(int *num_doms)
>  	if (dominfo == NULL)
>  		return NULL;
>  	xc_handle = xc_interface_open(0,0,0);
> +	if (xc_handle == NULL)
> +		return NULL;
>  	*num_doms = xc_domain_getinfolist(xc_handle, 0, 1024, dominfo);
>  	xc_interface_close(xc_handle);
>  	return dominfo;

Re: [PATCH 1/4] libxenstat: check xc_interface_open return value

[Wei Liu]

On Wed, Apr 08, 2015 at 04:21:56PM +0100, Andrew Cooper wrote:
> On 08/04/15 16:01, Wei Liu wrote:
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Charles Arnold <carnold@suse.com>
> 
> The xenstat handle passed around already has an open xc_handle.  That
> should be reused rather than opening a new one every time we want to get
> a list of domain ids.
> 

Yes, you're right. I will use that.

Wei.

> ~Andrew
> 
> > ---
> >  tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > index 2cb99e9..f3aeec4 100644
> > --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > @@ -366,6 +366,8 @@ static xc_domaininfo_t *get_domain_ids(int *num_doms)
> >  	if (dominfo == NULL)
> >  		return NULL;
> >  	xc_handle = xc_interface_open(0,0,0);
> > +	if (xc_handle == NULL)
> > +		return NULL;
> >  	*num_doms = xc_domain_getinfolist(xc_handle, 0, 1024, dominfo);
> >  	xc_interface_close(xc_handle);
> >  	return dominfo;

[PATCH 2/4] libxenstat: YAJL_GET_STRING may return NULL

[Wei Liu]

Passing NULL to strcmp can cause segmentation fault. Continue in that
case.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index f3aeec4..6bfbc2f 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -110,7 +110,7 @@ static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
 		ptr[0] = qblock[QMP_BLOCK_DEVICE]; /* "device" */
 		if ((dev_obj = yajl_tree_get(n, ptr, yajl_t_any)) != NULL) {
 			tmp = YAJL_GET_STRING(dev_obj);
-			if (strcmp(qmp_devname, tmp))
+			if (!tmp || strcmp(qmp_devname, tmp))
 				continue;
 		}
 		else
-- 
1.9.1

Re: [PATCH 2/4] libxenstat: YAJL_GET_STRING may return NULL

[Andrew Cooper]

On 08/04/15 16:01, Wei Liu wrote:
> Passing NULL to strcmp can cause segmentation fault. Continue in that
> case.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index f3aeec4..6bfbc2f 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -110,7 +110,7 @@ static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
>  		ptr[0] = qblock[QMP_BLOCK_DEVICE]; /* "device" */
>  		if ((dev_obj = yajl_tree_get(n, ptr, yajl_t_any)) != NULL) {
>  			tmp = YAJL_GET_STRING(dev_obj);
> -			if (strcmp(qmp_devname, tmp))
> +			if (!tmp || strcmp(qmp_devname, tmp))
>  				continue;
>  		}
>  		else

Re: [PATCH 2/4] libxenstat: YAJL_GET_STRING may return NULL

[Ian Jackson]

Wei Liu writes ("[PATCH 2/4] libxenstat: YAJL_GET_STRING may return NULL"):
> Passing NULL to strcmp can cause segmentation fault. Continue in that
> case.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

[PATCH 3/4] libxenstat: always free qmp_stats

[Wei Liu]

Originally qmp_stats is only freed in failure path and leaked in success
path.

Instead of wiring up the success path, rearrange the code a bit to
always free qmp_stats before checking if info is NULL.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index 6bfbc2f..e4f343b 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -95,10 +95,10 @@ static char *qmp_get_block_image(xenstat_node *node, char *qmp_devname, int qfd)
 		return NULL;
 
 	/* Use libyajl version 2.0.3 or newer for the tree parser feature with bug fixes */
-	if ((info = yajl_tree_parse((char *)qmp_stats, NULL, 0)) == NULL) {
-		free(qmp_stats);
+	info = yajl_tree_parse((char *)qmp_stats, NULL, 0);
+	free(qmp_stats);
+	if (info == NULL)
 		return NULL;
-	}
 
 	ptr[0] = qblock[QMP_BLOCK_RETURN]; /* "return" */
 	if ((ret_obj = yajl_tree_get(info, ptr, yajl_t_array)) == NULL)
-- 
1.9.1

Re: [PATCH 3/4] libxenstat: always free qmp_stats

[Ian Jackson]

Wei Liu writes ("[PATCH 3/4] libxenstat: always free qmp_stats"):
> Originally qmp_stats is only freed in failure path and leaked in success
> path.
> 
> Instead of wiring up the success path, rearrange the code a bit to
> always free qmp_stats before checking if info is NULL.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Having said that, I am not a great fan of the error handling cleanup
style adopted in this code in general.  The ownership and lifetime of
the various allocated objects has to be inferred by reading the code
in detail.

Ian.

Re: [PATCH 3/4] libxenstat: always free qmp_stats

[Wei Liu]

On Wed, Apr 08, 2015 at 04:49:58PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH 3/4] libxenstat: always free qmp_stats"):
> > Originally qmp_stats is only freed in failure path and leaked in success
> > path.
> > 
> > Instead of wiring up the success path, rearrange the code a bit to
> > always free qmp_stats before checking if info is NULL.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> Having said that, I am not a great fan of the error handling cleanup
> style adopted in this code in general.  The ownership and lifetime of
> the various allocated objects has to be inferred by reading the code
> in detail.
> 

Yeah... I'm with you. On the other hand I want my bug fix patch to be as
small as possible. Changing this function to goto style will make the
patch a lot bigger. I think we can clean this function up with another
patch if anyone feels keen enough.

Wei.

> Ian.

[PATCH 4/4] libxenstat: qmp_read fix and cleanup

[Wei Liu]

The second argument of poll(2) is the number of file descriptors. POLLIN
is defined as 1 so it happens to work.

Also do two cleanups while I was there:
1. There is only one fd, so a one-element array is enough.
2. Initialise pfd to make code linter happy.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Charles Arnold <carnold@suse.com>
---
 tools/xenstat/libxenstat/src/xenstat_qmp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
index e4f343b..260cd34 100644
--- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
+++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
@@ -289,13 +289,14 @@ static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
 static int qmp_read(int qfd, unsigned char **qstats)
 {
 	unsigned char buf[1024], *ptr;
-	struct pollfd pfd[2];
+	struct pollfd pfd[1];
 	int n, qsize = 0;
 
 	*qstats = NULL;
+	memset(pfd, 0, sizeof(pfd));
 	pfd[0].fd = qfd;
 	pfd[0].events = POLLIN;
-	while ((n = poll(pfd, POLLIN, 10)) > 0) {
+	while ((n = poll(pfd, 1, 10)) > 0) {
 		if (pfd[0].revents & POLLIN) {
 			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
 				free(*qstats);
@@ -311,6 +312,9 @@ static int qmp_read(int qfd, unsigned char **qstats)
 			ptr[qsize] = 0;
 			*qstats = ptr;
 		}
+		memset(pfd, 0, sizeof(pfd));
+		pfd[0].fd = qfd;
+		pfd[0].events = POLLIN;
 	}
 	return 1;
 }
-- 
1.9.1

Re: [PATCH 4/4] libxenstat: qmp_read fix and cleanup

[Andrew Cooper]

On 08/04/15 16:01, Wei Liu wrote:
> The second argument of poll(2) is the number of file descriptors. POLLIN
> is defined as 1 so it happens to work.
>
> Also do two cleanups while I was there:
> 1. There is only one fd, so a one-element array is enough.
> 2. Initialise pfd to make code linter happy.
>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Charles Arnold <carnold@suse.com>
> ---
>  tools/xenstat/libxenstat/src/xenstat_qmp.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> index e4f343b..260cd34 100644
> --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> @@ -289,13 +289,14 @@ static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
>  static int qmp_read(int qfd, unsigned char **qstats)
>  {
>  	unsigned char buf[1024], *ptr;
> -	struct pollfd pfd[2];
> +	struct pollfd pfd[1];
>  	int n, qsize = 0;
>  
>  	*qstats = NULL;
> +	memset(pfd, 0, sizeof(pfd));
>  	pfd[0].fd = qfd;
>  	pfd[0].events = POLLIN;
> -	while ((n = poll(pfd, POLLIN, 10)) > 0) {
> +	while ((n = poll(pfd, 1, 10)) > 0) {
>  		if (pfd[0].revents & POLLIN) {
>  			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
>  				free(*qstats);
> @@ -311,6 +312,9 @@ static int qmp_read(int qfd, unsigned char **qstats)
>  			ptr[qsize] = 0;
>  			*qstats = ptr;
>  		}
> +		memset(pfd, 0, sizeof(pfd));
> +		pfd[0].fd = qfd;
> +		pfd[0].events = POLLIN;

I don't think this is necessary.  .fd and .events should be untouched by
poll().

Ignore the complaint about revents being uninitialised.  That is because
Coverity doesn't have a model for poll(), and doesn't know that poll()
writes to revents.

I was going to find some copious free time to correctly model poll(),
which will fix several related defects.

~Andrew

>  	}
>  	return 1;
>  }

Re: [PATCH 4/4] libxenstat: qmp_read fix and cleanup

[Wei Liu]

On Wed, Apr 08, 2015 at 04:25:06PM +0100, Andrew Cooper wrote:
> On 08/04/15 16:01, Wei Liu wrote:
> > The second argument of poll(2) is the number of file descriptors. POLLIN
> > is defined as 1 so it happens to work.
> >
> > Also do two cleanups while I was there:
> > 1. There is only one fd, so a one-element array is enough.
> > 2. Initialise pfd to make code linter happy.
> >
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Charles Arnold <carnold@suse.com>
> > ---
> >  tools/xenstat/libxenstat/src/xenstat_qmp.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/xenstat/libxenstat/src/xenstat_qmp.c b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > index e4f343b..260cd34 100644
> > --- a/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > +++ b/tools/xenstat/libxenstat/src/xenstat_qmp.c
> > @@ -289,13 +289,14 @@ static size_t qmp_write(int qfd, char *cmd, size_t cmd_len)
> >  static int qmp_read(int qfd, unsigned char **qstats)
> >  {
> >  	unsigned char buf[1024], *ptr;
> > -	struct pollfd pfd[2];
> > +	struct pollfd pfd[1];
> >  	int n, qsize = 0;
> >  
> >  	*qstats = NULL;
> > +	memset(pfd, 0, sizeof(pfd));
> >  	pfd[0].fd = qfd;
> >  	pfd[0].events = POLLIN;
> > -	while ((n = poll(pfd, POLLIN, 10)) > 0) {
> > +	while ((n = poll(pfd, 1, 10)) > 0) {
> >  		if (pfd[0].revents & POLLIN) {
> >  			if ((n = read(qfd, buf, sizeof(buf))) < 0) {
> >  				free(*qstats);
> > @@ -311,6 +312,9 @@ static int qmp_read(int qfd, unsigned char **qstats)
> >  			ptr[qsize] = 0;
> >  			*qstats = ptr;
> >  		}
> > +		memset(pfd, 0, sizeof(pfd));
> > +		pfd[0].fd = qfd;
> > +		pfd[0].events = POLLIN;
> 
> I don't think this is necessary.  .fd and .events should be untouched by
> poll().
> 
> Ignore the complaint about revents being uninitialised.  That is because
> Coverity doesn't have a model for poll(), and doesn't know that poll()
> writes to revents.
> 

Sure.

> I was going to find some copious free time to correctly model poll(),
> which will fix several related defects.
> 

That would be good!

Wei.

> ~Andrew
> 
> >  	}
> >  	return 1;
> >  }

Re: [PATCH 4/4] libxenstat: qmp_read fix and cleanup

[Ian Jackson]

Andrew Cooper writes ("Re: [PATCH 4/4] libxenstat: qmp_read fix and cleanup"):
> On 08/04/15 16:01, Wei Liu wrote:
> > The second argument of poll(2) is the number of file descriptors. POLLIN
> > is defined as 1 so it happens to work.
...
> > @@ -311,6 +312,9 @@ static int qmp_read(int qfd, unsigned char **qstats)
> >  			ptr[qsize] = 0;
> >  			*qstats = ptr;
> >  		}
> > +		memset(pfd, 0, sizeof(pfd));
> > +		pfd[0].fd = qfd;
> > +		pfd[0].events = POLLIN;
> 
> I don't think this is necessary.  .fd and .events should be untouched by
> poll().

Indeed.

> Ignore the complaint about revents being uninitialised.  That is because
> Coverity doesn't have a model for poll(), and doesn't know that poll()
> writes to revents.

I would be happy with the patch either with or without the memset.

Ian.