From owner-freebsd-bugs@FreeBSD.ORG Thu Jul 14 15:30:19 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 6C60816A41C for ; Thu, 14 Jul 2005 15:30:19 +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 EE2CA43D5C for ; Thu, 14 Jul 2005 15:30:18 +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 j6EFUIHU024803 for ; Thu, 14 Jul 2005 15:30:18 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.3/8.13.1/Submit) id j6EFUIK2024802; Thu, 14 Jul 2005 15:30:18 GMT (envelope-from gnats) Resent-Date: Thu, 14 Jul 2005 15:30:18 GMT Resent-Message-Id: <200507141530.j6EFUIK2024802@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, Dan Lukes Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 6D18316A41C for ; Thu, 14 Jul 2005 15:23:33 +0000 (GMT) (envelope-from dan@kulesh.obluda.cz) Received: from kulesh.obluda.cz (kulesh.obluda.cz [193.179.22.243]) by mx1.FreeBSD.org (Postfix) with ESMTP id 2516E43D53 for ; Thu, 14 Jul 2005 15:23:31 +0000 (GMT) (envelope-from dan@kulesh.obluda.cz) Received: from kulesh.obluda.cz (localhost.eunet.cz [127.0.0.1]) by kulesh.obluda.cz (8.13.3/8.13.3) with ESMTP id j6EFNTg6034021 for ; Thu, 14 Jul 2005 17:23:30 +0200 (CEST) (envelope-from dan@kulesh.obluda.cz) Received: (from root@localhost) by kulesh.obluda.cz (8.13.3/8.13.1/Submit) id j6EFNTkl034020; Thu, 14 Jul 2005 17:23:29 +0200 (CEST) (envelope-from dan) Message-Id: <200507141523.j6EFNTkl034020@kulesh.obluda.cz> Date: Thu, 14 Jul 2005 17:23:29 +0200 (CEST) From: Dan Lukes To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Subject: 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: Thu, 14 Jul 2005 15:30:19 -0000 >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: