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

[Lucas De Marchi]

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. I think unwind with goto is pretty common
idiom in kernel and igt, so I think it's acceptable too. If I move the
open(), then I may have to free(buf) in 3 possible cases (fail on
vsnprintf, fail on open, normal case).

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. Nor moving the open() is looking good if you also consider
free'ing buf. So, let's keep it as is in this patch?

Lucas De Marchi

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