Re: [Man-db-devel] [PATCH] Fix several resource and memory leaks

[Colin Watson]

On Thu, Nov 01, 2018 at 12:07:39PM +0100, Nikola Forró wrote:
> Signed-off-by: Nikola Forró <address@hidden>

Thanks for this.  Looks mostly fine except:

> diff --git a/lib/decompress.c b/lib/decompress.c
> index 02b3ec05..ae364843 100644
> --- a/lib/decompress.c
> +++ b/lib/decompress.c
> @@ -49,10 +49,14 @@
>  static void decompress_zlib (void *data ATTRIBUTE_UNUSED)
>  {
>       gzFile zlibfile;
> +     int fd;
>  
> -     zlibfile = gzdopen (dup (STDIN_FILENO), "r");
> -     if (!zlibfile)
> +     zlibfile = gzdopen (fd = dup (STDIN_FILENO), "r");
> +     if (!zlibfile) {
> +             if (fd >= 0)
> +                     close (fd);

My first reaction is "why would fd be < 0 just because gzdopen failed?",
though of course this is because dup might fail.  This indicates that
the error checking is being done in the wrong order (a negative fd might
be passed to gzdopen).  Could you make this more like this?

  fd = dup (STDIN_FILENO);
  if (fd < 0)
          return;
  zlibfile = gzdopen (fd, "r");
  if (!zlibfile)
          return;

> diff --git a/src/man.c b/src/man.c
> index 5eacab69..ba5771f0 100644
> --- a/src/man.c
> +++ b/src/man.c
> @@ -1458,6 +1458,9 @@ static pipeline *make_roff_command (const char *dir, 
> const char *file,
>               pipeline_command (p, cmd);
>       }
>  
> +     if (fmt_prog)
> +             free (fmt_prog);
> +

free (NULL) is specified to be a no-op, and man-db assumes that in many
other places.  Please drop the if guard.

> diff --git a/src/mandb.c b/src/mandb.c
> index 78f6d1a6..81f35a0d 100644
> --- a/src/mandb.c
> +++ b/src/mandb.c
> @@ -574,8 +574,10 @@ static int process_manpath (const char *manpath, int 
> global_manpath,
>       tried->seen = 0;
>       hashtable_install (tried_catdirs, catpath, strlen (catpath), tried);
>  
> -     if (stat (manpath, &st) < 0 || !S_ISDIR (st.st_mode))
> +     if (stat (manpath, &st) < 0 || !S_ISDIR (st.st_mode)) {
> +             free (catpath);
>               return 0;
> +     }

This function already has a "goto out;" cleanup path which, among other
things, frees catpath.  Could you make this error case use that instead?
You'll need to initialise dbpaths to NULL and put an "if (dbpaths)"
guard around the dbpaths-related cleanups.

Thanks,

-- 
Colin Watson                                       address@hidden