From owner-svn-src-all@FreeBSD.ORG Thu Dec 6 01:36:17 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 939212F7; Thu, 6 Dec 2012 01:36:17 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail13.syd.optusnet.com.au (mail13.syd.optusnet.com.au [211.29.132.194]) by mx1.freebsd.org (Postfix) with ESMTP id 2614E8FC08; Thu, 6 Dec 2012 01:36:16 +0000 (UTC) Received: from c122-106-175-26.carlnfd1.nsw.optusnet.com.au (c122-106-175-26.carlnfd1.nsw.optusnet.com.au [122.106.175.26]) by mail13.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id qB61a6Q2002470 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 6 Dec 2012 12:36:07 +1100 Date: Thu, 6 Dec 2012 12:36:06 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Mateusz Guzik Subject: Re: svn commit: r243898 - head/usr.sbin/pw In-Reply-To: <20121205144842.GA29435@dft-labs.eu> Message-ID: <20121206120510.B1387@besplex.bde.org> References: <201212051356.qB5Duu0c068432@svn.freebsd.org> <20121205144842.GA29435@dft-labs.eu> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Zr21sKHG c=1 sm=1 a=OyMAZ6m7iocA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=dkUHpakOesgA:10 a=BAVIfVq5poG-GzVfkvAA:9 a=CjuIK1q_8ugA:10 a=bxQHXO5Py4tHmhUgaywp5w==:117 Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Dec 2012 01:36:17 -0000 On Wed, 5 Dec 2012, Mateusz Guzik wrote: > On Wed, Dec 05, 2012 at 01:56:56PM +0000, Eitan Adler wrote: >> >> Log: >> Simplify string duplication: use strdup instead of malloc + strcpy >> >> Submitted by: db >> Approved by: cperciva >> MFC after: 2 weeks >> >> Modified: >> head/usr.sbin/pw/grupd.c >> >> Modified: head/usr.sbin/pw/grupd.c >> ============================================================================== >> --- head/usr.sbin/pw/grupd.c Wed Dec 5 13:56:52 2012 (r243897) >> +++ head/usr.sbin/pw/grupd.c Wed Dec 5 13:56:56 2012 (r243898) >> @@ -50,12 +50,11 @@ setgrdir(const char * dir) >> { >> if (dir == NULL) >> return -1; >> - else { >> - char * d = malloc(strlen(dir)+1); >> - if (d == NULL) >> - return -1; >> - grpath = strcpy(d, dir); >> - } >> + else >> + grpath = strdup(dir); >> + if (grpath == NULL) >> + return -1; >> + >> return 0; >> } >> > > (yes, I know pw is a lot of work) > > This can be further deuglified with slight change: > > --- a/usr.sbin/pw/grupd.c > +++ b/usr.sbin/pw/grupd.c > @@ -50,8 +50,8 @@ setgrdir(const char * dir) > { > if (dir == NULL) > return -1; > - else > - grpath = strdup(dir); > + > + grpath = strdup(dir); > if (grpath == NULL) > return -1; The extra blank line reuglifies it. > Also the only consumer does not check for errors, but after cursory look > I'm not sure if it is ok to just exit. malloc() can't fail, and if it does then aborting immediately is as good as anything, especially in software of the, er, quality of pw. It only checks the result of malloc() in some places, so there is no point in checking it in other places. Some of the unchecked places are: - bm_alloc() is a wrapper around malloc(). It checks, but it returns NULL if malloc() failed, and neither of its 2 callers check if bm_alloc() succeeded. - in copymkdir(), there is a malloc(4096) which is not checked (except the buffer is immediatly used for read(2), and the kernel does check. pw even checks the result of the read. But then it mishandles the resulting EFAULT error from read(2), along with all other read errors, by quitting the read/write loop and not telling anyone that it failed) - in main(), there is a malloc(MAXPATHLEN) followed immediately by a snprintf() to it - in read_userconfig() and write_userconfig(), there is a malloc(LNBUFSZ) (1024) followed by indirection of the result. Lines created by this are extended by extendline(), which uses realloc() and checks the result. It returns -1 on error, and this is actually checked for, but the handling seems to be broken much like for the read/write loop (just quit the i/o and don't tell anyone about the error). So it seems that malloc() failures are checked for mainly in small utility functions, and it is mostly useless there too since larger functions don't check the results any more than they check for malloc(), and when they do check then the results are worse than for aborting in the utility functions. Bruce