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

[Kirill Tkhai]

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

>  		/* :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)
>