Commit 02c3605c authored by Kir Kolyshkin's avatar Kir Kolyshkin Committed by Pavel Emelyanov

criu/filesystems.c: refactor binfmt_misc_restore_bme

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).

[v2: use linux kernel style for 'else' after a block]
[xemul: Fix // comments ]

Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: 's avatarKir Kolyshkin <kir@openvz.org>
Acked-by: 's avatarKirill Tkhai <ktkhai@virtuozzo.com>
Signed-off-by: 's avatarPavel Emelyanov <xemul@virtuozzo.com>
parent 64f964f0
...@@ -271,25 +271,33 @@ static int binfmt_misc_restore_bme(struct mount_info *mi, BinfmtMiscEntry *bme, ...@@ -271,25 +271,33 @@ static int binfmt_misc_restore_bme(struct mount_info *mi, BinfmtMiscEntry *bme,
{ {
int ret; int ret;
/* :name:type:offset:magic/extension:mask:interpreter:flags */ if (!bme->name || !bme->interpreter)
if ((!bme->magic && !bme->extension) || !bme->interpreter) { goto bad_dump;
pr_perror("binfmt_misc: bad dump");
ret = -1; /* Either magic or extension should be there */
} else if (bme->magic) { if (bme->magic) {
ret = make_bfmtm_magic_str(buf, bme); ret = make_bfmtm_magic_str(buf, bme);
} else if (bme->extension) { } else if (bme->extension) {
/* :name:E::extension::interpreter:flags */ /* :name:E::extension::interpreter:flags */
ret = snprintf(buf, BINFMT_MISC_STR, ":%s:E::%s::%s:%s", ret = snprintf(buf, BINFMT_MISC_STR, ":%s:E::%s::%s:%s",
bme->name, bme->extension, bme->interpreter, bme->name, bme->extension, bme->interpreter,
bme->flags ? : "\0"); bme->flags ? : "\0");
} if (ret >= BINFMT_MISC_STR) /* output truncated */
ret = -1;
} else
ret = -1;
if (ret < 0)
goto bad_dump;
if (ret > 0) {
pr_debug("binfmt_misc_pattern=%s\n", buf); pr_debug("binfmt_misc_pattern=%s\n", buf);
ret = write_binfmt_misc_entry(mi->mountpoint, buf, bme); ret = write_binfmt_misc_entry(mi->mountpoint, buf, bme);
}
return ret; return ret;
bad_dump:
pr_perror("binfmt_misc: bad dump");
return -1;
} }
static int binfmt_misc_restore(struct mount_info *mi) static int binfmt_misc_restore(struct mount_info *mi)
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment