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

[Warner Losh]

On Wed, Feb 20, 2019, 9:59 PM Enji Cooper <yaneurabeya at gmail.com wrote:

>
> > 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.
>


It's been a long running debate since 92 or so when purify came out and
this problem started to be found. In the last 25 years the question hasn't
been settled. I tend to think it's a waste of time, though I get that
issues like this create a lot of false positives.

Warner

Thank you,
> -Enji
>