2023-04-17 07:59:13

by Hao Zeng

Subject: [PATCH v3] cpupower:Fix resource leaks in sysfs_get_enabled()

The sysfs_get_enabled() opened file processor not closed,
may cause a file handle leak.
Putting error handling and resource cleanup code together
makes the code easy to maintain and read.
Removed the unnecessary else if branch from the original
function, as it should return an error in cases other than '0'.

Signed-off-by: Hao Zeng <[email protected]>
Suggested-by: Shuah Khan <[email protected]>
---
tools/power/cpupower/lib/powercap.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
index 0ce29ee4c2e4..f0334a5f1acf 100644
--- a/tools/power/cpupower/lib/powercap.c
+++ b/tools/power/cpupower/lib/powercap.c
@@ -40,25 +40,31 @@ static int sysfs_get_enabled(char *path, int *mode)
{
int fd;
char yes_no;
+ int ret = 0;

*mode = 0;

fd = open(path, O_RDONLY);
- if (fd == -1)
- return -1;
+ if (fd == -1) {
+ ret = -1;
+ goto out;
+ }

if (read(fd, &yes_no, 1) != 1) {
- close(fd);
- return -1;
+ ret = -1;
+ goto out_close;
}

if (yes_no == '1') {
*mode = 1;
- return 0;
- } else if (yes_no == '0') {
- return 0;
+ } else if (yes_no != '0') {
+ ret = -1;
+ goto out_close;
}
- return -1;
+out_close:
+ close(fd);
+out:
+ return ret;
}

int powercap_get_enabled(int *mode)
--
2.37.2

2023-04-17 21:34:32

by Shuah Khan

Subject: Re: [PATCH v3] cpupower:Fix resource leaks in sysfs_get_enabled()

On 4/17/23 01:56, Hao Zeng wrote:
> The sysfs_get_enabled() opened file processor not closed,
> may cause a file handle leak.
> Putting error handling and resource cleanup code together
> makes the code easy to maintain and read.
> Removed the unnecessary else if branch from the original
> function, as it should return an error in cases other than '0'.
>
> Signed-off-by: Hao Zeng <[email protected]>
> Suggested-by: Shuah Khan <[email protected]>
> ---
> tools/power/cpupower/lib/powercap.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/power/cpupower/lib/powercap.c b/tools/power/cpupower/lib/powercap.c
> index 0ce29ee4c2e4..f0334a5f1acf 100644
> --- a/tools/power/cpupower/lib/powercap.c
> +++ b/tools/power/cpupower/lib/powercap.c
> @@ -40,25 +40,31 @@ static int sysfs_get_enabled(char *path, int *mode)
> {
> int fd;
> char yes_no;
> + int ret = 0;
>
> *mode = 0;
>
> fd = open(path, O_RDONLY);
> - if (fd == -1)
> - return -1;
> + if (fd == -1) {
> + ret = -1;
> + goto out;
> + }
>
> if (read(fd, &yes_no, 1) != 1) {
> - close(fd);
> - return -1;
> + ret = -1;
> + goto out_close;
> }
>
> if (yes_no == '1') {
> *mode = 1;
> - return 0;

You can just add goto out_close here

> - } else if (yes_no == '0') {
> - return 0;

Keep == '0' check and add goto out_close here
> + } else if (yes_no != '0') {

This can be just else with the above change.

> + ret = -1;
> + goto out_close;
> }
> - return -1;
> +out_close:
> + close(fd);
> +out:
> + return ret;
> }
>
> int powercap_get_enabled(int *mode)

thanks,
-- Shuah

2023-04-18 02:33:26

by Hao Zeng

Subject: Re: [PATCH v3] cpupower:Fix resource leaks in sysfs_get_enabled()

Dear Shuah:

Thank you again for your patience and guidance. This has benefited me a
lot. I will apply what I have learned this time to my work and kernel
contribution

Best regards
-- Hao

On Mon, 2023-04-17 at 15:09 -0600, Shuah Khan wrote:
> On 4/17/23 01:56, Hao Zeng wrote:
> > The sysfs_get_enabled() opened file processor not closed,
> > may cause a file handle leak.
> > Putting error handling and resource cleanup code together
> > makes the code easy to maintain and read.
> > Removed the unnecessary else if branch from the original
> > function, as it should return an error in cases other than '0'.
> >
> > Signed-off-by: Hao Zeng <[email protected]>
> > Suggested-by: Shuah Khan <[email protected]>
> > ---
> >   tools/power/cpupower/lib/powercap.c | 22 ++++++++++++++--------
> >   1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/power/cpupower/lib/powercap.c
> > b/tools/power/cpupower/lib/powercap.c
> > index 0ce29ee4c2e4..f0334a5f1acf 100644
> > --- a/tools/power/cpupower/lib/powercap.c
> > +++ b/tools/power/cpupower/lib/powercap.c
> > @@ -40,25 +40,31 @@ static int sysfs_get_enabled(char *path, int
> > *mode)
> >   {
> >         int fd;
> >         char yes_no;
> > +       int ret = 0;
> >  
> >         *mode = 0;
> >  
> >         fd = open(path, O_RDONLY);
> > -       if (fd == -1)
> > -               return -1;
> > +       if (fd == -1) {
> > +               ret = -1;
> > +               goto out;
> > +       }
> >  
> >         if (read(fd, &yes_no, 1) != 1) {
> > -               close(fd);
> > -               return -1;
> > +               ret = -1;
> > +               goto out_close;
> >         }
> >  
> >         if (yes_no == '1') {
> >                 *mode = 1;
> > -               return 0;
>
> You can just add goto out_close here
>
> > -       } else if (yes_no == '0') {
> > -               return 0;
>
> Keep == '0' check and add goto out_close here
> > +       } else if (yes_no != '0') {
>
> This can be just else with the above change.
>
> > +               ret = -1;
> > +               goto out_close;
> >         }
> > -       return -1;
> > +out_close:
> > +       close(fd);
> > +out:
> > +       return ret;
> >   }
> >  
> >   int powercap_get_enabled(int *mode)
>
> thanks,
> -- Shuah