From owner-svn-src-head@freebsd.org Thu Feb 21 01:17:34 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id C197014E4ECE; Thu, 21 Feb 2019 01:17:34 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 419D081547; Thu, 21 Feb 2019 01:17:33 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 7B5503D6168; Thu, 21 Feb 2019 12:17:23 +1100 (AEDT) Date: Thu, 21 Feb 2019 12:17:21 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: David Bright cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r344389 - head/usr.sbin/newsyslog In-Reply-To: <201902202205.x1KM5iZX036319@repo.freebsd.org> Message-ID: <20190221121712.Y989@besplex.bde.org> References: <201902202205.x1KM5iZX036319@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=P6RKvmIu c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=tFM545eivrnJSQZQWzUA:9 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 419D081547 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.95 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.95)[-0.949,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 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 Feb 2019 01:17:34 -0000 On Wed, 20 Feb 2019, David Bright wrote: > Log: > Complete fix for CID 1007454, CID 1007453: Resource leak in newsyslog > > The result of a strdup() was stored in a global variable and not freed > before program exit. This is a follow-up to r343906. That change This was an especially large bug in Coverity. Understanding that exit(3) exits is about the first thing to understand for a checker. Now it is also a style bug in the source code. > attempted to plug these resource leaks but managed to miss a code path > on which the leak still occurs. Plug the leak on that path, too. > Modified: head/usr.sbin/newsyslog/newsyslog.c > ============================================================================== > --- head/usr.sbin/newsyslog/newsyslog.c Wed Feb 20 21:24:56 2019 (r344388) > +++ head/usr.sbin/newsyslog/newsyslog.c Wed Feb 20 22:05:44 2019 (r344389) > @@ -793,6 +793,9 @@ usage(void) > fprintf(stderr, > "usage: newsyslog [-CFNPnrsv] [-a directory] [-d directory] [-f config_file]\n" > " [-S pidfile] [-t timefmt] [[-R tagname] file ...]\n"); > + /* Free global dynamically-allocated storage. */ > + free(timefnamefmt); > + free(requestor); > exit(1); > } There was no leak here. exit(3) frees storage much more finally than free(3). It is especially obvious that there is no leak here, since the exit() is 1-2 lines later than the frees. In theory, exit() might fail because it tries to allocate 100 MB more storage but wouldn't fail if 100 bytes are freed here (applications can easily do this foot shooting by allocating without freeing in atexit() destructors). In practice, even allocation failures "can't happen", except in programs that use setrlimit followed but foot shooting to test the limits. setrlimit is now broken for this purpose, since it doesn't limit allocations done using mmap() instead of break(), and malloc() now uses mmap(). If coverity understood this and wanted to spam you with warnings, then it would not warn about this, but would warn about more important things like failure to fflush() or fclose() or check for or handle errors for all open streams before calling exit(). Also, if all callers of usage() are not understood, for failures to switch stderr to unbuffered mode before using it in usage(). The error reporting is even harder to do if stderr is not available. Windowing systems and even curses need to do lots more cleanup _before_ exit() and it may be difficult to clean up enough to print error messages using the windowing system. Bruce