Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 21 Jun 2012 02:27:23 +1000 (EST)
From:      Bruce Evans <brde@optusnet.com.au>
To:        Eitan Adler <eadler@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r237286 - head/lib/libc/gen
Message-ID:  <20120621015220.J2636@besplex.bde.org>
In-Reply-To: <201206200638.q5K6cg7u024024@svn.freebsd.org>
References:  <201206200638.q5K6cg7u024024@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 20 Jun 2012, Eitan Adler wrote:

> Log:
>  Don't close an uninitialized descriptor. [1]
>  Add a sanity check for the validity of the passed fd.

Library functions shouldn't use assert() or abort().

The fd is not passed, but is a static variable under syslog()'s control.

In libc, only the following use assert.h:
- stdio/xprintf*.c.  Very nonstd.
- rpc/*.c (perhaps not everything)
- db/*.c (but partly under DEBUG.  Not the standard use where assert() is
   controlled by NDEBUG)
- nameser/ns_print.c
- ia64/gen/unwind.c (but under SANITY.  The standard NDEBUG is too hard to
   use here too)
- gen/getgrent.c. 
- regex/grot/main.c (test program.  Not part of libc)
- regex/regex/utils.h (but ifdefed, and I think turned off in production.
   Spencer knows how to use NDEBUG)
- regex/regcomp.c (ifdefed)
- include/isc/list.h
- net/*.c (just 2 files)
- posix1e/acl*.c (perhaps not everything)
- inet/inet_net_pton.c
- iconv/*.c (perhaps not everything).

The list was only short enough to be described by "only" in not very old
versions of FreeBSD.  posix1e and iconv doubled the number of files.

> Modified: head/lib/libc/gen/syslog.c
> ==============================================================================
> --- head/lib/libc/gen/syslog.c	Wed Jun 20 04:11:34 2012	(r237285)
> +++ head/lib/libc/gen/syslog.c	Wed Jun 20 06:38:41 2012	(r237286)
> @@ -41,6 +41,7 @@ __FBSDID("$FreeBSD$");
> #include <sys/un.h>
> #include <netdb.h>
>
> +#include <assert.h>
> #include <errno.h>
> #include <fcntl.h>
> #include <paths.h>
> @@ -413,8 +414,11 @@ void
> closelog(void)
> {
> 	THREAD_LOCK();
> -	(void)_close(LogFile);
> -	LogFile = -1;
> +	assert(LogFile >= -1);

Since this assert() can't fail (unless the CPU has a parity error or
or the memory behind Logfile has a parity error or another memory error
or was clobbered by a buffer overrun), this assert() doesn't break the
library but just wastes space.

> +	if (LogFile != -1) {
> +		(void)_close(LogFile);
> +		LogFile = -1;
> +	}
> 	LogTag = NULL;
> 	status = NOCONN;
> 	THREAD_UNLOCK();

Bruce



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120621015220.J2636>