[Cluster-devel] [PATCH 0/2] rgmanager: spots in flattening-related parts

[Cluster-devel] [PATCH 0/2] rgmanager: spots in flattening-related parts

[Jan Pokorný]

Just two spots I discovered while running through the code for flattening
resource trees in cluster.conf.  One cosmetic, one little more subtle.

Jan Pokorn? (2):
   resrules: fix free while passing the pointer to caller
   resrules: print_resource_rule doc comment conformance

  resrules.c |    9 +++------
  1 files changed, 3 insertions(+), 6 deletions(-)

[Cluster-devel] [PATCH 1/2] resrules: fix free while passing the pointer to caller

[Jan Pokorný]

The version ("OCF API Version" as declared in the code) for resource
rules cannot be obtained correctly as the memory is being immediately
freed before passing up to the caller.  What's worse, the caller
could then access uninitialized memory through this pointer
(print_resource_rule and especially destroy_resource_rule which could
lead to crash easily, IMHO).

The patch fixes this, making no difference between success
and failure in getting the version.  Both should be handled
correctly when either dumping resource rule or destroying it.

Aside: was this version field ever actively used or is this a legacy
        part not expected to be triggered?
[ I have no test case at hand, this was random spot, sorry. ]

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

diff --git a/resrules.c b/resrules.c
index cc59e30..581be9e 100644
--- a/resrules.c
+++ b/resrules.c
@@ -205,11 +205,8 @@ _get_version(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
  
  	snprintf(xpath, sizeof(xpath), "%s/@version", base);
  	ret = xpath_get_one(doc, ctx, xpath);
-	if (ret) {
-		rr->rr_version = ret;
-		free(ret);
-	}
-	rr->rr_version = NULL;
+	/* NULL or actual result of the query */
+	rr->rr_version = ret;
  }
  
  

[Cluster-devel] [PATCH 2/2] resrules: print_resource_rule doc comment conformance

[Jan Pokorný]

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

diff --git a/resrules.c b/resrules.c
index 581be9e..4fcc4f1 100644
--- a/resrules.c
+++ b/resrules.c
@@ -556,7 +556,7 @@ store_childtype(resource_child_t **childp, char *name, int start,
  
  
  /**
-   Print a resource_t structure to stdout
+   Print a resource_t structure to specified stream
  
     @param rr		Resource rule to print.
   */

[Cluster-devel] [PATCH 2b/2] resrules: print_resource_rule doc comment conformance

[Jan Pokorný]

(when changing doc. comment, do it properly)

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

diff --git a/resrules.c b/resrules.c
index 581be9e..353ead4 100644
--- a/resrules.c
+++ b/resrules.c
@@ -556,9 +556,10 @@ store_childtype(resource_child_t **childp, char *name, int start,
  
  
  /**
-   Print a resource_t structure to stdout
+   Print a resource_t structure to specified stream
  
     @param rr		Resource rule to print.
+   @param fp		Destination stream.
   */
  void
  print_resource_rule(FILE *fp, resource_rule_t *rr)

[Cluster-devel] [PATCH 1b/2] resrules: fix free while passing the pointer to caller

[Jan Pokorný]

(due to previously misconfigured MUA, sorry for inconvenience)

The version ("OCF API Version" as declared in the code) for resource
rules cannot be obtained correctly as the memory is being immediately
freed before passing up to the caller.  What's worse, the caller
could then access uninitialized memory through this pointer
(e.g., print_resource_rule).

The patch fixes this, making no difference between success
and failure in getting the version.  Both should be handled
correctly when either dumping resource rule or destroying it.

Aside: was this version field ever actively used of is this a legacy
       part not expected to be triggered?
[ I have no test case at hand, this was random spot, sorry. ]

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

diff --git a/resrules.c b/resrules.c
index cc59e30..581be9e 100644
--- a/resrules.c
+++ b/resrules.c
@@ -205,11 +205,8 @@ _get_version(xmlDocPtr doc, xmlXPathContextPtr ctx, char *base,
 
 	snprintf(xpath, sizeof(xpath), "%s/@version", base);
 	ret = xpath_get_one(doc, ctx, xpath);
-	if (ret) {
-		rr->rr_version = ret;
-		free(ret);
-	}
-	rr->rr_version = NULL;
+	/* NULL or actual result of the query */
+	rr->rr_version = ret;
 }
 
 

[Cluster-devel] [PATCH 2c/2] resrules: print_resource_rule doc comment conformance

[Jan Pokorný]

(when changing doc. comment, do it properly)
(due to previously misconfigured MUA, sorry for inconvenience)

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

diff --git a/resrules.c b/resrules.c
index 581be9e..353ead4 100644
--- a/resrules.c
+++ b/resrules.c
@@ -556,9 +556,10 @@ store_childtype(resource_child_t **childp, char *name, int start,
 
 
 /**
-   Print a resource_t structure to stdout
+   Print a resource_t structure to specified stream
 
    @param rr		Resource rule to print.
+   @param fp		Destination stream.
  */
 void
 print_resource_rule(FILE *fp, resource_rule_t *rr)

[Cluster-devel] [PATCH 1b/2] resrules: fix free while passing the pointer to caller

[Jan Pokorný]

On 02/04/12 13:29 +0200, Jan Pokorn? wrote:
> What's worse, the caller could then access uninitialized memory
> through this pointer (e.g., print_resource_rule).

As Ryan noted, this is not true, as the pointer was always set to
NULL.  So the patch only eliminates hidden NOOP.

-- Jan