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

[Quentin Schulz]

Hi,

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