linux-ext4 - Re: [PATCH v2 04/12] ss_add_info_dir: fix memory leak and check whether

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]

Message-ID: <YPD8sGirl9D/3CyL@mit.edu>
Date:   Thu, 15 Jul 2021 23:27:44 -0400
From:   "Theodore Y. Ts'o" <tytso@....edu>
To:     wuguanghao <wuguanghao3@...wei.com>
Cc:     linux-ext4@...r.kernel.org, artem.blagodarenko@...il.com,
        liuzhiqiang26@...wei.com, linfeilong@...wei.com
Subject: Re: [PATCH v2 04/12] ss_add_info_dir: fix memory leak and check
 whether

On Wed, Jun 30, 2021 at 04:27:16PM +0800, wuguanghao wrote:
> In ss_add_info_dir(), need free info->info_dirs before return,
> otherwise it will cause memory leak. At the same time, it is necessary
> to check whether dirs[n_dirs] is a null pointer, otherwise a segmentation
> fault will occur.
> 
> Signed-off-by: Wu Guanghao <wuguanghao3@...wei.com>
> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@...wei.com>
> Reviewed-by: Wu Bo <wubo40@...wei.com>
> ---
>  lib/ss/help.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/lib/ss/help.c b/lib/ss/help.c
> index 5204401b..6768b9b1 100644
> --- a/lib/ss/help.c
> +++ b/lib/ss/help.c
> @@ -148,6 +148,7 @@ void ss_add_info_dir(int sci_idx, char *info_dir, int *code_ptr)
>      dirs = (char **)realloc((char *)dirs,
>  			    (unsigned)(n_dirs + 2)*sizeof(char *));
>      if (dirs == (char **)NULL) {
> +	free(info->info_dirs);
>  	info->info_dirs = (char **)NULL;
>  	*code_ptr = errno;
>  	return;

Adding the free() isn't right fix.  The real problem is that this line
should be removed:

  	info->info_dirs = (char **)NULL;

The function is trying to add a string (a directory name) to list, and
the realloc() is trying to extend the list.  If the realloc fils, we
shouldn't be zapping the original list.  We should just be returning,
and leaving the original list of directories unchanged and untouched.

    	    		      	 	     - Ted