* Possible Memory leak in mdoc_validate.c? @ 2016-04-07 17:38 Abhinav Upadhyay 2016-04-07 18:10 ` Ingo Schwarze 0 siblings, 1 reply; 3+ messages in thread From: Abhinav Upadhyay @ 2016-04-07 17:38 UTC (permalink / raw) To: tech Hi, 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? 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. 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Possible Memory leak in mdoc_validate.c? 2016-04-07 17:38 Possible Memory leak in mdoc_validate.c? Abhinav Upadhyay @ 2016-04-07 18:10 ` Ingo Schwarze 2016-04-07 19:21 ` Abhinav Upadhyay 0 siblings, 1 reply; 3+ messages in thread From: Ingo Schwarze @ 2016-04-07 18:10 UTC (permalink / raw) To: Abhinav Upadhyay; +Cc: tech 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Possible Memory leak in mdoc_validate.c? 2016-04-07 18:10 ` Ingo Schwarze @ 2016-04-07 19:21 ` Abhinav Upadhyay 0 siblings, 0 replies; 3+ messages in thread From: Abhinav Upadhyay @ 2016-04-07 19:21 UTC (permalink / raw) To: Ingo Schwarze; +Cc: tech On Thu, Apr 7, 2016 at 11:40 PM, Ingo Schwarze <schwarze@usta.de> wrote: > 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. > Hi Ingo, Thanks for the explanation. That makes perfect sense :) -- Abhinav -- To unsubscribe send an email to tech+unsubscribe@mdocml.bsd.lv ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-07 19:21 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-07 17:38 Possible Memory leak in mdoc_validate.c? Abhinav Upadhyay 2016-04-07 18:10 ` Ingo Schwarze 2016-04-07 19:21 ` Abhinav Upadhyay
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).