From owner-svn-src-head@FreeBSD.ORG Thu Jun 21 02:38:56 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9A761106566B; Thu, 21 Jun 2012 02:38:56 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail15.syd.optusnet.com.au (mail15.syd.optusnet.com.au [211.29.132.196]) by mx1.freebsd.org (Postfix) with ESMTP id 1EE498FC0C; Thu, 21 Jun 2012 02:38:55 +0000 (UTC) Received: from c122-106-171-232.carlnfd1.nsw.optusnet.com.au (c122-106-171-232.carlnfd1.nsw.optusnet.com.au [122.106.171.232]) by mail15.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q5L2cVBq028971 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 21 Jun 2012 12:38:34 +1000 Date: Thu, 21 Jun 2012 12:38:31 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: d@delphij.net In-Reply-To: <4FE243A3.1070202@delphij.net> Message-ID: <20120621112708.W901@besplex.bde.org> References: <201206200638.q5K6cg7u024024@svn.freebsd.org> <20120621015220.J2636@besplex.bde.org> <4FE1FC23.9000904@freebsd.org> <690DF487-F7CB-421E-B6BC-F7CE6BC0F658@bsdimp.com> <4FE23F54.5060409@freebsd.org> <4FE243A3.1070202@delphij.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: src-committers@freebsd.org, Eitan Adler , Colin Percival , svn-src-all@freebsd.org, Warner Losh , Bruce Evans , svn-src-head@freebsd.org Subject: Re: svn commit: r237286 - head/lib/libc/gen X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jun 2012 02:38:56 -0000 On Wed, 20 Jun 2012, Xin Li wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA256 > > On 06/20/12 14:23, Colin Percival wrote: >> On 06/20/12 14:15, Warner Losh wrote: >>> On Jun 20, 2012, at 10:36 AM, Colin Percival wrote: >>>> On 06/20/12 09:27, Bruce Evans wrote: >>>>> 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(). >>>> >>>> Why not? Because correctly designed APIs handle errors by returning an error status. Less correctly designed APIs might handle errors by dumping core, but then they have to document this. >>> We've tried to avoid things that make the library dump core... >> >> You mean, we avoid it except in the places where we don't? It >> seems to me that dumping core is exactly the right way to handle a >> "can't ever happen" situation inside libc -- just like the ~250 >> instances of assert() in jemalloc. Doesn't inspire confidence. I missed this because jemalloc doesn't include assert.h. It rolls its own assert() "to reduce the chance of deadlock during assertion failure". The library assert is indeed unusable in malloc() since it calls stdio, which may call malloc(). (It is a standard bug that assert() must write on stderr, since write() and STDERR_FILENO are not available in plain STDC. stderr is under more direct control of the application than is STDERR_FILENO. Although it is normally unbuffered, the application may set it into some mode that is buffered with the buffer not yet allocated, or otherwise uses malloc(). jemalloc's assert() avoids this problem by writing to STDERR_FILENO). But even in jemalloc, the assert()s are in debugging code and are turned off in production. It is apparently not ready for production, since MALLOC_PRODUCTION is apparently never configured. MALLOC_PRODUCTION also turns off the default malloc options of AJ. The default A option gives much larger standards breakage and core dumps, by aborting for all warnings. Not configuring MALLOC_PRODUCTION also results in configuring MALLOC_DEBUG and MALLOC_STATS. These result in malloc() using stdio in other ways, although it has mounds of code to avoid this in the production version. >> If you mean "passing an invalid parameter to a library function >> shouldn't result in a core dump", I agree -- but that's not the >> case here. Any undefined behaviour can cause a core dump, even when it is an invalid parameter passed to a library function (examples are null pointers passed to string and memory functions). However, the library should be more carefully written than applications, so it shouldn't have any undefined behaviours, and if it actually checks for undefined behaviours then it should try not to abort when it finds one. > But malloc() is a rare place that we typically consider as "low level" > enough where, no better remedies are provided from API prospective -- > there is nothing better than crashing the program immediately, since > that would likely to lead us to where the smoking gun is. Library > procedures normally detect and report errors, but don't handle them > like this. > > Also, as Bruce pointed out, it's a case that can never happen and thus > the explicit assert is just a waste of space. The only good think about the assert() in closelog() is that it detects a problem (that can't happen) in some cases and aborts before destroying the evidence. Here are all uses of LogFile in syslog.c. This shows that the assert() is even more bogus than I thought (since there are several closes of LogFile, but only an assert() for the recently changed one). Of course there are no assert()s for critical uses of LogFile. % static int LogFile = -1; /* fd for log */ % ... % if (send(LogFile, tbuf, cnt, 0) < 0) { % ... % if (send(LogFile, tbuf, cnt, 0) >= 0) { % ... These use LogFile without checking that it is not -1. This error can happen -- see below. % static void % disconnectlog(void) % { % /* % * If the user closed the FD and opened another in the same slot, % * that's their problem. They should close it before calling on % * system services. % */ % if (LogFile != -1) { % _close(LogFile); % LogFile = -1; % } % status = NOCONN; /* retry connect */ % } disconnect() is very similar to closelog(). It already had the check to avoid the unnecessary close when LogFile has not been allocated, but doesn't have an assert(). % ... % static void % connectlog(void) % { % struct sockaddr_un SyslogAddr; /* AF_UNIX address of local logger */ % % if (LogFile == -1) { % if ((LogFile = _socket(AF_UNIX, SOCK_DGRAM, 0)) == -1) % return; % (void)_fcntl(LogFile, F_SETFD, FD_CLOEXEC); % } % if (LogFile != -1 && status == NOCONN) { Bogus check that LogFile != -1. If LogFile was -1, then we just called _socket() succeessfully to make it different from -1, or returned if _socket() failed. % ... % if (_connect(LogFile, (struct sockaddr *)&SyslogAddr, % ... % if (_connect(LogFile, (struct sockaddr *)&SyslogAddr, % ... % if (_connect(LogFile, (struct sockaddr *)&SyslogAddr, % ... % if (status == NOCONN) { % (void)_close(LogFile); % LogFile = -1; connectlog() also returns without allocating LogFile here. % } % ... % } % } Note that we don't bogusly assert() that LogFile >= 0 before we use it. We don't even check that it is not -1 before using it. And this seems to be an error that can actually happen. The 2 _send()s in the above are done soon after calling connectlog(). But connectlog() returns void, and doesn't abort() when it fails to allocate LogFile. It just leaves or resets LogFile to -1 and lets the _send()s fail. % ... % void % closelog(void) % { % THREAD_LOCK(); % assert(LogFile >= -1); The bogus assert(). We check for an error that can't happen (_socket() returning a negative value that is not -1, or something clobbering LogFile to a negative value that is not -1). We don't and can't check for another error that can't happen (something clobbering LogFile to a _non-negative_ but still invalid or conflicting value). % if (LogFile != -1) { % (void)_close(LogFile); % LogFile = -1; % } % LogTag = NULL; % status = NOCONN; % THREAD_UNLOCK(); % } Since closelog() doesn't really use LogFile, handling the case that can't happen (LogFile < -1) by trying the close and resetting state was as harmless as possible. It mainly destroys the evidence that LogFIle (and perhaps other state) got clobbered. But it doesn't detect the problem as early as possible. If you want that, then you need to check LogFile every time before it is used. Don't forget to check for parity errors in the CPU before every instruction too. Bruce