[PATCH] nm: Fix descriptor leak

[PATCH] nm: Fix descriptor leak

[Maks Mishin]

The descriptor 'dwfl_fd' is created at nm.c:1278 by calling
function 'dup' and lost at nm.c:1593.

Found by RASU JSC.

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 src/nm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/nm.c b/src/nm.c
index 3675f59b..fee397dd 100644
--- a/src/nm.c
+++ b/src/nm.c
@@ -1521,6 +1521,8 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
     }
   if (dwfl != NULL)
     dwfl_end (dwfl);
+  if (dwfl_fd != NULL)
+    close(dwfl_fd);
 }
 
 
-- 
2.30.2

Re: [PATCH] nm: Fix descriptor leak

[Mark Wielaard]

Hi,

On Thu, Mar 28, 2024 at 11:49:58PM +0300, Maks Mishin wrote:
> The descriptor 'dwfl_fd' is created at nm.c:1278 by calling
> function 'dup' and lost at nm.c:1593.

Sorry, I don't follow, the code at nm.c:1278 says:

          /* Duplicate an fd for dwfl_report_offline to swallow.  */
          int dwfl_fd = dup (fd);
          if (likely (dwfl_fd >= 0))

And then the code tracks whether the dwfl_report_offline call is
executed successfully and otherwise closed dwfl_fd. Specifically the
code does:

                  if (dwfl_report_offline (dwfl, fname, fname, dwfl_fd)
                      == NULL)
                    {
                      /* Consumed on success, not on failure.  */
                      close (dwfl_fd);
                    }

The dwfl_report functions are documented as "On success, FD is
consumed by the library". Which means fd is freed/closed when dwfl_end
is called.

> Found by RASU JSC.
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
> ---
>  src/nm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/nm.c b/src/nm.c
> index 3675f59b..fee397dd 100644
> --- a/src/nm.c
> +++ b/src/nm.c
> @@ -1521,6 +1521,8 @@ show_symbols (int fd, Ebl *ebl, GElf_Ehdr *ehdr,
>      }
>    if (dwfl != NULL)
>      dwfl_end (dwfl);
> +  if (dwfl_fd != NULL)
> +    close(dwfl_fd);
>  }

So this would double close dwfl_fd.  Also dwfl_fd is an int (file
descriptor) and so shouldn't be compared to NULL, but checked as above
(dwfl_fd >= 0).