[PATCH] selftests/mm: mlock2-tests: fix fclose(NULL) in is_vmflag_set()

[PATCH] selftests/mm: mlock2-tests: fix fclose(NULL) in is_vmflag_set()

[Hongfu Li]

From: Hongfu Li <lihongfu@kylinos.cn>

is_vmflag_set() jumps to the shared 'out' label when
seek_to_smaps_entry() fails. smaps is NULL on that path, and the
code at the 'out' label calls fclose(smaps) unconditionally.
fclose(NULL) is undefined behaviour, so the test crashes before
it can report a test result.

Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
---
 tools/testing/selftests/mm/mlock2-tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
index 144b550813a6..85e6cd24515a 100644
--- a/tools/testing/selftests/mm/mlock2-tests.c
+++ b/tools/testing/selftests/mm/mlock2-tests.c
@@ -66,7 +66,7 @@ static bool is_vmflag_set(unsigned long addr, const char *vmflag)
 	smaps = seek_to_smaps_entry(addr);
 	if (!smaps) {
 		ksft_print_msg("Unable to parse /proc/self/smaps\n");
-		goto out;
+		return false;
 	}
 
 	while (getline(&line, &size, smaps) > 0) {

---
base-commit: 74d73ffcffe03804f97e9fb82301dadf75671282
change-id: 20261104-for-mlock2-a67f86dad498

Best regards,
--  
Hongfu Li <lihongfu@kylinos.cn>

Re: [PATCH] selftests/mm: mlock2-tests: fix fclose(NULL) in is_vmflag_set()

[Liam R. Howlett]

On 26/11/04 04:49PM, Hongfu Li wrote:
> From: Hongfu Li <lihongfu@kylinos.cn>
> 
> is_vmflag_set() jumps to the shared 'out' label when
> seek_to_smaps_entry() fails. smaps is NULL on that path, and the
> code at the 'out' label calls fclose(smaps) unconditionally.
> fclose(NULL) is undefined behaviour, so the test crashes before
> it can report a test result.
> 
> Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>

The sending address should match the sign off.


> ---
>  tools/testing/selftests/mm/mlock2-tests.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
> index 144b550813a6..85e6cd24515a 100644
> --- a/tools/testing/selftests/mm/mlock2-tests.c
> +++ b/tools/testing/selftests/mm/mlock2-tests.c
> @@ -66,7 +66,7 @@ static bool is_vmflag_set(unsigned long addr, const char *vmflag)
>  	smaps = seek_to_smaps_entry(addr);
>  	if (!smaps) {
>  		ksft_print_msg("Unable to parse /proc/self/smaps\n");
> -		goto out;
> +		return false;
>  	}

This looks better.

The next function is also messy with the goto usage/ordering of
unrolling and should probably be changed in the same way.

>  
>  	while (getline(&line, &size, smaps) > 0) {
> 
> ---
> base-commit: 74d73ffcffe03804f97e9fb82301dadf75671282
> change-id: 20261104-for-mlock2-a67f86dad498
> 
> Best regards,
> --  
> Hongfu Li <lihongfu@kylinos.cn>
> 

Re: [PATCH] selftests/mm: mlock2-tests: fix fclose(NULL) in is_vmflag_set()

[Hongfu Li]

在 11/12/26 03:49, Liam R. Howlett 写道:
> On 26/11/04 04:49PM, Hongfu Li wrote:
>> From: Hongfu Li <lihongfu@kylinos.cn>
>>
>> is_vmflag_set() jumps to the shared 'out' label when
>> seek_to_smaps_entry() fails. smaps is NULL on that path, and the
>> code at the 'out' label calls fclose(smaps) unconditionally.
>> fclose(NULL) is undefined behaviour, so the test crashes before
>> it can report a test result.
>>
>> Signed-off-by: Hongfu Li <lihongfu@kylinos.cn>
> The sending address should match the sign off.

Sure, I'll send a follow-up from the same address.

>> ---
>>   tools/testing/selftests/mm/mlock2-tests.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/mm/mlock2-tests.c b/tools/testing/selftests/mm/mlock2-tests.c
>> index 144b550813a6..85e6cd24515a 100644
>> --- a/tools/testing/selftests/mm/mlock2-tests.c
>> +++ b/tools/testing/selftests/mm/mlock2-tests.c
>> @@ -66,7 +66,7 @@ static bool is_vmflag_set(unsigned long addr, const char *vmflag)
>>   	smaps = seek_to_smaps_entry(addr);
>>   	if (!smaps) {
>>   		ksft_print_msg("Unable to parse /proc/self/smaps\n");
>> -		goto out;
>> +		return false;
>>   	}
> This looks better.
>
> The next function is also messy with the goto usage/ordering of
> unrolling and should probably be changed in the same way.

Thanks for the review.

Agreed, get_value_for_name() has the same pattern. I'll handle it in
the same way in v2.

>>   
>>   	while (getline(&line, &size, smaps) > 0) {
>>
>> ---
>> base-commit: 74d73ffcffe03804f97e9fb82301dadf75671282
>> change-id: 20261104-for-mlock2-a67f86dad498
>>
>> Best regards,
>> --
>> Hongfu Li <lihongfu@kylinos.cn>
>>
-- 
Best regards,
Hongfu