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

[Bruce Evans]

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