Re: svn commit: r344389 - head/usr.sbin/newsyslog
[… other discussion omitted…] On Feb 21, 2019, at 12:22 PM, John Baldwin wrote: > > > I'm +1 on Bruce's point on this. I find it similar to the recent spate of > adding pointless '__dead2' annotations to usage functions that unconditionally > call exit() (and thus are already inferred as __dead2 by any compiler > written in this millenium) I’ve reverted (r344468) the two commits that contained the memory leak fixes at issue. Thanks for the feedback. -- David Bright d...@freebsd.org ___ 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"
Re: svn commit: r344389 - head/usr.sbin/newsyslog
On 2/20/19 9:20 PM, Warner Losh wrote: > On Wed, Feb 20, 2019, 9:59 PM Enji Cooper >> >>> On Feb 20, 2019, at 5:17 PM, Bruce Evans 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. I'm +1 on Bruce's point on this. I find it similar to the recent spate of adding pointless '__dead2' annotations to usage functions that unconditionally call exit() (and thus are already inferred as __dead2 by any compiler written in this millenium) -- John Baldwin ___ 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"
Re: svn commit: r344389 - head/usr.sbin/newsyslog
On Wed, Feb 20, 2019, 9:59 PM Enji Cooper > > On Feb 20, 2019, at 5:17 PM, Bruce Evans 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 > ___ 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"
Re: svn commit: r344389 - head/usr.sbin/newsyslog
> On Feb 20, 2019, at 5:17 PM, Bruce Evans 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"
Re: svn commit: r344389 - head/usr.sbin/newsyslog
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 ___ 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"
svn commit: r344389 - head/usr.sbin/newsyslog
Author: dab Date: Wed Feb 20 22:05:44 2019 New Revision: 344389 URL: https://svnweb.freebsd.org/changeset/base/344389 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 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. MFC after:3 days Sponsored by: Dell EMC Isilon Modified: head/usr.sbin/newsyslog/newsyslog.c 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); } ___ 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"
