Date: Thu, 6 Dec 2012 12:36:06 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Eitan Adler <eadler@FreeBSD.org> Subject: Re: svn commit: r243898 - head/usr.sbin/pw Message-ID: <20121206120510.B1387@besplex.bde.org> In-Reply-To: <20121205144842.GA29435@dft-labs.eu> References: <201212051356.qB5Duu0c068432@svn.freebsd.org> <20121205144842.GA29435@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121206120510.B1387>