From owner-freebsd-bugs@FreeBSD.ORG Fri Jul 15 16:40:23 2005 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 3BC0B16A41C for ; Fri, 15 Jul 2005 16:40:23 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id C5A5043D45 for ; Fri, 15 Jul 2005 16:40:22 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.3/8.13.3) with ESMTP id j6FGeMO4054155 for ; Fri, 15 Jul 2005 16:40:22 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.3/8.13.1/Submit) id j6FGeM1C054154; Fri, 15 Jul 2005 16:40:22 GMT (envelope-from gnats) Date: Fri, 15 Jul 2005 16:40:22 GMT Message-Id: <200507151640.j6FGeM1C054154@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Dan Lukes Cc: Subject: Re: bin/83464: [ PATCH ] Unhandled malloc failures within libgeom X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Dan Lukes List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jul 2005 16:40:23 -0000 The following reply was made to PR bin/83464; it has been noted by GNATS. From: Dan Lukes To: bug-followup@FreeBSD.org Cc: Subject: Re: bin/83464: [ PATCH ] Unhandled malloc failures within libgeom Date: Fri, 15 Jul 2005 18:32:22 +0200 This is a multi-part message in MIME format. --------------070408060806070200010105 Content-Type: text/plain; charset=ISO-8859-2; format=flowed Content-Transfer-Encoding: 7bit Well, so rewritten patch follows. 1. get_geomxml() is rewritten 2. The recent gctl_check_alloc() assign pointer to constant string when dynamic allocation of error message failed - but gctl_free try to free the req->error even if not allocated dynamically. Corrected. 3. realloc() within gctl_new_arg can cause memory leak. Changed to reallocf() 4. ap->name=strdup(name) within gctl_ro_param() and gctl_rw_param() not checked for failures. Corrected. 5. several malloc()/calloc()/strdup() used within StartElement() and EndElement() not checked for failures. As those functions have no way to return eror, I use the warn() then return. The XML parsing should fail later due syntax error because element in question has not been processed. WARNS can be raised to 6 Dan --------------070408060806070200010105 Content-Type: text/plain; name="patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="patch" --- lib/libgeom/geom_getxml.c.ORIG Mon Feb 10 01:11:43 2003 +++ lib/libgeom/geom_getxml.c Fri Jul 15 17:22:18 2005 @@ -39,28 +39,21 @@ geom_getxml() { char *p; - size_t l; - int i; + size_t l = 0; + int mib[3]; + size_t sizep = sizeof(mib)/sizeof(*mib); - l = 1024 * 1024; /* Start big, realloc back */ - p = malloc(l); - if (p) { - i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0); - if (i == 0) { - p = realloc(p, strlen(p) + 1); - return (p); - } + if (sysctlnametomib("kern.geom.confxml", mib, &sizep) != 0) + return (NULL); + if (sysctl(mib, sizep, NULL, &l, NULL, 0) != 0) + return (NULL); + l+=4096; + if ((p = malloc(l)) == NULL) + return (NULL); + if (sysctl(mib, sizep, p, &l, NULL, 0) != 0) { free(p); - } - l = 0; - i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0); - if (i != 0) return (NULL); - p = malloc(l + 4096); - i = sysctlbyname("kern.geom.confxml", p, &l, NULL, 0); - if (i == 0) { - p = realloc(p, strlen(p) + 1); - return (p); } - return (NULL); + + return (reallocf(p, strlen(p) + 1)); } --- lib/libgeom/geom_ctl.c.ORIG Mon Jun 2 20:54:50 2003 +++ lib/libgeom/geom_ctl.c Fri Jul 15 18:01:44 2005 @@ -45,6 +45,8 @@ #define GCTL_TABLE 1 #include +char nomemmsg[] = "Could not allocate memory"; + void gctl_dump(struct gctl_req *req, FILE *f) { @@ -109,7 +111,7 @@ return; gctl_set_error(req, "Could not allocate memory"); if (req->error == NULL) - req->error = "Could not allocate memory"; + req->error = nomemmsg; } /* @@ -134,7 +136,7 @@ struct gctl_req_arg *ap; req->narg++; - req->arg = realloc(req->arg, sizeof *ap * req->narg); + req->arg = reallocf(req->arg, sizeof *ap * req->narg); gctl_check_alloc(req, req->arg); if (req->arg == NULL) { req->narg = 0; @@ -149,14 +151,20 @@ gctl_ro_param(struct gctl_req *req, const char *name, int len, const void* value) { struct gctl_req_arg *ap; + char *ap_name; if (req == NULL || req->error != NULL) return; + ap_name = strdup(name); + gctl_check_alloc(req, ap_name); + if (ap_name == NULL) + return; ap = gctl_new_arg(req); - if (ap == NULL) + if (ap == NULL) { + free(ap_name); return; - ap->name = strdup(name); - gctl_check_alloc(req, ap->name); + } + ap->name = ap_name; ap->nlen = strlen(ap->name) + 1; ap->value = __DECONST(void *, value); ap->flag = GCTL_PARAM_RD; @@ -172,14 +180,20 @@ gctl_rw_param(struct gctl_req *req, const char *name, int len, void* value) { struct gctl_req_arg *ap; + char *ap_name; if (req == NULL || req->error != NULL) return; + ap_name = strdup(name); + gctl_check_alloc(req, ap_name); + if (ap_name == NULL) + return; ap = gctl_new_arg(req); - if (ap == NULL) + if (ap == NULL) { + free(ap_name); return; - ap->name = strdup(name); - gctl_check_alloc(req, ap->name); + } + ap->name = ap_name; ap->nlen = strlen(ap->name) + 1; ap->value = value; ap->flag = GCTL_PARAM_RW; @@ -201,12 +215,11 @@ req->version = GCTL_VERSION; req->lerror = BUFSIZ; /* XXX: arbitrary number */ - req->error = malloc(req->lerror); + req->error = calloc(1, req->lerror); if (req->error == NULL) { gctl_check_alloc(req, req->error); return (req->error); } - memset(req->error, 0, req->lerror); req->lerror--; fd = open(_PATH_DEV PATH_GEOM_CTL, O_RDONLY); if (fd < 0) @@ -232,7 +245,7 @@ free(req->arg[i].name); } free(req->arg); - if (req->error != NULL) + if (req->error != NULL && req->error != nomemmsg) free(req->error); free(req); } --- lib/libgeom/Makefile.ORIG Thu Jul 14 16:34:19 2005 +++ lib/libgeom/Makefile Fri Jul 15 17:44:37 2005 @@ -10,7 +10,7 @@ CFLAGS += -I${.CURDIR} -WARNS?= 3 +WARNS?= 6 DPADD= ${LIBBSDXML} ${LIBSBUF} LDADD= -lbsdxml -lsbuf --- lib/libgeom/geom_xml2tree.c.ORIG Tue May 24 12:10:38 2005 +++ lib/libgeom/geom_xml2tree.c Fri Jul 15 18:13:14 2005 @@ -84,6 +84,10 @@ } if (!strcmp(name, "class") && mt->class == NULL) { mt->class = calloc(1, sizeof *mt->class); + if (mt->class == NULL) { + warn("Problem during processing of '%s' element", name); + return; + } mt->class->lg_id = id; LIST_INSERT_HEAD(&mt->mesh->lg_class, mt->class, lg_class); LIST_INIT(&mt->class->lg_geom); @@ -92,6 +96,10 @@ } if (!strcmp(name, "geom") && mt->geom == NULL) { mt->geom = calloc(1, sizeof *mt->geom); + if (mt->geom == NULL) { + warn("Problem during processing of '%s' element", name); + return; + } mt->geom->lg_id = id; LIST_INSERT_HEAD(&mt->class->lg_geom, mt->geom, lg_geom); LIST_INIT(&mt->geom->lg_provider); @@ -105,6 +113,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 +133,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); @@ -162,6 +178,10 @@ mt = userData; sbuf_finish(mt->sbuf[mt->level]); p = strdup(sbuf_data(mt->sbuf[mt->level])); + if (p == NULL) { + warn("Problem during processing of '%s' element", name); + return; + } sbuf_delete(mt->sbuf[mt->level]); mt->sbuf[mt->level] = NULL; mt->level--; @@ -212,8 +232,16 @@ } if (mt->config != NULL) { - gc = calloc(sizeof *gc, 1); + gc = calloc(1, sizeof *gc); + if (gc == NULL) { + warn("Problem during processing of '%s' element", name); + return; + } gc->lg_name = strdup(name); + if (gc->lg_name == NULL) { + warn("Problem during processing of '%s' element", name); + return; + } gc->lg_val = p; LIST_INSERT_HEAD(mt->config, gc, lg_config); return; --------------070408060806070200010105--