[Cluster-devel] [PATCH] rgmanager: resrules: simplify reading attributes + avoid memleaks

[Cluster-devel] [PATCH] rgmanager: resrules: simplify reading attributes + avoid memleaks

[Jan Pokorný]

These memleaks (connected with attrname) could occur in some
recoverable error corner cases.

Getting of default value is postponed until it is actually needed
(i.e., the value is not inherited which should have the priority,
at least as per in-code comment).

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 resrules.c |   36 ++++++++++++++----------------------
 1 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/resrules.c b/resrules.c
index 353ead4..9d533f1 100644
--- a/resrules.c
+++ b/resrules.c
@@ -711,28 +711,20 @@ static int
 _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 		resource_rule_t *rr)
 {
-	char *ret, *attrname, *dflt = NULL, xpath[256];
+	char *ret, *attrname, xpath[256];
 	int x, flags, primary_found = 0;
 
 	for (x = 1; 1; x++) {
 		snprintf(xpath, sizeof(xpath), "%s/parameter[%d]/@name",
  			 base, x);
 
-		ret = xpath_get_one(doc,ctx,xpath);
-		if (!ret)
+		attrname = xpath_get_one(doc,ctx,xpath);
+		if (!attrname)
 			break;
 
 		flags = 0;
-		attrname = ret;
 		
 		/*
-		   See if there's a default value.
-		 */
-		snprintf(xpath, sizeof(xpath),
-			 "%s/parameter[%d]/content/@default", base, x);
-		dflt = xpath_get_one(doc,ctx,xpath);
-
-		/*
 		   See if this is either the primary identifier or
 		   a required field.
 		 */
@@ -761,6 +753,7 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 			if ((atoi(ret) != 0) || (ret[0] == 'y')) {
 				if (primary_found) {
 					free(ret);
+					free(attrname);
 					printf("Multiple primary "
 					       "definitions for "
 					       "resource type %s\n",
@@ -786,7 +779,8 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 		}
 
 		/*
-		   See if this is supposed to be inherited
+		   See if this is supposed to be inherited;
+		   inheritance supercedes a specified default value
 		 */
 		snprintf(xpath, sizeof(xpath), "%s/parameter[%d]/@inherit",
 			 base, x);
@@ -795,31 +789,29 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 
 			if (flags & (RA_REQUIRED | RA_PRIMARY | RA_UNIQUE)) {
 				free(ret);
+				free(attrname);
 				printf("Can not inherit and be primary, "
 				       "unique, or required\n");
 				return -1;
 			}
-			/*
-			   don't free ret.  Store as attr value.  If we had
-			   a default value specified from above, free it;
-			   inheritance supercedes a specified default value.
-			 */
-			if (dflt)
-				free(dflt);
+
+			/* Don't free ret */
+
 		} else {
 			/*
 			   Use default value, if specified, as the attribute
 			   value.
 			 */
-			ret = dflt;
+			snprintf(xpath, sizeof(xpath),
+				 "%s/parameter[%d]/content/@default", base, x);
+			ret = xpath_get_one(doc,ctx,xpath);
 		}
 
 		/*
 		   Store the attribute.  We'll ensure all required
 		   attributes are present soon.
 		 */
-		if (attrname)
-			store_attribute(&rr->rr_attrs, attrname, ret, flags);
+		store_attribute(&rr->rr_attrs, attrname, ret, flags);
 	}
 
 	return 0;

[Cluster-devel] [PATCH] rgmanager: resrules: simplify getting attributes + avoid memleaks

[Jan Pokorný]

These memleaks (connected with attrname, dflt and ret) could occur
in some recoverable error corner cases.

Getting of default value is postponed until it is actually needed
(i.e., the value is not inherited which should have the priority,
at least as per in-code comment).

update v2:
Now, also avoid memleak upon store_attribute failure.

Signed-off-by: Jan Pokorn? <jpokorny@redhat.com>
---
 resrules.c |   39 +++++++++++++++++----------------------
 1 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/resrules.c b/resrules.c
index 353ead4..557ef9c 100644
--- a/resrules.c
+++ b/resrules.c
@@ -711,28 +711,20 @@ static int
 _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 		resource_rule_t *rr)
 {
-	char *ret, *attrname, *dflt = NULL, xpath[256];
+	char *ret, *attrname, xpath[256];
 	int x, flags, primary_found = 0;
 
 	for (x = 1; 1; x++) {
 		snprintf(xpath, sizeof(xpath), "%s/parameter[%d]/@name",
  			 base, x);
 
-		ret = xpath_get_one(doc,ctx,xpath);
-		if (!ret)
+		attrname = xpath_get_one(doc,ctx,xpath);
+		if (!attrname)
 			break;
 
 		flags = 0;
-		attrname = ret;
 		
 		/*
-		   See if there's a default value.
-		 */
-		snprintf(xpath, sizeof(xpath),
-			 "%s/parameter[%d]/content/@default", base, x);
-		dflt = xpath_get_one(doc,ctx,xpath);
-
-		/*
 		   See if this is either the primary identifier or
 		   a required field.
 		 */
@@ -761,6 +753,7 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 			if ((atoi(ret) != 0) || (ret[0] == 'y')) {
 				if (primary_found) {
 					free(ret);
+					free(attrname);
 					printf("Multiple primary "
 					       "definitions for "
 					       "resource type %s\n",
@@ -786,7 +779,8 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 		}
 
 		/*
-		   See if this is supposed to be inherited
+		   See if this is supposed to be inherited;
+		   inheritance supercedes a specified default value
 		 */
 		snprintf(xpath, sizeof(xpath), "%s/parameter[%d]/@inherit",
 			 base, x);
@@ -795,31 +789,32 @@ _get_rule_attrs(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 
 			if (flags & (RA_REQUIRED | RA_PRIMARY | RA_UNIQUE)) {
 				free(ret);
+				free(attrname);
 				printf("Can not inherit and be primary, "
 				       "unique, or required\n");
 				return -1;
 			}
-			/*
-			   don't free ret.  Store as attr value.  If we had
-			   a default value specified from above, free it;
-			   inheritance supercedes a specified default value.
-			 */
-			if (dflt)
-				free(dflt);
+
+			/* Don't free ret */
+
 		} else {
 			/*
 			   Use default value, if specified, as the attribute
 			   value.
 			 */
-			ret = dflt;
+			snprintf(xpath, sizeof(xpath),
+				 "%s/parameter[%d]/content/@default", base, x);
+			ret = xpath_get_one(doc,ctx,xpath);
 		}
 
 		/*
 		   Store the attribute.  We'll ensure all required
 		   attributes are present soon.
 		 */
-		if (attrname)
-			store_attribute(&rr->rr_attrs, attrname, ret, flags);
+		if (store_attribute(&rr->rr_attrs, attrname, ret, flags) != 0) {
+			free(attrname);
+			free(ret);
+		}
 	}
 
 	return 0;