netdev - Re: [PATCH iproute2 v3 1/2] lib/fs: fix memory leak in get_task

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] [day] [month] [year] [list]

Message-ID: <20220308093043.7c1a131a@hermes.local>
Date:   Tue, 8 Mar 2022 09:30:43 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Andrea Claudi <aclaudi@...hat.com>
Cc:     netdev@...r.kernel.org, dsahern@...il.com, markzhang@...dia.com,
        leonro@...dia.com
Subject: Re: [PATCH iproute2 v3 1/2] lib/fs: fix memory leak in
 get_task_name()

On Tue,  8 Mar 2022 18:04:56 +0100
Andrea Claudi <aclaudi@...hat.com> wrote:

> asprintf() allocates memory which is not freed on the error path of
> get_task_name(), thus potentially leading to memory leaks.
> %m specifier on fscanf allocates memory, too, which needs to be freed by
> the caller.
> 
> This reworks get_task_name() to avoid memory allocation.
> - Pass a buffer and its lenght to the function, similarly to what
>   get_command_name() does, thus avoiding to allocate memory for
>   the string to be returned;
> - Use snprintf() instead of asprintf();
> - Use fgets() instead of fscanf() to limit string lenght.

Spelling s/lenght/length/

> 
> Fixes: 81bfd01a4c9e ("lib: move get_task_name() from rdma")
> Signed-off-by: Andrea Claudi <aclaudi@...hat.com>
> ---
>  include/utils.h |  2 +-
>  ip/iptuntap.c   | 17 ++++++++++-------
>  lib/fs.c        | 23 +++++++++++++----------
>  rdma/res-cmid.c |  8 +++++---
>  rdma/res-cq.c   |  8 +++++---
>  rdma/res-ctx.c  |  7 ++++---
>  rdma/res-mr.c   |  7 ++++---
>  rdma/res-pd.c   |  8 +++++---
>  rdma/res-qp.c   |  7 ++++---
>  rdma/res-srq.c  |  7 ++++---
>  rdma/stat.c     |  5 ++++-
>  11 files changed, 59 insertions(+), 40 deletions(-)
> 
> diff --git a/include/utils.h b/include/utils.h
> index b6c468e9..b0e0967c 100644
> --- a/include/utils.h
> +++ b/include/utils.h
> @@ -307,7 +307,7 @@ char *find_cgroup2_mount(bool do_mount);
>  __u64 get_cgroup2_id(const char *path);
>  char *get_cgroup2_path(__u64 id, bool full);
>  int get_command_name(const char *pid, char *comm, size_t len);
> -char *get_task_name(pid_t pid);
> +int get_task_name(pid_t pid, char *name, size_t len);
>  
>  int get_rtnl_link_stats_rta(struct rtnl_link_stats64 *stats64,
>  			    struct rtattr *tb[]);
> diff --git a/ip/iptuntap.c b/ip/iptuntap.c
> index 385d2bd8..35c9bf5b 100644
> --- a/ip/iptuntap.c
> +++ b/ip/iptuntap.c
> @@ -321,14 +321,17 @@ static void show_processes(const char *name)
>  			} else if (err == 2 &&
>  				   !strcmp("iff", key) &&
>  				   !strcmp(name, value)) {
> -				char *pname = get_task_name(pid);
> -
> -				print_string(PRINT_ANY, "name",
> -					     "%s", pname ? : "<NULL>");
> +				SPRINT_BUF(pname);
> +
> +				if (get_task_name(pid, pname, sizeof(pname))) {
> +					print_string(PRINT_ANY, "name",
> +						     "%s", "<NULL>");
> +				} else {
> +					print_string(PRINT_ANY, "name",
> +						     "%s", pname);
> +				}
>

Don't need brackets here. I can fix that.