From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from scc-mailout-kit-01.scc.kit.edu (scc-mailout-kit-01.scc.kit.edu [129.13.231.81]) by fantadrom.bsd.lv (OpenSMTPD) with ESMTP id c777a55f for ; Thu, 7 Apr 2016 13:10:24 -0500 (EST) Received: from asta-nat.asta.uni-karlsruhe.de ([172.22.63.82] helo=hekate.usta.de) by scc-mailout-kit-01.scc.kit.edu with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (envelope-from ) id 1aoENq-0004us-8I; Thu, 07 Apr 2016 20:10:24 +0200 Received: from donnerwolke.usta.de ([172.24.96.3]) by hekate.usta.de with esmtp (Exim 4.77) (envelope-from ) id 1aoENp-00073Y-Ic; Thu, 07 Apr 2016 20:10:21 +0200 Received: from athene.usta.de ([172.24.96.10]) by donnerwolke.usta.de with esmtp (Exim 4.84_2) (envelope-from ) id 1aoENb-0002Dd-KS; Thu, 07 Apr 2016 20:10:07 +0200 Received: from localhost (1031@localhost [local]); by localhost (OpenSMTPD) with ESMTPA id ab51ca2e; Thu, 7 Apr 2016 20:10:21 +0200 (CEST) Date: Thu, 7 Apr 2016 20:10:21 +0200 From: Ingo Schwarze To: Abhinav Upadhyay Cc: tech@mdocml.bsd.lv Subject: Re: Possible Memory leak in mdoc_validate.c? Message-ID: <20160407181021.GA15064@athene.usta.de> References: X-Mailinglist: mdocml-tech Reply-To: tech@mdocml.bsd.lv MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Hi Abhinav, Thanks for still caring about mandoc. Abhinav Upadhyay wrote on Thu, Apr 07, 2016 at 11:08:42PM +0530: > The function post_os in mdoc_validate.c has a static char *defbuf, > which doesn't seem to be free'd after allocation. Shouldn't it be > free'd and set to NULL if it was allocated? No, that would defeat the purpose. The whole point of the exercise is to keep the string around, such that it does not need to be constructed again. > On Linux, valgrind was reporting a warning that the memory allocated > here was still reachable, the following change got rid of that warning > for me. Such tools are not perfect, and this is a false positive. Use them as tools, don't blindly break the code by making them shut up in some random way. This is not a memory leak. Suppose a program, for example makewhatis(8), is running over a large number of mdoc(7) manuals. For each correctly formatted manual, this function post_os() will be called once. If the .Os macro has an argument or mandoc(1) was called with -Ios, that argument will be used and the code you propose to change will not be reached. If -Ios is not used, then for the first manual where the .Os macro does not have an argument, uname(3) will be called and the result will be formatted and saved in defbuf. A copy will be saved in that manual's mdoc structure. That copy will be free(3)d when done with that particular manual. For any other manuals having .Os macros without arguments during that run of the program, the string in defbuf will be copied again, such that uname(3) doesn't need to be called again. The earliest time when defbuf could possibly be freed is after completing processing of the last manual. But at that point, that's pointless because the program will exit anyway. Consider defbuf as a constant string of unspecified length. Yours, Ingo > Index: dist/mdoc_validate.c > =================================================================== > RCS file: /cvsroot/src/external/bsd/mdocml/dist/mdoc_validate.c,v > retrieving revision 1.9 > diff -u -r1.9 mdoc_validate.c > --- dist/mdoc_validate.c 7 Jan 2016 20:05:41 -0000 1.9 > +++ dist/mdoc_validate.c 3 Apr 2016 11:51:09 -0000 > @@ -2289,6 +2289,10 @@ > utsname.sysname, utsname.release); > } > mdoc->meta.os = mandoc_strdup(defbuf); > + if (NULL != defbuf) { > + free(defbuf); > + defbuf = NULL; > + } > #endif /*!OSNAME*/ > > out: -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv