diff mbox series

tools: fix handle leak in ifdtool.c

Message ID 20250516152538.75325-1-ant.v.moryakov@gmail.com
State Accepted
Delegated to: Tom Rini
Headers show
Series tools: fix handle leak in ifdtool.c | expand

Commit Message

Anton Moryakov May 16, 2025, 3:25 p.m. UTC 
From: Anton Moryakov <ant.v.moryakov@gmail.com>

Prevent file descriptor leaks by properly closing 'fd' and 'new_fd'
when fstat() or write() operations fail.

- Added close(fd) before return in open_for_read() if fstat() fails.
- Added close(new_fd) before return in write_image() if write() fails.
- No close needed if open() fails (fd == -1 is invalid).

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
---
 tools/ifdtool.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

Quentin Schulz May 19, 2025, 8:47 a.m. UTC | #1 
Hi Anton,

On 5/16/25 5:25 PM, ant.v.moryakov@gmail.com wrote:
> From: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> Prevent file descriptor leaks by properly closing 'fd' and 'new_fd'
> when fstat() or write() operations fail.
> 
> - Added close(fd) before return in open_for_read() if fstat() fails.
> - Added close(new_fd) before return in write_image() if write() fails.
> - No close needed if open() fails (fd == -1 is invalid).
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

Looks fine to me

Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>

I believe there are a few others in that file?

inject_region returns without closing, same for write_data, same for the 
main function with bios_fd at the very least.

Thanks!
Quentin
Anton Moryakov May 19, 2025, 8:51 a.m. UTC | #2 
Maybe. However, the static analyser did not swear at others


Пн, 19 мая 2025 г. в 11:47, Quentin Schulz <quentin.schulz@cherry.de>:

> Hi Anton,
>
> On 5/16/25 5:25 PM, ant.v.moryakov@gmail.com wrote:
> > From: Anton Moryakov <ant.v.moryakov@gmail.com>
> >
> > Prevent file descriptor leaks by properly closing 'fd' and 'new_fd'
> > when fstat() or write() operations fail.
> >
> > - Added close(fd) before return in open_for_read() if fstat() fails.
> > - Added close(new_fd) before return in write_image() if write() fails.
> > - No close needed if open() fails (fd == -1 is invalid).
> >
> > Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
>
> Looks fine to me
>
> Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de>
>
> I believe there are a few others in that file?
>
> inject_region returns without closing, same for write_data, same for the
> main function with bios_fd at the very least.
>
> Thanks!
> Quentin
>
Tom Rini June 3, 2025, 11:19 p.m. UTC | #3 
On Fri, 16 May 2025 18:25:38 +0300, ant.v.moryakov@gmail.com wrote:

> Prevent file descriptor leaks by properly closing 'fd' and 'new_fd'
> when fstat() or write() operations fail.
> 
> - Added close(fd) before return in open_for_read() if fstat() fails.
> - Added close(new_fd) before return in write_image() if write() fails.
> - No close needed if open() fails (fd == -1 is invalid).
> 
> [...]

Applied to u-boot/next, thanks!

[1/1] tools: fix handle leak in ifdtool.c
      commit: a4fb99ce4e6ee62f3278d7472b9b9282d7b289ed
diff mbox series

Patch

diff --git a/tools/ifdtool.c b/tools/ifdtool.c
index b70570361f4..5db6fa7d195 100644
--- a/tools/ifdtool.c
+++ b/tools/ifdtool.c
@@ -499,8 +499,10 @@  static int write_image(char *filename, char *image, int size)
 		      S_IWUSR | S_IRGRP | S_IROTH);
 	if (new_fd < 0)
 		return perror_fname("Could not open file '%s'", filename);
-	if (write(new_fd, image, size) != size)
+	if (write(new_fd, image, size) != size) {
+		close(new_fd);
 		return perror_fname("Could not write file '%s'", filename);
+	}
 	close(new_fd);
 
 	return 0;
@@ -604,8 +606,10 @@  int open_for_read(const char *fname, int *sizep)
 
 	if (fd == -1)
 		return perror_fname("Could not open file '%s'", fname);
-	if (fstat(fd, &buf) == -1)
+	if (fstat(fd, &buf) == -1) {
+		close(fd); 
 		return perror_fname("Could not stat file '%s'", fname);
+	}
 	*sizep = buf.st_size;
 	debug("File %s is %d bytes\n", fname, *sizep);