[Cluster-devel] [PATCH cluster 0/5] NULL-deref after failed malloc (STABLE3)

[Cluster-devel] [PATCH cluster 0/5] NULL-deref after failed malloc (STABLE3)

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

I audited cluster's STABLE3 branch for NULL-deref after
heap allocation failures.  I took the time to fix a few
and left FIXME comments marking the others.

[Cluster-devel] [PATCH cluster 1/5] src/clulib/ckpt_state.c (ds_key_init_nt): detect failed malloc

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

---
 rgmanager/src/clulib/ckpt_state.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/rgmanager/src/clulib/ckpt_state.c b/rgmanager/src/clulib/ckpt_state.c
index fd85aee..766097b 100644
--- a/rgmanager/src/clulib/ckpt_state.c
+++ b/rgmanager/src/clulib/ckpt_state.c
@@ -70,6 +70,7 @@ ds_key_init_nt(char *keyid, int maxsize, int timeout)
 	}

 	newnode = malloc(sizeof(*newnode));
+	// FIXME: detect failed malloc
 	memset(newnode,0,sizeof(*newnode));
 	snprintf((char *)newnode->kn_cpname.value, SA_MAX_NAME_LENGTH-1,
 		 "%s", keyid);
-- 
1.6.3.3.420.gd4b46

[Cluster-devel] [PATCH cluster 2/5] dlm_controld: handle heap allocation failure and plug leaks

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* group/dlm_controld/pacemaker.c (process_cluster): Don't dereference
NULL upon failing malloc or realloc.  Free "header" upon failure.
---
 group/dlm_controld/pacemaker.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
index fed9ca7..b9b38d0 100644
--- a/group/dlm_controld/pacemaker.c
+++ b/group/dlm_controld/pacemaker.c
@@ -135,10 +135,12 @@ void process_cluster(int ci)

     AIS_Message *msg = NULL;
     SaAisErrorT rc = SA_AIS_OK;
-    mar_res_header_t *header = NULL;
+    mar_res_header_t *header;
+    mar_res_header_t *h;
     static int header_len = sizeof(mar_res_header_t);

-    header = malloc(header_len);
+    if ((header = malloc(header_len)) == NULL)
+	goto bail;
     memset(header, 0, header_len);
     
     errno = 0;
@@ -160,8 +162,12 @@ void process_cluster(int ci)
     } else if(header->error != 0) {
 	log_error("Header contined error: %d", header->error);
     }
-    
-    header = realloc(header, header->size);
+
+    h_new = realloc(header, header->size);
+    if (h_new == NULL)
+	goto bail;
+    header = h_new;
+
     /* Use a char* so we can store the remainder into an offset */
     data = (char*)header;

@@ -250,6 +256,7 @@ void process_cluster(int ci)
     goto done;
     
   bail:
+    free (header);
     log_error("AIS connection failed");
     return;
 }
@@ -406,4 +413,3 @@ int fence_in_progress(int *count)
 {
 	return 0;
 }
-
-- 
1.6.3.3.420.gd4b46

[Cluster-devel] Re: [PATCH cluster 2/5] dlm_controld: handle heap allocation failure and plug leaks

[Jim Meyering]

Jim Meyering wrote:

> From: Jim Meyering <meyering@redhat.com>
>
> * group/dlm_controld/pacemaker.c (process_cluster): Don't dereference
> NULL upon failing malloc or realloc.  Free "header" upon failure.
> ---
>  group/dlm_controld/pacemaker.c |   16 +++++++++++-----
>  1 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
> index fed9ca7..b9b38d0 100644
> --- a/group/dlm_controld/pacemaker.c
> +++ b/group/dlm_controld/pacemaker.c
> @@ -135,10 +135,12 @@ void process_cluster(int ci)
>
>      AIS_Message *msg = NULL;
>      SaAisErrorT rc = SA_AIS_OK;
> -    mar_res_header_t *header = NULL;
> +    mar_res_header_t *header;
> +    mar_res_header_t *h;

Oops.  That "h" have been "h_new".
(Andrew Beekhof noticed this)
...
> +    h_new = realloc(header, header->size);

Here's the corrected patch:

From bbc3ffcaf72d8089a1783af8ec9dc5b726a2b68e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@redhat.com>
Date: Mon, 22 Jun 2009 23:37:21 +0200
Subject: [PATCH cluster] dlm_controld: handle heap allocation failure and plug leaks

* group/dlm_controld/pacemaker.c (process_cluster): Don't dereference
NULL upon failing malloc or realloc.  Free "header" upon failure.
---
 group/dlm_controld/pacemaker.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
index 509696a..9b0a2d8 100644
--- a/group/dlm_controld/pacemaker.c
+++ b/group/dlm_controld/pacemaker.c
@@ -135,10 +135,12 @@ void process_cluster(int ci)

     AIS_Message *msg = NULL;
     SaAisErrorT rc = SA_AIS_OK;
-    mar_res_header_t *header = NULL;
+    mar_res_header_t *header;
+    mar_res_header_t *h_new;
     static int header_len = sizeof(mar_res_header_t);

-    header = malloc(header_len);
+    if ((header = malloc(header_len)) == NULL)
+	goto bail;
     memset(header, 0, header_len);
     
     errno = 0;
@@ -160,8 +162,12 @@ void process_cluster(int ci)
     } else if(header->error != 0) {
 	log_error("Header contined error: %d", header->error);
     }
-    
-    header = realloc(header, header->size);
+
+    h_new = realloc(header, header->size);
+    if (h_new == NULL)
+	goto bail;
+    header = h_new;
+
     /* Use a char* so we can store the remainder into an offset */
     data = (char*)header;

@@ -252,6 +258,7 @@ void process_cluster(int ci)
     goto done;
     
   bail:
+    free (header);
     log_error("AIS connection failed");
     return;
 }
@@ -408,4 +415,3 @@ int fence_in_progress(int *count)
 {
 	return 0;
 }
-
-- 
1.6.3.3.420.gd4b46

[Cluster-devel] [PATCH cluster 3/5] dlm_controld: add comments: mark memory problems

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* group/dlm_controld/pacemaker.c (process_cluster): Mark additional
memory problems.
---
 group/dlm_controld/pacemaker.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/group/dlm_controld/pacemaker.c b/group/dlm_controld/pacemaker.c
index b9b38d0..b06e73d 100644
--- a/group/dlm_controld/pacemaker.c
+++ b/group/dlm_controld/pacemaker.c
@@ -191,6 +191,7 @@ void process_cluster(int ci)

 	log_debug("Decompressing message data");
 	uncompressed = malloc(new_size);
+	// FIXME: handle malloc failure
 	memset(uncompressed, 0, new_size);
 	
 	rc = BZ2_bzBuffToBuffDecompress(
@@ -212,6 +213,7 @@ void process_cluster(int ci)
     } else if(safe_str_eq("identify", data)) {
 	int pid = getpid();
 	char *pid_s = crm_itoa(pid);
+	// FIXME: can crm_itoa fail?  if so, does it return NULL?
 	send_ais_text(0, pid_s, TRUE, NULL, crm_msg_ais);
 	crm_free(pid_s);
 	goto done;
-- 
1.6.3.3.420.gd4b46

[Cluster-devel] [PATCH cluster 4/5] dlm/tests: handle malloc failure

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* dlm/tests/usertest/flood.c (main): Handle malloc failure.
---
 dlm/tests/usertest/flood.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/dlm/tests/usertest/flood.c b/dlm/tests/usertest/flood.c
index efe3a4b..c01bc0d 100644
--- a/dlm/tests/usertest/flood.c
+++ b/dlm/tests/usertest/flood.c
@@ -112,7 +112,11 @@ int main(int argc, char *argv[])
 	}
     }

-    resources = malloc(sizeof(char*) * rescount);
+    if ((resources = malloc(sizeof(char*) * rescount)) == NULL)
+    {
+	    perror("exhausted virtual memory");
+	    return 1;
+    }
     for (i=0; i < rescount; i++) {
 	    char resname[256];
 	    sprintf(resname, "TESTLOCK%d", i);
@@ -164,4 +168,3 @@ int main(int argc, char *argv[])

     return 0;
 }
-
-- 
1.6.3.3.420.gd4b46

[Cluster-devel] [PATCH cluster 5/5] cman: handle malloc failure (i.e., don't deref NULL)

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

* cman/daemon/commands.c (do_cmd_get_extrainfo):
Handle malloc failure (i.e., don't deref NULL).
(do_cmd_get_all_members): Likewise.
---
 cman/daemon/commands.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/cman/daemon/commands.c b/cman/daemon/commands.c
index b343bcb..c360909 100644
--- a/cman/daemon/commands.c
+++ b/cman/daemon/commands.c
@@ -538,6 +538,8 @@ static int do_cmd_get_extrainfo(char *cmdbuf, char **retbuf, int retsize, int *r
 		       sizeof(struct sockaddr_storage) * (MAX_INTERFACES*2))) {

 		*retbuf = malloc(sizeof(struct cl_extra_info) + sizeof(struct sockaddr_storage) * (MAX_INTERFACES*2));
+		if (*retbuf == NULL)
+			return -ENOMEM;
 		outbuf = *retbuf + offset;
 		einfo = (struct cl_extra_info *)outbuf;

@@ -635,6 +637,8 @@ static int do_cmd_get_all_members(char *cmdbuf, char **retbuf, int retsize, int
 		/* If there is not enough space in the default buffer, allocate some more. */
 		if ((retsize / sizeof(struct cl_cluster_node)) < total_nodes) {
 			*retbuf = malloc(sizeof(struct cl_cluster_node) * total_nodes + offset);
+			if (!*retbuf)
+				return -ENOMEM;
 			outbuf = *retbuf + offset;
 			log_printf(LOGSYS_LEVEL_DEBUG, "memb: get_all_members: allocated new buffer (retsize=%d)\n", retsize);
 		}
-- 
1.6.3.3.420.gd4b46