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

[Lucas De Marchi]

On Fri, Mar 01, 2024 at 04:15:22PM +0100, Janusz Krzysztofik wrote:
>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?

not necessarily... it also depends what operation may fail more
frequently: user opening the wrong file (also think about igt supporting
multiple fallbacks for alternate paths) or having a OOM situation.

>
>> 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?

because I'm not counting like "case (a) has 3, case (4) has 4 or 5 or 6
=> case (a) wins". Pointing being both cases need some unwind at the
end.

>
>>
>> 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.

because I don't really consider it as a least important one. If you keep
an fd open to a device (also including a sysfs file handled by a device),
then you may fail later in e.g. a module unload because the module will
be in use.  People then go and do `rmmod -f` which taints the kernel
(for a very good reason)

Other problem is igt not really using O_CLOEXEC very much which leads to
the fd leaking to other binaries, making the issue even worse.

>
>> 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
Lucas De Marchi

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