[v2,1/2] c-lib: fixed memleaks, add criu_local_free_opts()

Submitted by Martin Wuehrer on Jan. 7, 2019, 8:20 a.m.

Details

Message ID 20190107082043.18620-1-martin.wuehrer@artech.at
State New
Series "Series without cover letter"
Headers show

Commit Message

Martin Wuehrer Jan. 7, 2019, 8:20 a.m.
If `criu_local_init_opts()` is applied on the same opts-object
several times, not all of the allocated memory gets freed.
Therefore, the functions `criu_(local_)free_opts()` were introduced.
These functions ensure, that opts get freed accordingly.
Furthermore, `criu_(local_)free_opts()` gets part of the c-api,
and can therefore be called by external projects too.

Additionally, with this commit `criu_local_init_opts()` now uses
`criu_local_free_opts()`, to free the opts-parameter if it was already
initalized before.

This commit also contains a fix in `send_req_and_recv_resp_sk()` which
lead to a memory leak, if criu-notifications were received.

Signed-off-by: Martin Wührer <martin.wuehrer@artech.at>
---

Changes since v1:
As free() already checks if the pointer that should be freed is NULL before doing any operations,
any additional NULL-check before calling free() is unnecessary. (Thanks to Andrei Vagin avagin@gmail.com)
Therefore, this additional checks are removed by this commit.

 lib/c/criu.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++----
 lib/c/criu.h |   2 +
 2 files changed, 134 insertions(+), 9 deletions(-)

Patch hide | download patch | download mbox

diff --git a/lib/c/criu.c b/lib/c/criu.c
index ff9c9b87..53f16040 100644
--- a/lib/c/criu.c
+++ b/lib/c/criu.c
@@ -81,6 +81,128 @@  void criu_set_service_binary(const char *path)
 	criu_local_set_service_binary(global_opts, path);
 }
 
