[CRIU] [PATCH] crtools: Make fdset operations robust against open() errors

[Cyrill Gorcunov]

There are two cases for cr_fdset_open

 - It might be called with already allocated
   memory so we should reuse it.

 - It might be called with NULL pointing out
   that caller expects us to allocate memory.

If an open() error happens somewhere inside cr_fdset_open
it requires two error paths

 - Just close all files opened but don't free memory
   if it was not allocated by us

 - Close all files opened *and* free memory allocated
   by us.

In any case we should close all files opened so close_cr_fdset()
helper is splitted into two parts.

Also the caller should be ready for such semantics as well and
do not re-assign pointers obtained but simply test for NULL
on results.

Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
---

This one should do the trick I think.

 cr-dump.c |    8 ++---
 crtools.c |   85 +++++++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 55 insertions(+), 38 deletions(-)

diff --git a/cr-dump.c b/cr-dump.c
index a4b938f..8a76987 100644
--- a/cr-dump.c
+++ b/cr-dump.c
@@ -1214,8 +1214,8 @@ static int dump_one_task(struct pstree_item *item, struct cr_fdset *cr_fdset)
 		goto err;
 	};
 
-	cr_fdset = cr_fdset_open(item->pid, CR_FD_DESC_TASK, cr_fdset);
-	if (!cr_fdset)
+	ret = -1;
+	if (!cr_fdset_open(item->pid, CR_FD_DESC_TASK, cr_fdset))
 		goto err;
 
 	ret = collect_mappings(pid, pid_dir, &vma_area_list);
@@ -1350,9 +1350,7 @@ int cr_dump_tasks(pid_t pid, struct cr_options *opts)
 			goto err;
 
 		if (item->pid == pid) {
-			cr_fdset = cr_fdset_open(item->pid,
-					CR_FD_DESC_USE(CR_FD_PSTREE), cr_fdset);
-			if (!cr_fdset)
+			if (!cr_fdset_open(item->pid, CR_FD_DESC_USE(CR_FD_PSTREE), cr_fdset))
 				goto err;
 			if (dump_pstree(pid, &pstree_list, cr_fdset))
 				goto err;
diff --git a/crtools.c b/crtools.c
index 560c609..faabf18 100644
--- a/crtools.c
+++ b/crtools.c
@@ -123,22 +123,55 @@ static struct cr_fdset *alloc_cr_fdset(void)
 	return cr_fdset;
 }
 
+void __close_cr_fdset(struct cr_fdset *cr_fdset)
+{
+	unsigned int i;
+
+	if (!cr_fdset)
+		return;
+
+	for (i = 0; i < CR_FD_MAX; i++) {
+		if (cr_fdset->fds[i] == -1)
+			continue;
+		pr_debug("Closed %d/%d\n", i, cr_fdset->fds[i]);
+		close_safe(&cr_fdset->fds[i]);
+		cr_fdset->fds[i] = -1;
+	}
+}
+
+void close_cr_fdset(struct cr_fdset **cr_fdset)
+{
+	if (!cr_fdset || !*cr_fdset)
+		return;
+
+	__close_cr_fdset(*cr_fdset);
+
+	xfree(*cr_fdset);
+	*cr_fdset = NULL;
+}
+
 struct cr_fdset *cr_fdset_open(int pid, unsigned long use_mask, struct cr_fdset *cr_fdset)
 {
+	struct cr_fdset *fdset;
 	unsigned int i;
 	int ret = -1;
 	char path[PATH_MAX];
 
-	if (cr_fdset == NULL) {
-		cr_fdset = alloc_cr_fdset();
-		if (!cr_fdset)
+	/*
+	 * We either reuse existing fdset or create new one.
+	 */
+	if (!cr_fdset) {
+		fdset = alloc_cr_fdset();
+		if (!fdset)
 			goto err;
-	}
+	} else
+		fdset = cr_fdset;
 
 	for (i = 0; i < CR_FD_MAX; i++) {
 		if (!(use_mask & CR_FD_DESC_USE(i)))
 			continue;
-		if (cr_fdset->fds[i] != -1)
+
+		if (fdset->fds[i] != -1)
 			continue;
 
 		ret = get_image_path(path, sizeof(path),
@@ -157,15 +190,21 @@ struct cr_fdset *cr_fdset_open(int pid, unsigned long use_mask, struct cr_fdset
 			pr_perror("Unable to open %s", path);
 			goto err;
 		}
+		fdset->fds[i] = ret;
 
 		pr_debug("Opened %s with %d\n", path, ret);
 		if (write_img(ret, &fdset_template[i].magic))
 			goto err;
-
-		cr_fdset->fds[i] = ret;
 	}
+
+	return fdset;
+
 err:
-	return cr_fdset;
+	if (fdset != cr_fdset)
+		__close_cr_fdset(fdset);
+	else
+		close_cr_fdset(&fdset);
+	return NULL;
 }
 
 struct cr_fdset *prep_cr_fdset_for_restore(int pid, unsigned long use_mask)
@@ -194,43 +233,23 @@ struct cr_fdset *prep_cr_fdset_for_restore(int pid, unsigned long use_mask)
 			pr_perror("Unable to open %s", path);
 			goto err;
 		}
+		cr_fdset->fds[i] = ret;
 
 		pr_debug("Opened %s with %d\n", path, ret);
 		if (read_img(ret, &magic) < 0)
 			goto err;
 		if (magic != fdset_template[i].magic) {
-			close(ret);
 			pr_err("Magic doesn't match for %s\n", path);
 			goto err;
 		}
 
-		cr_fdset->fds[i] = ret;
 	}
-err:
-	return cr_fdset;
-}
-
-void close_cr_fdset(struct cr_fdset **cr_fdset)
-{
-	struct cr_fdset *fdset;
-	unsigned int i;
-
-	if (!cr_fdset || !*cr_fdset)
-		return;
-
-	fdset = *cr_fdset;
-
-	for (i = 0; i < CR_FD_MAX; i++) {
-		if (fdset->fds[i] == -1)
-			continue;
 
-		pr_debug("Closed %d/%d\n", i, fdset->fds[i]);
-		close(fdset->fds[i]);
-		fdset->fds[i] = -1;
-	}
+	return cr_fdset;
 
-	xfree(fdset);
-	*cr_fdset = NULL;
+err:
+	close_cr_fdset(&cr_fdset);
+	return NULL;
 }
 
 int main(int argc, char *argv[])
-- 
1.7.7.6