netdev - [PATCH iproute2 01/11] lib/fs: fix file leak in task_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue,  9 May 2023 14:21:15 -0700
From: Stephen Hemminger <stephen@...workplumber.org>
To: netdev@...r.kernel.org
Cc: Stephen Hemminger <stephen@...workplumber.org>
Subject: [PATCH iproute2 01/11] lib/fs: fix file leak in task_get_name

Fixes the problem identified -fanalyzer.
Why did rdma choose to reimplement the same function as
exiting glibc pthread_getname().

fs.c: In function ‘get_task_name’:
fs.c:355:12: warning: leak of FILE ‘f’ [CWE-775] [-Wanalyzer-file-leak]
  355 |         if (!fgets(name, len, f))
      |            ^
  ‘get_task_name’: events 1-9
    |
    |  345 |         if (!pid)
    |      |            ^
    |      |            |
    |      |            (1) following ‘false’ branch (when ‘pid != 0’)...
    |......
    |  348 |         if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path))
    |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |            ||
    |      |            |(2) ...to here
    |      |            (3) following ‘false’ branch...
    |......
    |  351 |         f = fopen(path, "r");
    |      |             ~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (4) ...to here
    |      |             (5) opened here
    |  352 |         if (!f)
    |      |            ~
    |      |            |
    |      |            (6) assuming ‘f’ is non-NULL
    |      |            (7) following ‘false’ branch (when ‘f’ is non-NULL)...
    |......
    |  355 |         if (!fgets(name, len, f))
    |      |            ~ ~~~~~~~~~~~~~~~~~~~
    |      |            | |
    |      |            | (8) ...to here
    |      |            (9) following ‘true’ branch...
    |
  ‘get_task_name’: event 10
    |
    |cc1:
    | (10): ...to here
    |
  ‘get_task_name’: event 11
    |
    |  355 |         if (!fgets(name, len, f))
    |      |            ^
    |      |            |
    |      |            (11) ‘f’ leaks here; was opened at (5)
    |
fs.c:355:12: warning: leak of ‘f’ [CWE-401] [-Wanalyzer-malloc-leak]
  ‘get_task_name’: events 1-9
    |
    |  345 |         if (!pid)
    |      |            ^
    |      |            |
    |      |            (1) following ‘false’ branch (when ‘pid != 0’)...
    |......
    |  348 |         if (snprintf(path, sizeof(path), "/proc/%d/comm", pid) >= sizeof(path))
    |      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |            ||
    |      |            |(2) ...to here
    |      |            (3) following ‘false’ branch...
    |......
    |  351 |         f = fopen(path, "r");
    |      |             ~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (4) ...to here
    |      |             (5) allocated here
    |  352 |         if (!f)
    |      |            ~
    |      |            |
    |      |            (6) assuming ‘f’ is non-NULL
    |      |            (7) following ‘false’ branch (when ‘f’ is non-NULL)...
    |......
    |  355 |         if (!fgets(name, len, f))
    |      |            ~ ~~~~~~~~~~~~~~~~~~~
    |      |            | |
    |      |            | (8) ...to here
    |      |            (9) following ‘true’ branch...
    |
  ‘get_task_name’: event 10
    |
    |cc1:
    | (10): ...to here
    |
  ‘get_task_name’: event 11
    |
    |  355 |         if (!fgets(name, len, f))
    |      |            ^
    |      |            |
    |      |            (11) ‘f’ leaks here; was allocated at (5)

Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma")
Signed-off-by: Stephen Hemminger <stephen@...workplumber.org>
---
 lib/fs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/fs.c b/lib/fs.c
index 22d4af7583dd..7f4b159ccb65 100644
--- a/lib/fs.c
+++ b/lib/fs.c
@@ -352,8 +352,10 @@ int get_task_name(pid_t pid, char *name, size_t len)
 	if (!f)
 		return -1;
 
-	if (!fgets(name, len, f))
+	if (!fgets(name, len, f)) {
+		fclose(f);
 		return -1;
+	}
 
 	/* comm ends in \n, get rid of it */
 	name[strcspn(name, "\n")] = '\0';
-- 
2.39.2