diff mbox series

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

Message ID 20240820182705.139842-2-jmoyer@redhat.com (mailing list archive)
State Accepted
Commit 00119a66148912f2671d22e7a96d39c31832ff6a
Headers show
Series [ndctl,1/2] ndctl/keys.c: don't leak fd in error cases | expand

Commit Message

Jeff Moyer Aug. 20, 2024, 6:26 p.m. UTC 
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>
---
 ndctl/keys.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Dave Jiang Aug. 20, 2024, 7:59 p.m. UTC | #1 
On 8/20/24 11:26 AM, 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>

Reviewed-by: Dave Jiang <dave.jiang@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;
>  }
>
Verma, Vishal L Aug. 21, 2024, 9:50 p.m. UTC | #2 
On Tue, 2024-08-20 at 14:26 -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;
>  }
>
diff mbox series

Patch

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;
 }