Skip to contentSkip to conversation

xenstat: fix unsigned less-than-0 comparison

PATCH In review Matthew Daley 3 participants 4 messages May 4, 2014 – May 8, 2014

lore
Share & tools
lore

Matthew Daley

author
#0 lore raw

PATCH 1/2 xenstat: fix unsigned less-than-0 comparison

Commit 1438d36f ("xenstat: Fix buffer over-run with new_domains being negative.") attempted to fix the handling of a negative error result from xc_domain_getinfolist in xenstat_get_node. However, it forgot to change the result variable from an unsigned type to a signed one.

Do so, allowing the error result to be handled properly.

Signed-off-by Matthew Daley <[email protected]>
---
 tools/xenstat/libxenstat/src/xenstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/xenstat/libxenstat/src/xenstat.c +1 -1
index e5facb8..8072a90 100644
@@ -164,7 +164,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 	xenstat_node *node;
 	xc_physinfo_t physinfo = { 0 };
 	xc_domaininfo_t domaininfo[DOMAIN_CHUNK_SIZE];
-	unsigned int new_domains;
+	int new_domains;
 	unsigned int i;
 
 	/* Create the node */
Signature · 1 line
1.9.2
author
#1 lore raw

PATCH 2/2 xenstat: don't leak memory in getBridge

getBridge's method of returning a result was a little confused:
allocating a result buffer but never using it.

Simplify by instead allowing a result buffer to be passed in and modifying the single usage to match.

Signed-off-by Matthew Daley <[email protected]>
---
 tools/xenstat/libxenstat/src/xenstat_linux.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)
tools/xenstat/libxenstat/src/xenstat_linux.c +7 -9
index 931b24e..24335e1 100644
@@ -64,14 +64,12 @@ static const char PROCNETDEV_HEADER[] =
 
 /* We need to get the name of the bridge interface for use with bonding interfaces */
 /* Use excludeName parameter to avoid adding bridges we don't care about, eg. virbr0 */
-char *getBridge(char *excludeName)
+void getBridge(char *excludeName, char *result, size_t resultLen)
 {
 	struct dirent *de;
 	DIR *d;
 
-	char tmp[256] = { 0 }, *bridge;
-
-	bridge = (char *)malloc(16 * sizeof(char));
+	char tmp[256] = { 0 };
 
 	d = opendir("/sys/class/net");
 	while ((de = readdir(d)) != NULL) {

@@ -79,14 +77,14 @@ char *getBridge(char *excludeName)
 			&& (strstr(de->d_name, excludeName) == NULL)) {
 				sprintf(tmp, "/sys/class/net/%s/bridge", de->d_name);
 
-				if (access(tmp, F_OK) == 0)
-					bridge = de->d_name;
+				if (access(tmp, F_OK) == 0) {
+					strncpy(result, de->d_name, resultLen - 1);
+					result[resultLen - 1] = 0;
+				}
 		}
 	}
 
 	closedir(d);
-
-	return bridge;
 }
 
 /* parseNetLine provides regular expression based parsing for lines from /proc/net/dev, all the */

@@ -279,7 +277,7 @@ int xenstat_collect_networks(xenstat_node * node)
 	      SEEK_SET);
 
 	/* We get the bridge devices for use with bonding interface to get bonding interface stats */
-	snprintf(devBridge, 16, "%s", getBridge("vir"));
+	getBridge("vir", devBridge, sizeof(devBridge));
 	snprintf(devNoBridge, 16, "p%s", devBridge);
 
 	while (fgets(line, 512, priv->procnetdev)) {
Signature · 1 line
1.9.2
Reviewed-by on v1 1/2
#2 lore raw
In reply to Matthew Daley
Quoted 8 lines · Matthew Daley

Commit 1438d36f ("xenstat: Fix buffer over-run with new_domains being negative.") attempted to fix the handling of a negative error result from xc_domain_getinfolist in xenstat_get_node. However, it forgot to change the result variable from an unsigned type to a signed one.

Do so, allowing the error result to be handled properly.

Signed-off-by Matthew Daley <[email protected]>
Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

I was looking at this by changing the underlay libxc calls to using 'unsigned' instead of 'int' but that has kind of been preempted.

Thanks for fixing it!

Quoted 25 lines
---
 tools/xenstat/libxenstat/src/xenstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
tools/xenstat/libxenstat/src/xenstat.c +1 -1
index e5facb8..8072a90 100644
@@ -164,7 +164,7 @@ xenstat_node *xenstat_get_node(xenstat_handle * handle, unsigned int flags)
 	xenstat_node *node;
 	xc_physinfo_t physinfo = { 0 };
 	xc_domaininfo_t domaininfo[DOMAIN_CHUNK_SIZE];
-	unsigned int new_domains;
+	int new_domains;
 	unsigned int i;
 
 	/* Create the node */
Signature · 7 lines
1.9.2


_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel
#3 lore raw
Quoted 11 lines · Konrad Rzeszutek Wilk
Quoted 8 lines · Matthew Daley

Commit 1438d36f ("xenstat: Fix buffer over-run with new_domains being negative.") attempted to fix the handling of a negative error result from xc_domain_getinfolist in xenstat_get_node. However, it forgot to change the result variable from an unsigned type to a signed one.

Do so, allowing the error result to be handled properly.

Signed-off-by Matthew Daley <[email protected]>
Reviewed-by Konrad Rzeszutek Wilk <[email protected]>

Acked both + applied, thanks!