bin/83464: [ PATCH ] Unhandled malloc failures within libgeom

[Dan Lukes]

>Number:         83464
>Category:       bin
>Synopsis:       [ PATCH ] Unhandled malloc failures within libgeom
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    freebsd-bugs
>State:          open
>Quarter:        
>Keywords:       
>Date-Required:
>Class:          sw-bug
>Submitter-Id:   current-users
>Arrival-Date:   Thu Jul 14 15:30:18 GMT 2005
>Closed-Date:
>Last-Modified:
>Originator:     Dan Lukes
>Release:        FreeBSD 5.4-STABLE i386
>Organization:
Obludarium
>Environment:
System: FreeBSD 5.4-STABLE #8: Sat Jul 9 16:31:08 CEST 2005 i386
lib/libgeom/geom_getxml.c,v 1.1 2003/02/10 00:11:43 phk
lib/libgeom/geom_xml2tree.c,v 1.4 2004/03/09 21:14:18 jhb

>Description:
	Unhandled malloc failures & typo within libgeom
>How-To-Repeat:
>Fix:
	Major problem is unhandlet malloc failures

	In advance, there are two typos within code - second invocation of
sysctlbyname within geom_getxml() seems to called to obtain len of sysctl
structure, so second argument should be NULL. We can't use (p) here as it
may not be NULL everytime.

	Before third invocation of sysctlbyname() the p is allocated
as 'l+4096', but the len is claimed to be 'l' only during sysctlbyname()
call.

	The reallocf should be used instead of realloc - just for sure. Or
we are pretty sure the shrinking realloc can't fail for any reason ?

	BTW, someone should decide if this code isn't so dirty. Now the
alghoritm:
1. alloc 1MB buffer for config
2. when failed, ask for structure length then alloc buffer for it +4kB

	IMHO, the step 1 is trying to save one invocation od sysctlbyname
(the query for structure len) for the price of allocating 1MB memory.

	It price seems to be so high for me. The calling of geom_getxml() 
seems not to be so time sensitive. I recomment delete the first part of
code, so remaining portion start at 'l = 0' statement.

	If current code need 1T or 3T then shorter version require 2T
everytime (saving overhead from allocating 1MB memory). If we are interested
on processing time then two sysctlbyname() should be rewritten as one
sysctlnametomig() and two sysctl(). The resulting time should be about 1,3T

	The patch attached doesn't contain those recomended change. It patch only real
bugs. 

	I can send other patch with recomended changes when a commiter decide I
should do it.

--- patch begins here ---
--- lib/libgeom/geom_getxml.c.ORIG	Mon Feb 10 01:11:43 2003
+++ lib/libgeom/geom_getxml.c	Thu Jul 14 16:32:19 2005
@@ -47,19 +47,20 @@
 	if (p) {
 		i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0);
 		if (i == 0) {
-			p = realloc(p, strlen(p) + 1);
+			p = reallocf(p, strlen(p) + 1);
 			return (p);
 		}
 		free(p);
 	}
 	l = 0;
-	i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0);
+	i = sysctlbyname("kern.geom.confxml", NULL, &l, NULL, 0);
 	if (i != 0)
 		return (NULL);
-	p = malloc(l + 4096);
+	l+=4096;
+	p = malloc(l);
 	i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0);
 	if (i == 0) {
-		p = realloc(p, strlen(p) + 1);
+		p = reallocf(p, strlen(p) + 1);
 		return (p);
 	}
 	return (NULL);
--- lib/libgeom/geom_xml2tree.c.ORIG	Wed Mar 17 01:03:34 2004
+++ lib/libgeom/geom_xml2tree.c	Thu Jul 14 16:53:36 2005
@@ -105,6 +105,10 @@
 	}
 	if (!strcmp(name, "consumer") && mt->consumer == NULL) {
 		mt->consumer = calloc(1, sizeof *mt->consumer);
+		if (mt->consumer == NULL) {
+			warn("Problem during processing of '%s' element", name);
+			return;
+		}
 		mt->consumer->lg_id = id;
 		LIST_INSERT_HEAD(&mt->geom->lg_consumer, mt->consumer,
 		    lg_consumer);
@@ -121,6 +125,10 @@
 	}
 	if (!strcmp(name, "provider") && mt->provider == NULL) {
 		mt->provider = calloc(1, sizeof *mt->provider);
+		if (mt->provider == NULL) {
+			warn("Problem during processing of '%s' element", name);
+			return;
+		}
 		mt->provider->lg_id = id;
 		LIST_INSERT_HEAD(&mt->geom->lg_provider, mt->provider,
 		    lg_provider);
--- patch ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted: