[PATCH] tools/gpio: prevent resource leak

[PATCH] tools/gpio: prevent resource leak

[heminhong]

In the main() function, the open() function is used to open the file.
When the file is successfully opened, fd is used to interact with the file,
but the fd is not closed, it will cause resource leak.

Signed-off-by: heminhong <heminhong@kylinos.cn>
---
 tools/gpio/gpio-watch.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/gpio/gpio-watch.c b/tools/gpio/gpio-watch.c
index 41e76d244192..162c2a8f07c8 100644
--- a/tools/gpio/gpio-watch.c
+++ b/tools/gpio/gpio-watch.c
@@ -42,11 +42,14 @@ int main(int argc, char **argv)
 		memset(&req, 0, sizeof(req));
 
 		req.offset = strtoul(argv[j], &end, 0);
-		if (*end != '\0')
+		if (*end != '\0') {
+			close(fd);
 			goto err_usage;
+		}
 
 		ret = ioctl(fd, GPIO_V2_GET_LINEINFO_WATCH_IOCTL, &req);
 		if (ret) {
+			close(fd);
 			perror("unable to set up line watch");
 			return EXIT_FAILURE;
 		}
@@ -58,6 +61,7 @@ int main(int argc, char **argv)
 	for (;;) {
 		ret = poll(&pfd, 1, 5000);
 		if (ret < 0) {
+			close(pfd.fd);
 			perror("error polling the linechanged fd");
 			return EXIT_FAILURE;
 		} else if (ret > 0) {
@@ -66,7 +70,7 @@ int main(int argc, char **argv)
 			if (rd < 0 || rd != sizeof(chg)) {
 				if (rd != sizeof(chg))
 					errno = EIO;
-
+				close(pfd.fd);
 				perror("error reading line change event");
 				return EXIT_FAILURE;
 			}
@@ -82,6 +86,7 @@ int main(int argc, char **argv)
 				event = "config changed";
 				break;
 			default:
+				close(pfd.fd);
 				fprintf(stderr,
 					"invalid event type received from the kernel\n");
 				return EXIT_FAILURE;
@@ -91,7 +96,7 @@ int main(int argc, char **argv)
 			       chg.info.offset, event, (uint64_t)chg.timestamp_ns);
 		}
 	}
-
+	close(pfd.fd);
 	return 0;
 
 err_usage:
-- 
2.25.1

Re: [PATCH] tools/gpio: prevent resource leak

[Kent Gibson]

On Thu, Nov 09, 2023 at 04:54:19PM +0800, heminhong wrote:
> In the main() function, the open() function is used to open the file.
> When the file is successfully opened, fd is used to interact with the file,
> but the fd is not closed, it will cause resource leak.
>

All open files are automatically closed when a process exits.
That includes both those returned by open() and by the GPIO ioctls.
So explicitly closing them here before exiting is redundant.

Cheers,
Kent.

Re: [PATCH] tools/gpio: prevent resource leak

[Linus Walleij]

On Thu, Nov 9, 2023 at 1:36 PM Kent Gibson <warthog618@gmail.com> wrote:
> On Thu, Nov 09, 2023 at 04:54:19PM +0800, heminhong wrote:

> > In the main() function, the open() function is used to open the file.
> > When the file is successfully opened, fd is used to interact with the file,
> > but the fd is not closed, it will cause resource leak.
>
> All open files are automatically closed when a process exits.
> That includes both those returned by open() and by the GPIO ioctls.
> So explicitly closing them here before exiting is redundant.

Yup. And this is one of the reasons why we have the character device
in the first place.

The character device gets released and all GPIOs are released
if the program crashes.

You can imagine what happens with the sysfs ABI if a bash
script crashes halfway through some complex allocating and
banging GPIOs left and right. Not good at all.

Yours,
Linus Walleij

Re: [PATCH] tools/gpio: prevent resource leak

[Andy Shevchenko]

On Thu, Nov 9, 2023 at 2:36 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Nov 09, 2023 at 04:54:19PM +0800, heminhong wrote:
> > In the main() function, the open() function is used to open the file.
> > When the file is successfully opened, fd is used to interact with the file,
> > but the fd is not closed, it will cause resource leak.
> >
>
> All open files are automatically closed when a process exits.
> That includes both those returned by open() and by the GPIO ioctls.
> So explicitly closing them here before exiting is redundant.

I would argue that this is a good practice for GPIO cases.
More the GPIOs we have, the more line handles we can get, then default
MAX open FD limit may occur. The best is to combine both.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] tools/gpio: prevent resource leak

[Kent Gibson]

On Thu, Nov 09, 2023 at 03:53:45PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 9, 2023 at 2:36 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > On Thu, Nov 09, 2023 at 04:54:19PM +0800, heminhong wrote:
> > > In the main() function, the open() function is used to open the file.
> > > When the file is successfully opened, fd is used to interact with the file,
> > > but the fd is not closed, it will cause resource leak.
> > >
> >
> > All open files are automatically closed when a process exits.
> > That includes both those returned by open() and by the GPIO ioctls.
> > So explicitly closing them here before exiting is redundant.
>
> I would argue that this is a good practice for GPIO cases.
> More the GPIOs we have, the more line handles we can get, then default
> MAX open FD limit may occur. The best is to combine both.
>

That makes sense if the application is long lived and is continually
requesting and releasing resources, but that is not the case here - this
is a short lived app that makes a single request.
Note the "here" in my initial reply.

Cheers,
Kent.