> On Feb 20, 2019, at 5:17 PM, Bruce Evans <b...@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 _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
