Re: [PATCH ndctl 1/2] ndctl/keys.c: don't leak fd in error cases

["Verma, Vishal L" <vishal.l.verma@intel.com>]

On Fri, 2024-05-03 at 16:54 -0400, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that fd is leaked in some cases.  The
> change to the while loop is optional.  I only did that to make the
> code consistent.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
>  ndctl/keys.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ndctl/keys.c b/ndctl/keys.c
> index 2c1f474..cc55204 100644
> --- a/ndctl/keys.c
> +++ b/ndctl/keys.c
> @@ -108,7 +108,7 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  	struct stat st;
>  	ssize_t read_bytes = 0;
>  	int rc, fd;
> -	char *blob, *pl, *rdptr;
> +	char *blob = NULL, *pl, *rdptr;
>  	char prefix[] = "load ";
>  	bool need_prefix = false;
>  
> @@ -125,16 +125,16 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  	rc = fstat(fd, &st);
>  	if (rc < 0) {
>  		fprintf(stderr, "stat: %s\n", strerror(errno));
> -		return NULL;
> +		goto out_close;
>  	}
>  	if ((st.st_mode & S_IFMT) != S_IFREG) {
>  		fprintf(stderr, "%s not a regular file\n", path);
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	if (st.st_size == 0 || st.st_size > 4096) {
>  		fprintf(stderr, "Invalid blob file size\n");
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	*size = st.st_size;
> @@ -166,15 +166,13 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  			fprintf(stderr, "Failed to read from blob
> file: %s\n",
>  					strerror(errno));
>  			free(blob);
> -			close(fd);
> -			return NULL;
> +			blob = NULL;
> +			goto out_close;
>  		}
>  		read_bytes += rc;
>  		rdptr += rc;
>  	} while (read_bytes != st.st_size);
>  
> -	close(fd);
> -
>  	if (postfix) {
>  		pl += read_bytes;
>  		*pl = ' ';
> @@ -182,6 +180,8 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  		rc = sprintf(pl, "keyhandle=%s", postfix);
>  	}
>  
> +out_close:
> +	close(fd);
>  	return blob;
>  }
>