+void criu_local_free_opts(criu_opts *opts)
+{
+	if(!opts) {
+		return;
+	}	
+	if(opts->rpc) {
+		int i;
+		if (opts->rpc->exec_cmd) {
+			for (i = 0; i < opts->rpc->n_exec_cmd; i++) {
+				free(opts->rpc->exec_cmd[i]);
+			}
+			free(opts->rpc->exec_cmd);
		}
+		opts->rpc->n_exec_cmd = 0;
+
+		if(opts->rpc->unix_sk_ino) {
+			for (i = 0; i < opts->rpc->n_unix_sk_ino; i++) {
+				free(opts->rpc->unix_sk_ino[i]);
+			}
+			free(opts->rpc->unix_sk_ino);
+		}
+		opts->rpc->n_unix_sk_ino = 0;
+
+		if(opts->rpc->ext_mnt) {
+			for (i = 0; i < opts->rpc->n_ext_mnt; i++) {
+				if (opts->rpc->ext_mnt[i]) {
+					free(opts->rpc->ext_mnt[i]->val);
+					free(opts->rpc->ext_mnt[i]->key);
+					free(opts->rpc->ext_mnt[i]);
+				}
+			}
+			free(opts->rpc->ext_mnt);
+		}
+		opts->rpc->n_ext_mnt = 0;
+		
+		if(opts->rpc->cg_root) {
+			for (i = 0; i < opts->rpc->n_cg_root; i++) {
+				if (opts->rpc->cg_root[i]) {
+					free(opts->rpc->cg_root[i]->ctrl);
+					free(opts->rpc->cg_root[i]->path);
+					free(opts->rpc->cg_root[i]);
+				}
+			}
+			free(opts->rpc->cg_root);
+		}
+		opts->rpc->n_cg_root = 0;
+		
+		if(opts->rpc->veths) {
+			for (i = 0; i < opts->rpc->n_veths; i++) {
+				if (opts->rpc->veths[i]) {
+					free(opts->rpc->veths[i]->if_in);
+					free(opts->rpc->veths[i]->if_out);
+					free(opts->rpc->veths[i]);
+				}
+			}
+			free(opts->rpc->veths);
+		}
+		opts->rpc->n_veths = 0;
+		
+		if(opts->rpc->enable_fs) {
+			for (i = 0; i < opts->rpc->n_enable_fs; i++) {
+				free(opts->rpc->enable_fs[i]);
+			}
+			free(opts->rpc->enable_fs);
+		}
+		opts->rpc->n_enable_fs = 0;
+		
+		if(opts->rpc->skip_mnt) {
+			for (i = 0; i < opts->rpc->n_skip_mnt; i++) {
+				free(opts->rpc->skip_mnt[i]);
+			}
+			free(opts->rpc->skip_mnt);
+		}
+		opts->rpc->n_skip_mnt = 0;
+
+		if(opts->rpc->irmap_scan_paths) {
+			for (i = 0; i < opts->rpc->n_irmap_scan_paths; i++) {
+				free(opts->rpc->irmap_scan_paths[i]);
+			}
+			free(opts->rpc->irmap_scan_paths);
+		}
+		opts->rpc->n_irmap_scan_paths = 0;
+
+		if(opts->rpc->cgroup_dump_controller) {
+			for (i = 0; i < opts->rpc->n_cgroup_dump_controller; i++) {
+				free(opts->rpc->cgroup_dump_controller[i]);
+			}
+			free(opts->rpc->cgroup_dump_controller);
+		}
+		opts->rpc->n_cgroup_dump_controller = 0;
+
+		if(opts->rpc->inherit_fd) {
+			for (i = 0; i < opts->rpc->n_inherit_fd; i++) {
+				if (opts->rpc->inherit_fd[i]) {
+					free(opts->rpc->inherit_fd[i]->key);
+					free(opts->rpc->inherit_fd[i]);
+				}
+			}
+			free(opts->rpc->inherit_fd);
+		}
+		opts->rpc->n_inherit_fd = 0;
+
+		if(opts->rpc->external) {
+			for (i = 0; i < opts->rpc->n_external; i++) {
+				free(opts->rpc->external[i]);
+			}
+			free(opts->rpc->external);
+		}
+		opts->rpc->n_external = 0;
+
+		free(opts->rpc->cgroup_props_file);
+		free(opts->rpc->cgroup_props);
+		free(opts->rpc->parent_img);
+		free(opts->rpc->root);
+		free(opts->rpc->freeze_cgroup);
+		free(opts->rpc->log_file);
+		free(opts->rpc);
+	}
+	free(opts);
+	opts = NULL;
+}
+
 int criu_local_init_opts(criu_opts **o)
 {
 	criu_opts *opts = NULL;
@@ -88,13 +210,7 @@  int criu_local_init_opts(criu_opts **o)
 
 	opts = *o;
 
-	if (opts) {
-		if (opts->rpc)
-			criu_opts__free_unpacked(opts->rpc, NULL);
-
-		free(opts);
-		opts = NULL;
-	}
+	criu_local_free_opts(opts);
 
 	rpc = malloc(sizeof(CriuOpts));
 	if (rpc == NULL) {
@@ -107,7 +223,7 @@  int criu_local_init_opts(criu_opts **o)
 	opts = malloc(sizeof(criu_opts));
 	if (opts == NULL) {
 		perror("Can't allocate memory for criu opts");
-		criu_opts__free_unpacked(rpc, NULL);
+		criu_local_free_opts(opts);
 		return -1;
 	}
 
@@ -127,6 +243,11 @@  int criu_init_opts(void)
 	return criu_local_init_opts(&global_opts);
 }
 
+void criu_free_opts(void)
+{
+	criu_local_free_opts(global_opts);
+}
+
 void criu_local_set_notify_cb(criu_opts *opts, int (*cb)(char *action, criu_notify_arg_t na))
 {
 	opts->notify = cb;
@@ -1123,8 +1244,10 @@  again:
 			ret = opts->notify((*resp)->notify->script, (*resp)->notify);
 
 		ret = send_notify_ack(fd, ret);
-		if (!ret)
+		if (!ret) {
+			criu_resp__free_unpacked(*resp, NULL);
 			goto again;
+		}
 		else
 			goto exit;
 	}
diff --git a/lib/c/criu.h b/lib/c/criu.h
index ae8683bc..e244be85 100644
--- a/lib/c/criu.h
+++ b/lib/c/criu.h
@@ -56,6 +56,7 @@  void criu_set_service_comm(enum criu_service_comm);
  * the list down below. 0 on success, -1 on fail.
  */
 int criu_init_opts(void);
+void criu_free_opts(void);
 
 void criu_set_pid(int pid);
 void criu_set_images_dir_fd(int fd); /* must be set for dump/restore */
@@ -158,6 +159,7 @@  int criu_dump_iters(int (*more)(criu_predump_info pi));
 typedef struct criu_opts criu_opts;
 
 int criu_local_init_opts(criu_opts **opts);
+void criu_local_free_opts(criu_opts *opts);
 
 void criu_local_set_service_address(criu_opts *opts, char *path);
 void criu_local_set_service_fd(criu_opts *opts, int fd);

Comments

Radostin Stoyanov Jan. 19, 2019, 10:04 a.m.
On 07/01/2019 08:20, Martin Wührer wrote:
> If `criu_local_init_opts()` is applied on the same opts-object
> several times, not all of the allocated memory gets freed.
> Therefore, the functions `criu_(local_)free_opts()` were introduced.
> These functions ensure, that opts get freed accordingly.
> Furthermore, `criu_(local_)free_opts()` gets part of the c-api,
> and can therefore be called by external projects too.
>
> Additionally, with this commit `criu_local_init_opts()` now uses
> `criu_local_free_opts()`, to free the opts-parameter if it was already
> initalized before.
>
> This commit also contains a fix in `send_req_and_recv_resp_sk()` which
> lead to a memory leak, if criu-notifications were received.
>
> Signed-off-by: Martin Wührer <martin.wuehrer@artech.at>
> ---
>
> Changes since v1:
> As free() already checks if the pointer that should be freed is NULL before doing any operations,
> any additional NULL-check before calling free() is unnecessary. (Thanks to Andrei Vagin avagin@gmail.com)
> Therefore, this additional checks are removed by this commit.
>
>  lib/c/criu.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++----
>  lib/c/criu.h |   2 +
>  2 files changed, 134 insertions(+), 9 deletions(-)
>
> diff --git a/lib/c/criu.c b/lib/c/criu.c
> index ff9c9b87..53f16040 100644
> --- a/lib/c/criu.c
> +++ b/lib/c/criu.c
> @@ -81,6 +81,128 @@ void criu_set_service_binary(const char *path)
>  	criu_local_set_service_binary(global_opts, path);
>  }
>  
> +void criu_local_free_opts(criu_opts *opts)
> +{
> +	if(!opts) {
> +		return;
> +	}	
> +	if(opts->rpc) {
> +		int i;
> +		if (opts->rpc->exec_cmd) {
> +			for (i = 0; i < opts->rpc->n_exec_cmd; i++) {
> +				free(opts->rpc->exec_cmd[i]);
> +			}
> +			free(opts->rpc->exec_cmd);
> 		}
When applying the patch I've got "error: corrupt patch at line 25"
Manually adding a '+' at the beginning of this line fixed the issue.

> +		opts->rpc->n_exec_cmd = 0;
> +
> +		if(opts->rpc->unix_sk_ino) {
> +			for (i = 0; i < opts->rpc->n_unix_sk_ino; i++) {
> +				free(opts->rpc->unix_sk_ino[i]);
> +			}
> +			free(opts->rpc->unix_sk_ino);
> +		}
> +		opts->rpc->n_unix_sk_ino = 0;
> +
> +		if(opts->rpc->ext_mnt) {
> +			for (i = 0; i < opts->rpc->n_ext_mnt; i++) {
> +				if (opts->rpc->ext_mnt[i]) {
> +					free(opts->rpc->ext_mnt[i]->val);
> +					free(opts->rpc->ext_mnt[i]->key);
> +					free(opts->rpc->ext_mnt[i]);
> +				}
> +			}
> +			free(opts->rpc->ext_mnt);
> +		}
> +		opts->rpc->n_ext_mnt = 0;
> +		
> +		if(opts->rpc->cg_root) {
> +			for (i = 0; i < opts->rpc->n_cg_root; i++) {
> +				if (opts->rpc->cg_root[i]) {
> +					free(opts->rpc->cg_root[i]->ctrl);
> +					free(opts->rpc->cg_root[i]->path);
> +					free(opts->rpc->cg_root[i]);
> +				}
> +			}
> +			free(opts->rpc->cg_root);
> +		}
> +		opts->rpc->n_cg_root = 0;
> +		
> +		if(opts->rpc->veths) {
> +			for (i = 0; i < opts->rpc->n_veths; i++) {
> +				if (opts->rpc->veths[i]) {
> +					free(opts->rpc->veths[i]->if_in);
> +					free(opts->rpc->veths[i]->if_out);
> +					free(opts->rpc->veths[i]);
> +				}
> +			}
> +			free(opts->rpc->veths);
> +		}
> +		opts->rpc->n_veths = 0;
> +		
> +		if(opts->rpc->enable_fs) {
> +			for (i = 0; i < opts->rpc->n_enable_fs; i++) {
> +				free(opts->rpc->enable_fs[i]);
> +			}
> +			free(opts->rpc->enable_fs);
> +		}
> +		opts->rpc->n_enable_fs = 0;
> +		
> +		if(opts->rpc->skip_mnt) {
> +			for (i = 0; i < opts->rpc->n_skip_mnt; i++) {
> +				free(opts->rpc->skip_mnt[i]);
> +			}
> +			free(opts->rpc->skip_mnt);
> +		}
> +		opts->rpc->n_skip_mnt = 0;
> +
> +		if(opts->rpc->irmap_scan_paths) {
> +			for (i = 0; i < opts->rpc->n_irmap_scan_paths; i++) {
> +				free(opts->rpc->irmap_scan_paths[i]);
> +			}
> +			free(opts->rpc->irmap_scan_paths);
> +		}
> +		opts->rpc->n_irmap_scan_paths = 0;
> +
> +		if(opts->rpc->cgroup_dump_controller) {
> +			for (i = 0; i < opts->rpc->n_cgroup_dump_controller; i++) {
> +				free(opts->rpc->cgroup_dump_controller[i]);
> +			}
> +			free(opts->rpc->cgroup_dump_controller);
> +		}
> +		opts->rpc->n_cgroup_dump_controller = 0;
> +
> +		if(opts->rpc->inherit_fd) {
> +			for (i = 0; i < opts->rpc->n_inherit_fd; i++) {
> +				if (opts->rpc->inherit_fd[i]) {
> +					free(opts->rpc->inherit_fd[i]->key);
> +					free(opts->rpc->inherit_fd[i]);
> +				}
> +			}
> +			free(opts->rpc->inherit_fd);
> +		}
> +		opts->rpc->n_inherit_fd = 0;
> +
> +		if(opts->rpc->external) {
> +			for (i = 0; i < opts->rpc->n_external; i++) {
> +				free(opts->rpc->external[i]);
> +			}
> +			free(opts->rpc->external);
> +		}
> +		opts->rpc->n_external = 0;
> +
> +		free(opts->rpc->cgroup_props_file);
> +		free(opts->rpc->cgroup_props);
> +		free(opts->rpc->parent_img);
> +		free(opts->rpc->root);
> +		free(opts->rpc->freeze_cgroup);
> +		free(opts->rpc->log_file);
> +		free(opts->rpc);
> +	}
> +	free(opts);
> +	opts = NULL;
> +}
> +
>  int criu_local_init_opts(criu_opts **o)
>  {
>  	criu_opts *opts = NULL;
> @@ -88,13 +210,7 @@ int criu_local_init_opts(criu_opts **o)
>  
>  	opts = *o;
>  
> -	if (opts) {
> -		if (opts->rpc)
> -			criu_opts__free_unpacked(opts->rpc, NULL);
> -
> -		free(opts);
> -		opts = NULL;
> -	}
> +	criu_local_free_opts(opts);
>  
>  	rpc = malloc(sizeof(CriuOpts));
>  	if (rpc == NULL) {
> @@ -107,7 +223,7 @@ int criu_local_init_opts(criu_opts **o)
>  	opts = malloc(sizeof(criu_opts));
>  	if (opts == NULL) {
>  		perror("Can't allocate memory for criu opts");
> -		criu_opts__free_unpacked(rpc, NULL);
> +		criu_local_free_opts(opts);
>  		return -1;
>  	}
>  
> @@ -127,6 +243,11 @@ int criu_init_opts(void)
>  	return criu_local_init_opts(&global_opts);
>  }
>  
> +void criu_free_opts(void)
> +{
> +	criu_local_free_opts(global_opts);
> +}
> +
>  void criu_local_set_notify_cb(criu_opts *opts, int (*cb)(char *action, criu_notify_arg_t na))
>  {
>  	opts->notify = cb;
> @@ -1123,8 +1244,10 @@ again:
>  			ret = opts->notify((*resp)->notify->script, (*resp)->notify);
>  
>  		ret = send_notify_ack(fd, ret);
> -		if (!ret)
> +		if (!ret) {
> +			criu_resp__free_unpacked(*resp, NULL);
>  			goto again;
> +		}
>  		else
>  			goto exit;
>  	}
> diff --git a/lib/c/criu.h b/lib/c/criu.h
> index ae8683bc..e244be85 100644
> --- a/lib/c/criu.h
> +++ b/lib/c/criu.h
> @@ -56,6 +56,7 @@ void criu_set_service_comm(enum criu_service_comm);
>   * the list down below. 0 on success, -1 on fail.
>   */
>  int criu_init_opts(void);
> +void criu_free_opts(void);
>  
>  void criu_set_pid(int pid);
>  void criu_set_images_dir_fd(int fd); /* must be set for dump/restore */
> @@ -158,6 +159,7 @@ int criu_dump_iters(int (*more)(criu_predump_info pi));
>  typedef struct criu_opts criu_opts;
>  
>  int criu_local_init_opts(criu_opts **opts);
> +void criu_local_free_opts(criu_opts *opts);
>  
>  void criu_local_set_service_address(criu_opts *opts, char *path);
>  void criu_local_set_service_fd(criu_opts *opts, int fd);