Re: [LTP] [PATCH v1] lib/safe_file_ops.c: Fix resource leak

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> safe_file_scanf and safe_file_vprintf suffered
> from resource leak, as opened file descriptor
> was not closed in case of error.
>
> Signed-off-by: Bogdan Lezhepekov <bogdan.lezhepekov@suse.com>
> ---
>  lib/safe_file_ops.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/safe_file_ops.c b/lib/safe_file_ops.c
> index f803691d8..d7231df4d 100644
> --- a/lib/safe_file_ops.c
> +++ b/lib/safe_file_ops.c
> @@ -130,7 +130,7 @@ void safe_file_scanf(const char *file, const int lineno,
>         if (f == NULL) {
>                 tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
>                         "Failed to open FILE '%s' for reading", path);
> -               return;
> +               goto out;

This one is wrong, we just tested that f == NULL

>         }
>  
>         exp_convs = tst_count_scanf_conversions(fmt);
> @@ -142,14 +142,14 @@ void safe_file_scanf(const char *file, const int lineno,
>         if (ret == EOF) {
>                 tst_brkm_(file, lineno, TBROK, cleanup_fn,
>                         "The FILE '%s' ended prematurely", path);
> -               return;

Technically this tst_brkm_() call does not return unless we are in the
test cleanup. During the rest of the test execution it ends up calling
exit(TBROK). So this return is reached only if something fails during
the test cleanup. I guess that we can fix it like this, but the test
will exit shortly after anyways.

> +               goto out;
>         }
>  
>         if (ret != exp_convs) {
>                 tst_brkm_(file, lineno, TBROK, cleanup_fn,
>                         "Expected %i conversions got %i FILE '%s'",
>                         exp_convs, ret, path);
> -               return;
> +               goto out;
>         }
>  
>         if (fclose(f)) {
> @@ -157,6 +157,8 @@ void safe_file_scanf(const char *file, const int lineno,
>                         "Failed to close FILE '%s'", path);
>                 return;
>         }
> +out:
> +       fclose(f);
>  }
>  
>  
> @@ -261,13 +263,13 @@ static void safe_file_vprintf(const char *file, const int lineno,
>         if (f == NULL) {
>                 tst_brkm_(file, lineno, TBROK | TERRNO, cleanup_fn,
>                         "Failed to open FILE '%s' for writing", path);
> -               return;
> +               goto out;

And here as well.

>         }
>  
>         if (vfprintf(f, fmt, va) < 0) {
>                 tst_brkm_(file, lineno, TBROK, cleanup_fn,
>                         "Failed to print to FILE '%s'", path);
> -               return;
> +               goto out;
>         }
>  
>         if (fclose(f)) {
> @@ -275,6 +277,8 @@ static void safe_file_vprintf(const char *file, const int lineno,
>                         "Failed to close FILE '%s'", path);
>                 return;
>         }
> +out:
> +       fclose(f);
>  }
>  
>  void safe_file_printf(const char *file, const int lineno,
> -- 
> 2.35.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp