[PATCH i-g-t v4 2/4] lib/igt_sysfs: stop leaking fd on write failures

[Janusz Krzysztofik]

On Thursday, 29 February 2024 18:33:46 CET Lucas De Marchi wrote:
> On Thu, Feb 29, 2024 at 12:06:24PM +0100, Janusz Krzysztofik wrote:
> >Hi Lucas,
> >
> >On Wednesday, 28 February 2024 23:31:32 CET Lucas De Marchi wrote:
> >> Make sure to close the fd before returning.
> >>
> >> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> >> ---
> >>  lib/igt_sysfs.c | 21 ++++++++++++++-------
> >>  1 file changed, 14 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> >> index 2b0225138..2997925e5 100644
> >> --- a/lib/igt_sysfs.c
> >> +++ b/lib/igt_sysfs.c
> >> @@ -515,29 +515,36 @@ int igt_sysfs_vprintf(int dir, const char *attr, 
const char *fmt, va_list ap)
> >>  	va_copy(tmp, ap);
> >>  	ret = vsnprintf(stack, sizeof(stack), fmt, tmp);
> >>  	va_end(tmp);
> >> -	if (igt_debug_on(ret < 0))
> >> -		return -EINVAL;
> >> +	if (igt_debug_on(ret < 0)) {
> >> +		ret = -EINVAL;
> >> +		goto end;
> >> +	}
> >>
> >>  	if (ret > sizeof(stack)) {
> >>  		unsigned int len = ret + 1;
> >>
> >>  		buf = malloc(len);
> >> -		if (igt_debug_on(!buf))
> >> -			return -ENOMEM;
> >> +		if (igt_debug_on(!buf)) {
> >> +			ret = -ENOMEM;
> >> +			goto end;
> >> +		}
> >>
> >>  		ret = vsnprintf(buf, ret, fmt, ap);
> >>  		if (igt_debug_on(ret > len)) {
> >> -			free(buf);
> >> -			return -EINVAL;
> >> +			ret = -EINVAL;
> >> +			goto free_buf;
> >>  		}
> >>  	}
> >>
> >>  	ret = igt_writen(fd, buf, ret);
> >>
> >> -	close(fd);
> >
> >How about moving the code that opens the fd from the top of the body of 
this
> >function to just in front of the call to igt_writen(), and keeping this
> >close() here?  That way we wouldn't need the end: section of the error 
unwind
> >path, nor those two jumps to it.
> 
> The caveat would be trying to print the string before knowing if the
> file can really be written to. 

Why the caveat?

Do you agree that opening a file is more expensive then allocating a buffer 
and printing to it?  Do you agree that, from the POV of code efficiency, we 
might want to start from less expensive steps?

> I think unwind with goto is pretty common
> idiom in kernel and igt, so I think it's acceptable too. 

No doubts.

> If I move the
> open(), then I may have to free(buf) in 3 possible cases (fail on
> vsnprintf, fail on open, normal case).

Since you based your justification on counting the cases, why haven't you also 
compared the numbers of paths leading to close(fd) for completeness of the 
picture?

> 
> The other option (that I prefer, but is not common in igt) would be to use
> __attribute__((cleanup(closep))) which would automatically close the
> file on any function return. For dealing with heap-allocated + stack in
> kmod I use a scratchbuf struct to keep track of when it's needed to free
> the buf.  But I'm not sure its worth bringing those to igt for this one
> user. 

Definitely not, I believe, if only for one user.

> Nor moving the open() is looking good if you also consider
> free'ing buf. 

With close(fd) silently not considered as a factor that matters as much as 
free(buf), what looks good to you may be only your half of the picture.

> So, let's keep it as is in this patch?

OK, if you prefer.

Acked-by: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>

Thanks,
Janusz

> 
> Lucas De Marchi
> 
> >
> >Thanks,
> >Janusz
> >
> >
> >> +free_buf:
> >>  	if (buf != stack)
> >>  		free(buf);
> >>
> >> +end:
> >> +	close(fd);
> >> +
> >>  	return ret;
> >>  }
> >>
> >>
> >
> >
> >
> >
>