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

[Kirill Tkhai]

On 12.10.2016 18:45, Kir Kolyshkin wrote:
> 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.

As I see, the most places in ./criu directory use kernel style. The only exceptions are in ./criu/arch/ppc64.

My opinion, the only style is better, and I'd prefer the kernel style.

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