svn commit: r344389 - head/usr.sbin/newsyslog

[Enji Cooper]

> On Feb 20, 2019, at 5:17 PM, Bruce Evans <brde at optusnet.com.au> wrote:
> 
> 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.

I agree with Bruce. Items like these should be ignored in the Coverity UI as false positives with reasoning, like “global variables; freed on exit”.

As others have noted in past mailing threads, freeing variables on exit can cause applications to hang for a period of time, while the memory is being reclaimed. I think it’s best to ignore these kinds of allocations on exit to avoid introducing unnecessary complexity in the program, as they’re benign issues.

Thank you,
-Enji