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

[Kir Kolyshkin]

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]

Cc: Kirill Tkhai <ktkhai at virtuozzo.com>
Signed-off-by: Kir Kolyshkin <kir at openvz.org>
---
 criu/filesystems.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/criu/filesystems.c b/criu/filesystems.c
index d66a6c6..25ae89f 100644
--- a/criu/filesystems.c
+++ b/criu/filesystems.c
@@ -271,25 +271,33 @@ 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) {
 		/* :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)
-- 
2.7.4