[PATCH 00/13] Add missing handling of malloc() failure

[PATCH 00/13] Add missing handling of malloc() failure

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Hi,

I cam across a couple of these last week while I was chasing the
assemble problem of IMSM raids, so I started going through the code to
fix up some more of these.

This is just low hanging fruit, and there is more to be done, but at
least it's a few fixes.

Cheers,
Jes


Jes Sorensen (13):
  count_active() catch malloc() failure
  Catch malloc() failures
  Try to catch malloc() failures in Assemble.c
  Attempt to catch malloc() failure in Detail.c
  Handle malloc() failure in Examine.c
  Handle malloc() errors in Manage.c
  Fix malloc() failure handling in Monitor.c
  Handle malloc() failures in devline()
  Fix malloc handling in dlink.c
  Handle malloc() failure in Grow.c
  Handle malloc() failure in mdopen.c
  Handle malloc() failure in mdstat.c
  Catch malloc() failure

 Assemble.c    |   37 ++++++++++++++++++++-
 Detail.c      |   22 +++++++++++++
 Examine.c     |    5 +++
 Grow.c        |    7 ++++
 Incremental.c |    4 ++
 Manage.c      |   13 ++++++-
 Monitor.c     |    7 ++++
 config.c      |   99 ++++++++++++++++++++++++++++++++++++++++++---------------
 dlink.c       |    6 ++-
 mdopen.c      |    6 +++
 mdstat.c      |    5 +++
 policy.c      |    5 +++
 util.c        |   19 +++++++++--
 13 files changed, 200 insertions(+), 35 deletions(-)

-- 
1.7.6.4

[PATCH 01/13] count_active() catch malloc() failure

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Incremental.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/Incremental.c b/Incremental.c
index c21c971..35f0ce7 100644
--- a/Incremental.c
+++ b/Incremental.c
@@ -756,6 +756,10 @@ static int count_active(struct supertype *st, struct mdinfo *sra,
 
 			best = calloc(raid_disks, sizeof(int));
 			devmap = calloc(raid_disks * numdevs, 1);
+			if (!best || !devmap) {
+				fprintf(stderr, Name ": out of memory.\n");
+				exit(1);
+			}
 
 			st->ss->getinfo_super(st, &info, devmap);
 		}
-- 
1.7.6.4

[PATCH 02/13] Catch malloc() failures

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 util.c |   19 ++++++++++++++++---
 1 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/util.c b/util.c
index f785f03..5bb71b2 100644
--- a/util.c
+++ b/util.c
@@ -370,6 +370,11 @@ int enough_fd(int fd)
 	    array.raid_disks <= 0)
 		return 0;
 	avail = calloc(array.raid_disks, 1);
+	if (!avail) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		exit(1);
+	}
 	for (i=0; i < 1024 && array.nr_disks > 0; i++) {
 		disk.number = i;
 		if (ioctl(fd, GET_DISK_INFO, &disk) != 0)
@@ -1035,8 +1040,10 @@ struct supertype *guess_super_type(int fd, enum guess_types guess_type)
 	int bestsuper = -1;
 	int i;
 
-	st = malloc(sizeof(*st));
-	memset(st, 0, sizeof(*st));
+	st = calloc(sizeof(*st), 1);
+	if (!st)
+		return st;
+
 	st->container_dev = NoMdDev;
 
 	for (i=0 ; superlist[i]; i++) {
@@ -1479,6 +1486,8 @@ int add_disk(int mdfd, struct supertype *st,
 					break;
 			if (sd2 == NULL) {
 				sd2 = malloc(sizeof(*sd2));
+				if (!sd2)
+					return -ENOMEM;
 				*sd2 = *info;
 				sd2->next = sra->devs;
 				sra->devs = sd2;
@@ -1688,7 +1697,11 @@ void append_metadata_update(struct supertype *st, void *buf, int len)
 {
 
 	struct metadata_update *mu = malloc(sizeof(*mu));
-
+	if (!mu) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		exit(1);
+	}
 	mu->buf = buf;
 	mu->len = len;
 	mu->space = NULL;
-- 
1.7.6.4

[PATCH 03/13] Try to catch malloc() failures in Assemble.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Assemble.c |   37 +++++++++++++++++++++++++++++++++++--
 1 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index 2000dd0..6337681 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -706,7 +706,17 @@ int Assemble(struct supertype *st, char *mddev,
 	bitmap_done = 0;
 	content->update_private = NULL;
 	devices = malloc(num_devs * sizeof(*devices));
+	if (!devices) {
+		close(mdfd);
+		return -ENOMEM;
+	}
 	devmap = calloc(num_devs * content->array.raid_disks, 1);
+	if (!devmap) {
+		close(mdfd);
+		free(devices);
+		return -ENOMEM;
+	}
+
 	for (tmpdev = devlist; tmpdev; tmpdev=tmpdev->next) if (tmpdev->used == 1) {
 		char *devname = tmpdev->devname;
 		struct stat stb;
@@ -847,6 +857,16 @@ int Assemble(struct supertype *st, char *mddev,
 				int newbestcnt = i+10;
 				int *newbest = malloc(sizeof(int)*newbestcnt);
 				int c;
+				if (!newbest) {
+					if (best)
+						free(best);
+					close(mdfd);
+					free(devices);
+					free(devmap);
+					fprintf(stderr, Name ": %s unable to "
+						"allocate memory\n", __func__);
+					return -ENOMEM;
+				}
 				for (c=0; c < newbestcnt; c++)
 					if (c < bestcnt)
 						newbest[c] = best[c];
@@ -912,8 +932,14 @@ int Assemble(struct supertype *st, char *mddev,
 	/* now we have some devices that might be suitable.
 	 * I wonder how many
 	 */
-	avail = malloc(content->array.raid_disks);
-	memset(avail, 0, content->array.raid_disks);
+	avail = calloc(content->array.raid_disks, 1);
+	if (!avail) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		close(mdfd);
+		free(devices);
+		free(devmap);
+	};
 	okcnt = 0;
 	sparecnt=0;
 	rebuilding_cnt=0;
@@ -1172,6 +1198,13 @@ int Assemble(struct supertype *st, char *mddev,
 	if (content->reshape_active) {
 		int err = 0;
 		int *fdlist = malloc(sizeof(int)* bestcnt);
+		if (!fdlist) {
+			fprintf(stderr, Name ": %s unable to allocate memory\n",
+				__func__);
+			close(mdfd);
+			free(devices);
+			return -ENOMEM;
+		}
 		if (verbose > 0)
 			fprintf(stderr, Name ":%s has an active reshape - checking "
 				"if critical section needs to be restored\n",
-- 
1.7.6.4

[PATCH 04/13] Attempt to catch malloc() failure in Detail.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Detail.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/Detail.c b/Detail.c
index ca34abe..4130821 100644
--- a/Detail.c
+++ b/Detail.c
@@ -147,6 +147,11 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 			info = st->ss->container_content(st, subarray);
 		else {
 			info = malloc(sizeof(*info));
+			if (!info) {
+				fprintf(stderr, Name ": %s unable to allocate "
+					"memory\n", __func__);
+				return -ENOMEM;
+			}
 			st->ss->getinfo_super(st, info, NULL);
 		}
 		if (!info)
@@ -227,6 +232,11 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 	}
 
 	disks = malloc(max_disks * sizeof(mdu_disk_info_t));
+	if (!disks) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		return -ENOMEM;
+	}
 	for (d=0; d<max_disks; d++) {
 		disks[d].state = (1<<MD_DISK_REMOVED);
 		disks[d].major = disks[d].minor = 0;
@@ -252,6 +262,11 @@ int Detail(char *dev, int brief, int export, int test, char *homehost)
 	}
 
 	avail = calloc(array.raid_disks, 1);
+	if (!avail) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		return -ENOMEM;
+	}
 
 	for (d= 0; d < array.raid_disks; d++) {
 		mdu_disk_info_t disk = disks[d];
@@ -563,6 +578,13 @@ This is pretty boring
 				if (devices) {
 					devices = realloc(devices,
 							  strlen(devices)+1+strlen(dv)+1);
+					if (!devices) {
+						fprintf(stderr, Name ": %s "
+							"unable to realloc "
+							"memory\n", __func__);
+						return -ENOMEM;
+					}
+
 					strcat(strcat(devices,","),dv);
 				} else
 					devices = strdup(dv);
-- 
1.7.6.4

[PATCH 05/13] Handle malloc() failure in Examine.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Examine.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Examine.c b/Examine.c
index 5d71e53..74b5f80 100644
--- a/Examine.c
+++ b/Examine.c
@@ -131,6 +131,11 @@ int Examine(struct mddev_dev *devlist, int brief, int export, int scan,
 			}
 			if (!ap) {
 				ap = malloc(sizeof(*ap));
+				if (!ap) {
+					fprintf(stderr, Name ": %s unable to "
+						"allocate memory\n", __func__);
+					return -ENOMEM;
+				}
 				ap->devs = dl_head();
 				ap->next = arrays;
 				ap->spares = 0;
-- 
1.7.6.4

[PATCH 06/13] Handle malloc() errors in Manage.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Manage.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Manage.c b/Manage.c
index 2d8c916..024318b 100644
--- a/Manage.c
+++ b/Manage.c
@@ -143,6 +143,11 @@ static void remove_devices(int devnum, char *path)
 	be = base + strlen(base);
 
 	path2 = malloc(strlen(path)+20);
+	if (!path2) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		exit(1);
+	}
 	strcpy(path2, path);
 	pe = path2 + strlen(path2);
 	
@@ -898,8 +903,12 @@ int Manage_subdevs(char *devname, int fd,
 				 * As we are "--re-add"ing we must find a spare slot
 				 * to fill.
 				 */
-				char *used = malloc(array.raid_disks);
-				memset(used, 0, array.raid_disks);
+				char *used = calloc(array.raid_disks, 1);
+				if (!used) {
+					fprintf(stderr, Name ": %s unable to "
+						"allocate memory\n", __func__);
+					return -ENOMEM;
+				}
 				for (j=0; j< tst->max_devs; j++) {
 					mdu_disk_info_t disc2;
 					disc2.number = j;
-- 
1.7.6.4

[PATCH 07/13] Fix malloc() failure handling in Monitor.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Monitor.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 101bca4..675ec00 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -177,6 +177,13 @@ int Monitor(struct mddev_dev *devlist,
 				st->devname = strdup(mdlist->devname);
 			else {
 				st->devname = malloc(8+strlen(mdlist->devname)+1);
+				/*
+				 * Treat malloc() failure the same as above
+				 */
+				if (!st->devname) {
+					free(st);
+					continue;
+				}
 				strcpy(strcpy(st->devname, "/dev/md/"),
 				       mdlist->devname);
 			}
-- 
1.7.6.4

[PATCH 08/13] Handle malloc() failures in devline()

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

In addition let the error ripple down and handle it in the calling
functions.

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 config.c |   99 +++++++++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 73 insertions(+), 26 deletions(-)

diff --git a/config.c b/config.c
index c0a6baa..97c39e6 100644
--- a/config.c
+++ b/config.c
@@ -179,6 +179,11 @@ struct mddev_dev *load_partitions(void)
 		if (!name)
 			continue;
 		d = malloc(sizeof(*d));
+		if (!d) {
+			fprintf(stderr, Name ": %s unable to allocate memory\n",
+				__func__);
+			continue;
+		}
 		d->devname = strdup(name);
 		d->next = rv;
 		d->used = 0;
@@ -343,15 +348,22 @@ static void createline(char *line)
 	}
 }
 
-void devline(char *line)
+int devline(char *line)
 {
 	char *w;
 	struct conf_dev *cd;
+	int rv = 0;
 
 	for (w=dl_next(line); w != line; w=dl_next(w)) {
 		if (w[0] == '/' || strcasecmp(w, "partitions") == 0 ||
 		    strcasecmp(w, "containers") == 0) {
 			cd = malloc(sizeof(*cd));
+			if (!cd) {
+				fprintf(stderr, Name ": %s unable to allocate "
+					"memory\n", __func__);
+				rv = -ENOMEM;
+				goto fail;
+			}
 			cd->name = strdup(w);
 			cd->next = cdevlist;
 			cdevlist = cd;
@@ -360,6 +372,15 @@ void devline(char *line)
 				w);
 		}
 	}
+out:
+	return rv;
+fail:
+	while(cdevlist) {
+		cd = cdevlist->next;
+		free(cdevlist);
+		cdevlist = cd;
+	}
+	goto out;
 }
 
 struct mddev_ident *mddevlist = NULL;
@@ -711,7 +732,7 @@ void autoline(char *line)
 	free(seen);
 }
 
-int loaded = 0;
+static int loaded = 0;
 
 static char *conffile = NULL;
 void set_conffile(char *file)
@@ -719,27 +740,34 @@ void set_conffile(char *file)
 	conffile = file;
 }
 
-void load_conffile(void)
+int load_conffile(void)
 {
 	FILE *f;
 	char *line;
+	int rv = 0;
 
-	if (loaded) return;
+	if (loaded > 0)
+		return rv;
+	if (loaded < 0)
+		return loaded;
 	if (conffile == NULL)
 		conffile = DefaultConfFile;
 
 	if (strcmp(conffile, "none") == 0) {
 		loaded = 1;
-		return;
+		return rv;
 	}
 	if (strcmp(conffile, "partitions")==0) {
 		char *list = dl_strdup("DEV");
 		dl_init(list);
 		dl_add(list, dl_strdup("partitions"));
-		devline(list);
+		rv = devline(list);
 		free_line(list);
-		loaded = 1;
-		return;
+		if (!rv)
+			loaded = 1;
+		else
+			loaded = -1;
+		return rv;
 	}
 	f = fopen(conffile, "r");
 	/* Debian chose to relocate mdadm.conf into /etc/mdadm/.
@@ -753,14 +781,20 @@ void load_conffile(void)
 		if (f)
 			conffile = DefaultAltConfFile;
 	}
-	if (f == NULL)
-		return;
+	if (f == NULL) {
+		rv = -errno;
+		goto out;
+	}
 
 	loaded = 1;
 	while ((line=conf_line(f))) {
 		switch(match_keyword(line)) {
 		case Devices:
-			devline(line);
+			rv = devline(line);
+			if (rv) {
+				loaded = -1;
+				goto out;
+			}
 			break;
 		case Array:
 			arrayline(line);
@@ -795,47 +829,60 @@ void load_conffile(void)
 		free_line(line);
 	}
 
+out:
 	fclose(f);
-
-/*    printf("got file\n"); */
+	return rv;
 }
 
 char *conf_get_mailaddr(void)
 {
-	load_conffile();
-	return alert_email;
+	if (!load_conffile())
+		return alert_email;
+	else
+		return NULL;
 }
 
 char *conf_get_mailfrom(void)
 {
-	load_conffile();
-	return alert_mail_from;
+	if (!load_conffile())
+		return alert_mail_from;
+	else
+		return NULL;
 }
 
 char *conf_get_program(void)
 {
-	load_conffile();
-	return alert_program;
+	if (!load_conffile())
+		return alert_program;
+	else
+		return NULL;
 }
 
 char *conf_get_homehost(int *require_homehostp)
 {
-	load_conffile();
-	if (require_homehostp)
-		*require_homehostp = require_homehost;
-	return home_host;
+	if (!load_conffile()) {
+		if (require_homehostp)
+			*require_homehostp = require_homehost;
+		return home_host;
+	} else
+		return NULL;
 }
 
 struct createinfo *conf_get_create_info(void)
 {
-	load_conffile();
-	return &createinfo;
+	if (!load_conffile())
+		return &createinfo;
+	else
+		return NULL;
 }
 
 struct mddev_ident *conf_get_ident(char *dev)
 {
 	struct mddev_ident *rv;
-	load_conffile();
+	int r;
+	r = load_conffile();
+	if (r)
+		return NULL;
 	rv = mddevlist;
 	while (dev && rv && (rv->devname == NULL
 			     || !devname_matches(dev, rv->devname)))
-- 
1.7.6.4

[PATCH 09/13] Fix malloc handling in dlink.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 dlink.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/dlink.c b/dlink.c
index d734281..168ca2f 100644
--- a/dlink.c
+++ b/dlink.c
@@ -15,8 +15,10 @@ void *dl_head()
 {
     void *h;
     h = dl_alloc(0);
-    dl_next(h) = h;
-    dl_prev(h) = h;
+    if (h) {
+	    dl_next(h) = h;
+	    dl_prev(h) = h;
+    }
     return h;
 }
 
-- 
1.7.6.4

[PATCH 10/13] Handle malloc() failure in Grow.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 Grow.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Grow.c b/Grow.c
index 0e4dd10..eef95ed 100644
--- a/Grow.c
+++ b/Grow.c
@@ -3111,6 +3111,8 @@ static void validate(int afd, int bfd, unsigned long long offset)
 			abuflen = len;
 			abuf = malloc(abuflen);
 			bbuf = malloc(abuflen);
+			if (!abuf || !bbuf)
+				fail("out of memory");
 		}
 
 		lseek64(bfd, offset+__le64_to_cpu(bsb2.devstart2)*512, 0);
@@ -3501,6 +3503,11 @@ int Grow_restart(struct supertype *st, struct mdinfo *info, int *fdlist, int cnt
 
 		/* Now need the data offsets for all devices. */
 		offsets = malloc(sizeof(*offsets)*info->array.raid_disks);
+		if (!offsets) {
+			fprintf(stderr, Name ": %s unable to allocate memory\n",
+				__func__);
+			return -ENOMEM;
+		}
 		for(j=0; j<info->array.raid_disks; j++) {
 			if (fdlist[j] < 0)
 				continue;
-- 
1.7.6.4

[PATCH 11/13] Handle malloc() failure in mdopen.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 mdopen.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/mdopen.c b/mdopen.c
index 555ab84..1a795aa 100644
--- a/mdopen.c
+++ b/mdopen.c
@@ -65,6 +65,12 @@ void make_parts(char *dev, int cnt)
 	} else
 		   return;
 	name = malloc(nlen);
+	if (!name) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		return;
+	}
+
 	for (i=1; i <= cnt ; i++) {
 		struct stat stb2;
 		snprintf(name, nlen, "%s%s%d", dev, dig?"p":"", i);
-- 
1.7.6.4

[PATCH 12/13] Handle malloc() failure in mdstat.c

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 mdstat.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/mdstat.c b/mdstat.c
index abf6bf9..346a1e8 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -104,6 +104,11 @@ static int add_member_devname(struct dev_member **m, char *name)
 		return 0;
 
 	new = malloc(sizeof(*new));
+	if (!new) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		return 0;
+	}
 	new->name = strndup(name, t - name);
 	new->next = *m;
 	*m = new;
-- 
1.7.6.4

[PATCH 13/13] Catch malloc() failure

[Jes.Sorensen]

From: Jes Sorensen <Jes.Sorensen@redhat.com>

Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
---
 policy.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/policy.c b/policy.c
index 7959c97..4908ab6 100644
--- a/policy.c
+++ b/policy.c
@@ -47,6 +47,11 @@ static void pol_new(struct dev_policy **pol, char *name, const char *val,
 	const char *real_metadata = NULL;
 	int i;
 
+	if (!n) {
+		fprintf(stderr, Name ": %s unable to allocate memory\n",
+			__func__);
+		return;
+	}
 	n->name = name;
 	n->value = val;
 
-- 
1.7.6.4

Re: [PATCH 00/13] Add missing handling of malloc() failure

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2200 bytes --]

On Wed, 26 Oct 2011 17:30:13 +0200 Jes.Sorensen@redhat.com wrote:

> From: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> Hi,
> 
> I cam across a couple of these last week while I was chasing the
> assemble problem of IMSM raids, so I started going through the code to
> fix up some more of these.
> 
> This is just low hanging fruit, and there is more to be done, but at
> least it's a few fixes.

Hi,

 I'm not at all sure this sort of thing is worth it.
 There is no guarantee that the memory is actually there when malloc returns
 non-NULL.  You might only get a page mapped when you access the memory, and
 equally the failure could happen at that time.

 If there is something genuinely useful that can be done when malloc fails,
 the it might make sense, but just printing a message an exiting seems
 pointless.

 If we were to go that route, I would probably want to use a #define to
 replace everything with a wrapper (xmalloc??) that printed a message and
 failed.

 Do you have a strong reason to add these checks?

Thanks,
NeilBrown


> 
> Cheers,
> Jes
> 
> 
> Jes Sorensen (13):
>   count_active() catch malloc() failure
>   Catch malloc() failures
>   Try to catch malloc() failures in Assemble.c
>   Attempt to catch malloc() failure in Detail.c
>   Handle malloc() failure in Examine.c
>   Handle malloc() errors in Manage.c
>   Fix malloc() failure handling in Monitor.c
>   Handle malloc() failures in devline()
>   Fix malloc handling in dlink.c
>   Handle malloc() failure in Grow.c
>   Handle malloc() failure in mdopen.c
>   Handle malloc() failure in mdstat.c
>   Catch malloc() failure
> 
>  Assemble.c    |   37 ++++++++++++++++++++-
>  Detail.c      |   22 +++++++++++++
>  Examine.c     |    5 +++
>  Grow.c        |    7 ++++
>  Incremental.c |    4 ++
>  Manage.c      |   13 ++++++-
>  Monitor.c     |    7 ++++
>  config.c      |   99 ++++++++++++++++++++++++++++++++++++++++++---------------
>  dlink.c       |    6 ++-
>  mdopen.c      |    6 +++
>  mdstat.c      |    5 +++
>  policy.c      |    5 +++
>  util.c        |   19 +++++++++--
>  13 files changed, 200 insertions(+), 35 deletions(-)
> 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 00/13] Add missing handling of malloc() failure

[Jes Sorensen]

On 10/27/11 06:49, NeilBrown wrote:
>> Hi,
>> > 
>> > I cam across a couple of these last week while I was chasing the
>> > assemble problem of IMSM raids, so I started going through the code to
>> > fix up some more of these.
>> > 
>> > This is just low hanging fruit, and there is more to be done, but at
>> > least it's a few fixes.
> Hi,
> 
>  I'm not at all sure this sort of thing is worth it.
>  There is no guarantee that the memory is actually there when malloc returns
>  non-NULL.  You might only get a page mapped when you access the memory, and
>  equally the failure could happen at that time.
> 
>  If there is something genuinely useful that can be done when malloc fails,
>  the it might make sense, but just printing a message an exiting seems
>  pointless.
> 
>  If we were to go that route, I would probably want to use a #define to
>  replace everything with a wrapper (xmalloc??) that printed a message and
>  failed.
> 
>  Do you have a strong reason to add these checks?

Hi Neil,

There are a couple of reasons for this. You are right that things could
happen during touching the page at first, but in general malloc() is
supposed to fail if the system is unable to provide the pages.

The main reason why I think this is worth doing is for support reasons.
While in most cases these cases will never fail, I much prefer a user to
find out of memory messages in their log files or on the terminal, than
having them report a segfault which we need to investigate the reason
of. Having better error messages in place should reduce the cost of this.

Second, there are cases where malloc() fails where I think we really
need to try and unwind, rather than possibly leaving raid or metadata in
an inconsistent state. Passing errors back should help with this.

Third I think it is good practice not to leave error codes unchecked. In
the cases where I let the code exit() on error, I was pretty much
following the precedence in the same function I added the check to. I
don't think it is ideal, but I do think it is a starting point.

Some of the bits in my patchset could definitely handle the errors
better, but I still don't know all the code that well, so I think it is
a start. Getting error handling consistent across the board is a big
project and I don't think it is realistic to do it all in one go.

Cheers,
Jes