Possible Memory leak in mdoc_validate.c?

Possible Memory leak in mdoc_validate.c?

[Abhinav Upadhyay]

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

Re: Possible Memory leak in mdoc_validate.c?

[Ingo Schwarze]

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

Re: Possible Memory leak in mdoc_validate.c?

[Abhinav Upadhyay]

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