diff mbox

depmod: fix errorpath memleaks in report cycles logic

Message ID 20170320100951.20884-1-yauheni.kaliuta@redhat.com (mailing list archive)
State Accepted
Headers show

Commit Message

Yauheni Kaliuta March 20, 2017, 10:09 a.m. UTC 
From: Yauheni Kaliuta <y.kaliuta@gmail.com>

The c7ce9f0c80f3d561078a78205a14c5ba7663cfdd commit (depmod:
handle nested loops) introduced a bunch of possible memory leaks
in error path. In the real world scenario it is not a problem,
since the utility quits if it detects any of the errors, but from
the programming point of view, it is not nice. So, add the
cleanups.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/depmod.c | 54 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 17 deletions(-)

Comments

Yauheni Kaliuta March 20, 2017, 10:28 a.m. UTC | #1 
Hi!

>>>>> On Mon, 20 Mar 2017 12:09:51 +0200, Yauheni Kaliuta  wrote:

 > From: Yauheni Kaliuta <y.kaliuta@gmail.com>

Ooops. Could you please remove the line? ;)

 > The c7ce9f0c80f3d561078a78205a14c5ba7663cfdd commit (depmod:
 > handle nested loops) introduced a bunch of possible memory leaks
 > in error path. In the real world scenario it is not a problem,
 > since the utility quits if it detects any of the errors, but from
 > the programming point of view, it is not nice. So, add the
 > cleanups.

 > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 > ---
 >  tools/depmod.c | 54 +++++++++++++++++++++++++++++++++++++-----------------
 >  1 file changed, 37 insertions(+), 17 deletions(-)

 > diff --git a/tools/depmod.c b/tools/depmod.c
 > index 116adbeb14a0..7f9e9804787e 100644
 > --- a/tools/depmod.c
 > +++ b/tools/depmod.c
 > @@ -1481,10 +1481,10 @@ static void depmod_list_remove_data(struct kmod_list **list, void *data)
 >  	*list = l;
 >  }
 
 > -static void depmod_report_one_cycle(struct depmod *depmod,
 > -				    struct vertex *vertex,
 > -				    struct kmod_list **roots,
 > -				    struct hash *loop_set)
 > +static int depmod_report_one_cycle(struct depmod *depmod,
 > +				   struct vertex *vertex,
 > +				   struct kmod_list **roots,
 > +				   struct hash *loop_set)
 >  {
 >  	const char sep[] = " -> ";
 >  	size_t sz;
 > @@ -1493,6 +1493,7 @@ static void depmod_report_one_cycle(struct depmod *depmod,
 >  	int i;
 >  	int n;
 >  	struct vertex *v;
 > +	int rc;
 
 >  	array_init(&reverse, 3);
 
 > @@ -1503,7 +1504,10 @@ static void depmod_report_one_cycle(struct depmod *depmod,
 
 >  		sz += v->mod->modnamesz - 1;
 >  		array_append(&reverse, v);
 > -		hash_add(loop_set, v->mod->modname, NULL);
 > +		rc = hash_add(loop_set, v->mod->modname, NULL);
 > +		if (rc != 0)
 > +			return rc;
 > +		/* the hash will be freed where created */
 >  	}
 >  	sz += vertex->mod->modnamesz - 1;
 
 > @@ -1528,6 +1532,8 @@ static void depmod_report_one_cycle(struct depmod *depmod,
 
 >  	free(buf);
 >  	array_free_array(&reverse);
 > +
 > +	return 0;
 >  }
 
 >  static int depmod_report_cycles_from_root(struct depmod *depmod,
 > @@ -1545,17 +1551,18 @@ static int depmod_report_cycles_from_root(struct depmod *depmod,
 >  	struct mod *m;
 >  	struct mod **itr, **itr_end;
 >  	size_t is;
 > +	int ret = -ENOMEM;
 
 >  	root = vertex_new(root_mod, NULL);
 >  	if (root == NULL) {
 >  		ERR("No memory to report cycles\n");
 > -		return -ENOMEM;
 > +		goto out;
 >  	}
 
 >  	l = kmod_list_append(free_list, root);
 >  	if (l == NULL) {
 >  		ERR("No memory to report cycles\n");
 > -		return -ENOMEM;
 > +		goto out;
 >  	}
 >  	free_list = l;
 
 > @@ -1570,8 +1577,13 @@ static int depmod_report_cycles_from_root(struct depmod *depmod,
 >  		 * from part of a loop or from a branch after a loop
 >  		 */
 >  		if (m->visited && m == root->mod) {
 > -			depmod_report_one_cycle(depmod, vertex,
 > -						roots, loop_set);
 > +			int rc;
 > +			rc = depmod_report_one_cycle(depmod, vertex,
 > +						     roots, loop_set);
 > +			if (rc != 0) {
 > +				ret = rc;
 > +				goto out;
 > +			}
 >  			continue;
 >  		}
 
 > @@ -1598,7 +1610,7 @@ static int depmod_report_cycles_from_root(struct depmod *depmod,
 >  			v = vertex_new(dep, vertex);
 >  			if (v == NULL) {
 >  				ERR("No memory to report cycles\n");
 > -				return -ENOMEM;
 > +				goto out;
 >  			}
 >  			assert(is < stack_size);
 >  			stack[is++] = v;
 > @@ -1606,12 +1618,15 @@ static int depmod_report_cycles_from_root(struct depmod *depmod,
 >  			l = kmod_list_append(free_list, v);
 >  			if (l == NULL) {
 >  				ERR("No memory to report cycles\n");
 > -				return -ENOMEM;
 > +				goto out;
 >  			}
 >  			free_list = l;
 
 >  		}
 >  	}
 > +	ret = 0;
 > +
 > +out:
 >  	while (free_list) {
 >  		v = free_list->data;
 >  		l = kmod_list_remove(free_list);
 > @@ -1619,7 +1634,7 @@ static int depmod_report_cycles_from_root(struct depmod *depmod,
 >  		free(v);
 >  	}
 
 > -	return 0;
 > +	return ret;
 >  }
 
 >  static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 > @@ -1643,7 +1658,7 @@ static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 >  		l = kmod_list_append(roots, m);
 >  		if (l == NULL) {
 >  			ERR("No memory to report cycles\n");
 > -			return;
 > +			goto out_list;
 >  		}
 >  		roots = l;
 >  		n_r++;
 > @@ -1652,13 +1667,13 @@ static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 >  	stack = malloc(n_r * sizeof(void *));
 >  	if (stack == NULL) {
 >  		ERR("No memory to report cycles\n");
 > -		return;
 > +		goto out_list;
 >  	}
 
 >  	loop_set = hash_new(16, NULL);
 >  	if (loop_set == NULL) {
 >  		ERR("No memory to report cycles\n");
 > -		return;
 > +		goto out_list;
 >  	}
 
 >  	while (roots != NULL) {
 > @@ -1670,14 +1685,19 @@ static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 >  						     &roots,
 >  						     stack, n_r, loop_set);
 >  		if (err < 0)
 > -			goto err;
 > +			goto out_hash;
 >  	}
 
 >  	num_cyclic = hash_get_count(loop_set);
 >  	ERR("Found %d modules in dependency cycles!\n", num_cyclic);
 
 > -err:
 > +out_hash:
 >  	hash_free(loop_set);
 > +out_list:
 > +	while (roots != NULL) {
 > +		/* no need to free data, come from outside */
 > +		roots = kmod_list_remove(roots);
 > +	}
 >  }
 
 >  static int depmod_calculate_dependencies(struct depmod *depmod)
 > -- 
 > 2.9.2.368.g08bb350

 > --
 > To unsubscribe from this list: send the line "unsubscribe linux-modules" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lucas De Marchi May 9, 2017, 6:48 a.m. UTC | #2 
On Mon, Mar 20, 2017 at 3:28 AM, Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
> Hi!
>
>>>>>> On Mon, 20 Mar 2017 12:09:51 +0200, Yauheni Kaliuta  wrote:
>
>  > From: Yauheni Kaliuta <y.kaliuta@gmail.com>
>
> Ooops. Could you please remove the line? ;)

Yep. Applied, thanks.

Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tools/depmod.c b/tools/depmod.c
index 116adbeb14a0..7f9e9804787e 100644
--- a/tools/depmod.c
+++ b/tools/depmod.c
@@ -1481,10 +1481,10 @@  static void depmod_list_remove_data(struct kmod_list **list, void *data)
 	*list = l;
 }
 
-static void depmod_report_one_cycle(struct depmod *depmod,
-				    struct vertex *vertex,
-				    struct kmod_list **roots,
-				    struct hash *loop_set)
+static int depmod_report_one_cycle(struct depmod *depmod,
+				   struct vertex *vertex,
+				   struct kmod_list **roots,
+				   struct hash *loop_set)
 {
 	const char sep[] = " -> ";
 	size_t sz;
@@ -1493,6 +1493,7 @@  static void depmod_report_one_cycle(struct depmod *depmod,
 	int i;
 	int n;
 	struct vertex *v;
+	int rc;
 
 	array_init(&reverse, 3);
 
@@ -1503,7 +1504,10 @@  static void depmod_report_one_cycle(struct depmod *depmod,
 
 		sz += v->mod->modnamesz - 1;
 		array_append(&reverse, v);
-		hash_add(loop_set, v->mod->modname, NULL);
+		rc = hash_add(loop_set, v->mod->modname, NULL);
+		if (rc != 0)
+			return rc;
+		/* the hash will be freed where created */
 	}
 	sz += vertex->mod->modnamesz - 1;
 
@@ -1528,6 +1532,8 @@  static void depmod_report_one_cycle(struct depmod *depmod,
 
 	free(buf);
 	array_free_array(&reverse);
+
+	return 0;
 }
 
 static int depmod_report_cycles_from_root(struct depmod *depmod,
@@ -1545,17 +1551,18 @@  static int depmod_report_cycles_from_root(struct depmod *depmod,
 	struct mod *m;
 	struct mod **itr, **itr_end;
 	size_t is;
+	int ret = -ENOMEM;
 
 	root = vertex_new(root_mod, NULL);
 	if (root == NULL) {
 		ERR("No memory to report cycles\n");
-		return -ENOMEM;
+		goto out;
 	}
 
 	l = kmod_list_append(free_list, root);
 	if (l == NULL) {
 		ERR("No memory to report cycles\n");
-		return -ENOMEM;
+		goto out;
 	}
 	free_list = l;
 
@@ -1570,8 +1577,13 @@  static int depmod_report_cycles_from_root(struct depmod *depmod,
 		 * from part of a loop or from a branch after a loop
 		 */
 		if (m->visited && m == root->mod) {
-			depmod_report_one_cycle(depmod, vertex,
-						roots, loop_set);
+			int rc;
+			rc = depmod_report_one_cycle(depmod, vertex,
+						     roots, loop_set);
+			if (rc != 0) {
+				ret = rc;
+				goto out;
+			}
 			continue;
 		}
 
@@ -1598,7 +1610,7 @@  static int depmod_report_cycles_from_root(struct depmod *depmod,
 			v = vertex_new(dep, vertex);
 			if (v == NULL) {
 				ERR("No memory to report cycles\n");
-				return -ENOMEM;
+				goto out;
 			}
 			assert(is < stack_size);
 			stack[is++] = v;
@@ -1606,12 +1618,15 @@  static int depmod_report_cycles_from_root(struct depmod *depmod,
 			l = kmod_list_append(free_list, v);
 			if (l == NULL) {
 				ERR("No memory to report cycles\n");
-				return -ENOMEM;
+				goto out;
 			}
 			free_list = l;
 
 		}
 	}
+	ret = 0;
+
+out:
 	while (free_list) {
 		v = free_list->data;
 		l = kmod_list_remove(free_list);
@@ -1619,7 +1634,7 @@  static int depmod_report_cycles_from_root(struct depmod *depmod,
 		free(v);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
@@ -1643,7 +1658,7 @@  static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 		l = kmod_list_append(roots, m);
 		if (l == NULL) {
 			ERR("No memory to report cycles\n");
-			return;
+			goto out_list;
 		}
 		roots = l;
 		n_r++;
@@ -1652,13 +1667,13 @@  static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 	stack = malloc(n_r * sizeof(void *));
 	if (stack == NULL) {
 		ERR("No memory to report cycles\n");
-		return;
+		goto out_list;
 	}
 
 	loop_set = hash_new(16, NULL);
 	if (loop_set == NULL) {
 		ERR("No memory to report cycles\n");
-		return;
+		goto out_list;
 	}
 
 	while (roots != NULL) {
@@ -1670,14 +1685,19 @@  static void depmod_report_cycles(struct depmod *depmod, uint16_t n_mods,
 						     &roots,
 						     stack, n_r, loop_set);
 		if (err < 0)
-			goto err;
+			goto out_hash;
 	}
 
 	num_cyclic = hash_get_count(loop_set);
 	ERR("Found %d modules in dependency cycles!\n", num_cyclic);
 
-err:
+out_hash:
 	hash_free(loop_set);
+out_list:
+	while (roots != NULL) {
+		/* no need to free data, come from outside */
+		roots = kmod_list_remove(roots);
+	}
 }
 
 static int depmod_calculate_dependencies(struct depmod *depmod)