[CRIU] [PATCH 01/23] criu/filesystems.c: refactor binfmt_misc_restore_bme

[Kir Kolyshkin]

On 10/12/2016 02:14 AM, Kirill Tkhai wrote:
>
> On 12.10.2016 04:46, Kir Kolyshkin wrote:
>> The following error is emitted by clang:
>>
>>>    CC       criu/filesystems.o
>>> criu/filesystems.c:280:13: error: variable 'ret' is used uninitialized
>>>     whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>>>          } else if (bme->extension) {
>>>                     ^~~~~~~~~~~~~~
>>> criu/filesystems.c:287:6: note: uninitialized use occurs here
>>>          if (ret > 0) {
>>>              ^~~
>>> criu/filesystems.c:280:9: note: remove the 'if' if its condition is
>>>     always true
>>>          } else if (bme->extension) {
>>>                 ^~~~~~~~~~~~~~~~~~~~
>>> criu/filesystems.c:272:9: note: initialize the variable 'ret' to silence
>>>     this warning
>>>          int ret;
>>>                 ^
>>>                  = 0
>>> 1 error generated.
>> This code was a result of commit 398e7d3.
>>
>> If we look closely, this is a false alarm, as "else if (bme->extension)"
>> is always true as it was checked before. But this is not very clear,
>> and the issue with clangs still needs to be fixed.
>>
>> There are many ways to do so:
>>
>> 1. Initialize ret to 0. This is what initial version of this patch did.
>>
>> 2. Remove the always-true condition, like this:
>>
>> 	-	} else if (bme->extension) {
>> 	+	} else {
>>
>> In my opinion this would hurt readability.
>>
>> 3. Change the code flow, improving readability at the same time.
>>
>> I believe that #3 is what this patch does. In addition, it fixes
>> handling of a few corner cases:
>>
>> - an overflow in snprintf();
>> - a case when bme->name is NULL (as it is used for strlen/snprintf,
>>    there is a potential for SIGSEGV);
>> - a case of ret == 0 (currently there is no code flow that
>>    results in ret being 0, so it's just for the future).
>>
>> Cc: Kirill Tkhai <ktkhai at virtuozzo.com>
>> Signed-off-by: Kir Kolyshkin <kir at openvz.org>
>> ---
>>   criu/filesystems.c | 30 ++++++++++++++++++++----------
>>   1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/criu/filesystems.c b/criu/filesystems.c
>> index d66a6c6..cabfbf2 100644
>> --- a/criu/filesystems.c
>> +++ b/criu/filesystems.c
>> @@ -271,25 +271,35 @@ static int binfmt_misc_restore_bme(struct mount_info *mi, BinfmtMiscEntry *bme,
>>   {
>>   	int ret;
>>   
>> -	/* :name:type:offset:magic/extension:mask:interpreter:flags */
>> -	if ((!bme->magic && !bme->extension) || !bme->interpreter) {
>> -		pr_perror("binfmt_misc: bad dump");
>> -		ret = -1;
>> -	} else if (bme->magic) {
>> +	if (!bme->name || !bme->interpreter)
>> +		goto bad_dump;
>> +
>> +	/* Either magic or extension should be there */
>> +	if (bme->magic) {
>>   		ret = make_bfmtm_magic_str(buf, bme);
>> -	} else if (bme->extension) {
>> +	}
>> +	else if (bme->extension) {
> Don't we use a specific style for "} else" condition? Currently,
> the most place are:
>
> } else
>
> and not
>
> }
> else

Thanks for noting it, as I was paying attention, too. Looked around in 
the code and found that both forms are used. If the one you want is 
preferable, I am to resend the fixed version.

>
>>   		/* :name:E::extension::interpreter:flags */
>>   		ret = snprintf(buf, BINFMT_MISC_STR, ":%s:E::%s::%s:%s",
>>   			       bme->name, bme->extension, bme->interpreter,
>>   			       bme->flags ? : "\0");
>> +		if (ret >= BINFMT_MISC_STR) // output truncated
>> +			ret = -1;
>>   	}
>> +	else
>> +		ret = -1;
>>   
>> -	if (ret > 0) {
>> -		pr_debug("binfmt_misc_pattern=%s\n", buf);
>> -		ret = write_binfmt_misc_entry(mi->mountpoint, buf, bme);
>> -	}
>> +	if (ret < 0)
>> +		goto bad_dump;
>> +
>> +	pr_debug("binfmt_misc_pattern=%s\n", buf);
>> +	ret = write_binfmt_misc_entry(mi->mountpoint, buf, bme);
>>   
>>   	return ret;
>> +
>> +bad_dump:
>> +	pr_perror("binfmt_misc: bad dump");
>> +	return -1;
>>   }
>>   
>>   static int binfmt_misc_restore(struct mount_info *mi)
>>