[PATCH] tools: Fix memory and descriptor leak

[Quentin Schulz]

Hi,

On 4/18/25 10:19 AM, ant.v.moryakov at gmail.com wrote:
> From: Maks Mishin <maks.mishinFZ at gmail.com>
> 
> Signed-off-by: Maks Mishin <maks.mishinFZ at gmail.com>

Same remark as for the first patch in this series, are you Maks? If no, 
we need your Signed-off-by too.

> ---
>   tools/zynqmpbif.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
> index 82ce0ac1..76b7a35f 100644
> --- a/tools/zynqmpbif.c
> +++ b/tools/zynqmpbif.c
> @@ -226,8 +226,10 @@ static char *read_full_file(const char *filename, size_t *size)
>   	bufp = buf;
>   	while (len < sbuf.st_size) {
>   		r = read(fd, bufp, sbuf.st_size - len);
> -		if (r < 0)
> +		if (r < 0) {
> +			free(buf);
>   			return NULL;

Shouldn't we close the file descriptor too?

Looking at the code a bit more, it seems like we should be closing the 
file descriptor in other error code paths as well.

I would recommend to go for a goto: instead.

e.g.:

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 82ce0ac1a52..0b82f349758 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -226,12 +226,16 @@ static char *read_full_file(const char *filename, 
size_t *size)
  	bufp = buf;
  	while (len < sbuf.st_size) {
  		r = read(fd, bufp, sbuf.st_size - len);
-		if (r < 0)
-			return NULL;
+		if (r < 0) {
+			free(buf);
+			buf = NULL;
+			goto out:
+		}
  		len += r;
  		bufp += r;
  	}

+out:
  	close(fd);

  	return buf;


Then, another patch on top of that, which closes the file descriptor for 
other error code paths as well, e.g.:

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 0b82f349758..81ca9cc7844 100644
--- a/tools/zynqmpbif.c
+++ b/tools/zynqmpbif.c
@@ -205,7 +205,7 @@ static const struct bif_flags bif_flags[] = {

  static char *read_full_file(const char *filename, size_t *size)
  {
-	char *buf, *bufp;
+	char *buf = NULL, *bufp;
  	struct stat sbuf;
  	int len = 0, r, fd;

@@ -214,14 +214,14 @@ static char *read_full_file(const char *filename, 
size_t *size)
  		return NULL;

  	if (fstat(fd, &sbuf) < 0)
-		return NULL;
+		goto out;

  	if (size)
  		*size = sbuf.st_size;

  	buf = malloc(sbuf.st_size);
  	if (!buf)
-		return NULL;
+		goto out;

  	bufp = buf;
  	while (len < sbuf.st_size) {


> +		}
>   		len += r;
>   		bufp += r;
>   	}
> @@ -793,6 +795,8 @@ static const struct bif_file_type *get_file_type(struct bif_entry *entry)
>   
>   	if (read(fd, &header, sizeof(header)) != sizeof(header)) {
>   		printf("Error reading file %s", entry->filename);
> +		if (fd)
> +			close(fd);

Fixing a different issue, please in a separate patch.

It seems very wrong to me to close a file descriptor in another function 
that opened it, is this really something we want to be doing? I know we 
already are doing it, (a few lines after that, close(fd) is called), but 
should we "fix" it this way?

Cheers,
Quentin