[PATCH] tools: Fix memory and descriptor leak

[PATCH] tools: Fix memory and descriptor leak

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
---
 tools/zynqmpbif.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/tools/zynqmpbif.c b/tools/zynqmpbif.c
index 82ce0ac1a52..33c68eac9f3 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,24 +214,28 @@ 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) {
 		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;
-- 
2.30.2

[PATCH] scripts: Fix potential null-deref

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 scripts/kconfig/menu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 5c5c1374..a0d0d2af 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -812,8 +812,10 @@ static void get_symbol_str(struct gstr *r, struct symbol *sym,
 			}
 		}
 	}
-	for_all_prompts(sym, prop)
-		get_prompt_str(r, prop, head);
+	if (sym) {
+		for_all_prompts(sym, prop)
+			get_prompt_str(r, prop, head);
+	}
 
 	prop = get_symbol_prop(sym);
 	if (prop) {
-- 
2.34.1

[PATCH] tools: Fix memory and descriptor leak

[ant.v.moryakov]

From: Maks Mishin <maks.mishinFZ@gmail.com>

Signed-off-by: Maks Mishin <maks.mishinFZ@gmail.com>
---
 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;
+		}
 		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);
 		return NULL;
 	}
 
-- 
2.34.1

Re: [PATCH] tools: Fix memory and descriptor leak

[Quentin Schulz]

Hi,

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