svn commit: r300952 - head/usr.sbin/services_mkdb

[Don Lewis]

On 29 May, Alan Somers wrote:
> On Sun, May 29, 2016 at 4:41 AM, Ed Schouten <ed at freebsd.org> wrote:
>> Author: ed
>> Date: Sun May 29 10:41:27 2016
>> New Revision: 300952
>> URL: https://svnweb.freebsd.org/changeset/base/300952
>>
>> Log:
>>   Invoke the dirname() function in a POSIX compliant way.
>>
>>   POSIX requires that the argument of dirname() is of type "char *". In
>>   other words, the input buffer can be modified by the function to store
>>   the directory name.
>>
>>   Pull a copy of the string before calling dirname(). We don't care about
>>   freeing up the memory afterwards, as this is done at the very bottom of
>>   main(), right before the program terminates.
>>
>>   Reviewed by:  bapt
>>   Differential Revision:        https://reviews.freebsd.org/D6628
>>
>> Modified:
>>   head/usr.sbin/services_mkdb/services_mkdb.c
>>
>> Modified: head/usr.sbin/services_mkdb/services_mkdb.c
>> ==============================================================================
>> --- head/usr.sbin/services_mkdb/services_mkdb.c Sun May 29 07:39:56 2016        (r300951)
>> +++ head/usr.sbin/services_mkdb/services_mkdb.c Sun May 29 10:41:27 2016        (r300952)
>> @@ -92,7 +92,7 @@ main(int argc, char *argv[])
>>         size_t   cnt = 0;
>>         StringList *sl, ***svc;
>>         size_t port, proto;
>> -       char *dbname_dir;
>> +       char *dbname_dir, *dbname_dirbuf;
>>         int dbname_dir_fd = -1;
>>
>>         setprogname(argv[0]);
>> @@ -172,7 +172,8 @@ main(int argc, char *argv[])
>>          * fsync() to the directory where file lies
>>          */
>>         if (rename(tname, dbname) == -1 ||
>> -           (dbname_dir = dirname(dbname)) == NULL ||
>> +           (dbname_dirbuf = strdup(dbname)) == NULL ||
>> +           (dbname_dir = dirname(dbname_dirbuf)) == NULL ||
>>             (dbname_dir_fd = open(dbname_dir, O_RDONLY|O_DIRECTORY)) == -1 ||
>>             fsync(dbname_dir_fd) != 0) {
>>                 if (dbname_dir_fd != -1)
>>
> 
> Even though the program is about to exit, it's worth freeing the
> memory just to make Coverity shut up.

I usually don't bother in that situation because it's not worth the
added code complexity.  In Coverity, I'll mark them as a bug, set the
severity to insignficant, and set action to ignore.  The situation is
different if I think the code might get used elsewhere and the memory
could be wasted for a longer period of time.

I've also run into situations where there is a real downside to doing
this sort of cleanup before calling exit().  Generally this is with a
process that has allocated some sort of large data structure in memory
over a long period of time, especially if the process has grown larger
than physical RAM and is partially resident in swap.  It's maddening to
watch the process page madly for an extended period of time as it chases
pointers all of ther place and calls free() zillions of times when it
would be so much faster to just call exit().  Adding more RAM may not be
an option in such cases, since I've had to endure this on machines that
had the maximum amount of RAM that they could hold.