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

[John Baldwin]

On 2/20/19 9:20 PM, Warner Losh wrote:
> 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.

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