linux-kernel - Re: [PATCH v2] tools/bootconfig: fix resource leak in apply

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: <20200508093059.GF9365@kadam>
Date:   Fri, 8 May 2020 12:30:59 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Yunfeng Ye <yeyunfeng@...wei.com>
Cc:     mhiramat@...nel.org, rostedt@...dmis.org,
        linux-kernel@...r.kernel.org, Markus.Elfring@....de,
        kernel-janitors@...r.kernel.org, Shiyuan Hu <hushiyuan@...wei.com>,
        Hewenliang <hewenliang4@...wei.com>
Subject: Re: [PATCH v2] tools/bootconfig: fix resource leak in apply_xbc()

On Fri, May 08, 2020 at 02:51:15PM +0800, Yunfeng Ye wrote:
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 16b9a420e6fd..d034f86022b7 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -314,31 +314,33 @@ int apply_xbc(const char *path, const char *xbc_path)
>  	ret = delete_xbc(path);
>  	if (ret < 0) {
>  		pr_err("Failed to delete previous boot config: %d\n", ret);
> -		return ret;
> +		goto free_data;
>  	}
> 
>  	/* Apply new one */
>  	fd = open(path, O_RDWR | O_APPEND);
>  	if (fd < 0) {
>  		pr_err("Failed to open %s: %d\n", path, fd);
> -		return fd;
> +		ret = fd;
> +		goto free_data;
>  	}
>  	/* TODO: Ensure the @path is initramfs/initrd image */
>  	ret = write(fd, data, size + 8);
>  	if (ret < 0) {
>  		pr_err("Failed to apply a boot config: %d\n", ret);
> -		return ret;
> +		goto close_fd;
>  	}
>  	/* Write a magic word of the bootconfig */
>  	ret = write(fd, BOOTCONFIG_MAGIC, BOOTCONFIG_MAGIC_LEN);

write returns the number of bytes written on success

> -	if (ret < 0) {
> +	if (ret < 0)
>  		pr_err("Failed to apply a boot config magic: %d\n", ret);
> -		return ret;
> -	}
> +
> +close_fd:
>  	close(fd);
> +free_data:
>  	free(data);
> 
> -	return 0;
> +	return ret;

But we want to return zero on success.

>  }

Btw, these leaks are totally harmless.  This is a short running user
space program with is going to immediately exit on error so the memory
will be freed anyway.  But the benifit is to silence static checker
warnings so that's useful.

regards,
dan carpenter