Merge pull request 'Do a better job cleaning up memory' (#239) from dcbaker/pkgconf:free-memory into master

Reviewed-on: ariadne/pkgconf#239

.github/workflows/test.yml

@@ -60,6 +60,30 @@ jobs:
         run: |
           meson test -v -C _build
 
+  debian-meson-asan:
+    runs-on: ubuntu-latest
+    container:
+      image: debian:testing
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v2
+
+      - name: Update system and add dependencies
+        run: |
+          apt-get update
+          apt-get install -y kyua atf-sh build-essential meson
+
+      - name: Build
+        run: |
+          meson _build -Db_sanitize=address
+          meson compile -C _build
+
+      - name: Run tests
+        run: |
+          meson test -v -C _build
+        env:
+          ASAN_OPTIONS: "exitcode=7"
+
   debian-autotools:
     runs-on: ubuntu-latest
     container:

.woodpecker.yml

@@ -12,6 +12,21 @@ pipeline:
         IMAGE: debian
         BUILD: meson
 
+  debian-meson-asan:
+    image: debian:testing
+    environment:
+      - ASAN_OPTIONS="exitcode=7"
+    commands:
+      - apt-get update
+      - apt-get install -y kyua atf-sh build-essential meson
+      - meson _build -Db_sanitize=address
+      - meson compile -C _build
+      - meson test -v -C _build
+    when:
+      matrix:
+        IMAGE: debian
+        BUILD: meson
+
   debian-autotools:
     image: debian:testing
     commands:

cli/main.c

@@ -1183,17 +1183,21 @@ main(int argc, char *argv[])
 					pkgconf_error(&pkg_client, "Package '%s' was not found\n", pkgiter->package);
 
 				ret = EXIT_FAILURE;
-				goto out;
+				goto cleanup;
 			}
 
 			if (pkgconf_compare_version(pkg->version, required_module_version) >= 0)
 			{
 				ret = EXIT_SUCCESS;
-				goto out;
+				goto cleanup;
 			}
 		}
 
 		ret = EXIT_FAILURE;
+cleanup:
+		if (pkg)
+			pkgconf_pkg_unref(&pkg_client, pkg);
+		pkgconf_dependency_free(&deplist);
 		goto out;
 	}
 	else if (required_exact_module_version != NULL)
@@ -1219,17 +1223,21 @@ main(int argc, char *argv[])
 					pkgconf_error(&pkg_client, "Package '%s' was not found\n", pkgiter->package);
 
 				ret = EXIT_FAILURE;
-				goto out;
+				goto cleanup2;
 			}
 
 			if (pkgconf_compare_version(pkg->version, required_exact_module_version) == 0)
 			{
 				ret = EXIT_SUCCESS;
-				goto out;
+				goto cleanup2;
 			}
 		}
 
 		ret = EXIT_FAILURE;
+cleanup2:
+		if (pkg)
+			pkgconf_pkg_unref(&pkg_client, pkg);
+		pkgconf_dependency_free(&deplist);
 		goto out;
 	}
 	else if (required_max_module_version != NULL)
@@ -1255,17 +1263,21 @@ main(int argc, char *argv[])
 					pkgconf_error(&pkg_client, "Package '%s' was not found\n", pkgiter->package);
 
 				ret = EXIT_FAILURE;
-				goto out;
+				goto cleanup3;
 			}
 
 			if (pkgconf_compare_version(pkg->version, required_max_module_version) <= 0)
 			{
 				ret = EXIT_SUCCESS;
-				goto out;
+				goto cleanup3;
 			}
 		}
 
 		ret = EXIT_FAILURE;
+cleanup3:
+		if (pkg)
+			pkgconf_pkg_unref(&pkg_client, pkg);
+		pkgconf_dependency_free(&deplist);
 		goto out;
 	}
 
@@ -1337,7 +1349,7 @@ main(int argc, char *argv[])
 	}
 
 	if ((want_flags & PKG_VALIDATE) == PKG_VALIDATE)
-		return 0;
+		goto out;
 
 	if ((want_flags & PKG_UNINSTALLED) == PKG_UNINSTALLED)
 	{

libpkgconf/cache.c

@@ -16,6 +16,8 @@
 #include <libpkgconf/stdinc.h>
 #include <libpkgconf/libpkgconf.h>
 
+#include <assert.h>
+
 /*
  * !doc
  *
@@ -86,6 +88,9 @@ cache_dump(const pkgconf_client_t *client)
 pkgconf_pkg_t *
 pkgconf_cache_lookup(pkgconf_client_t *client, const char *id)
 {
+	if (client->cache_table == NULL)
+		return NULL;
+
 	pkgconf_pkg_t **pkg;
 
 	pkg = bsearch(id, client->cache_table,
@@ -150,6 +155,9 @@ pkgconf_cache_add(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 void
 pkgconf_cache_remove(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 {
+	if (client->cache_table == NULL)
+		return;
+
 	if (pkg == NULL)
 		return;
 
@@ -167,6 +175,8 @@ pkgconf_cache_remove(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 	if (slot == NULL)
 		return;
 
+	(*slot)->flags &= ~PKGCONF_PKG_PROPF_CACHED;
+	pkgconf_pkg_unref(client, *slot);
 	*slot = NULL;
 
 	qsort(client->cache_table, client->cache_count,
@@ -181,19 +191,15 @@ pkgconf_cache_remove(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 	}
 
 	client->cache_count--;
+	if (client->cache_count > 0)
+	{
 		client->cache_table = pkgconf_reallocarray(client->cache_table,
 			client->cache_count, sizeof(void *));
-}
+	}
-
+	else
-static inline void
-clear_dependency_matches(pkgconf_list_t *list)
-{
-	pkgconf_node_t *iter;
-
-	PKGCONF_FOREACH_LIST_ENTRY(list->head, iter)
 	{
-		pkgconf_dependency_t *dep = iter->data;
+		free(client->cache_table);
-		dep->match = NULL;
+		client->cache_table = NULL;
 	}
 }
 
@@ -211,30 +217,15 @@ clear_dependency_matches(pkgconf_list_t *list)
 void
 pkgconf_cache_free(pkgconf_client_t *client)
 {
-	pkgconf_pkg_t **cache_table;
+	if (client->cache_table == NULL)
-	size_t i, count;
+		return;
 
-	cache_table = pkgconf_reallocarray(NULL, client->cache_count, sizeof (void *));
+	while (client->cache_count > 0)
-	memcpy(cache_table, client->cache_table,
+		pkgconf_cache_remove(client, client->cache_table[0]);
-		client->cache_count * sizeof (void *));
 
-	/* first we clear cached match pointers */
+	free(client->cache_table);
-	for (i = 0, count = client->cache_count; i < count; i++)
+	client->cache_table = NULL;
-	{
+	client->cache_count = 0;
-		pkgconf_pkg_t *pkg = cache_table[i];
-
-		clear_dependency_matches(&pkg->required);
-		clear_dependency_matches(&pkg->requires_private);
-		clear_dependency_matches(&pkg->provides);
-		clear_dependency_matches(&pkg->conflicts);
-	}
-
-	/* now forcibly free everything */
-	for (i = 0, count = client->cache_count; i < count; i++)
-	{
-		pkgconf_pkg_t *pkg = cache_table[i];
-		pkgconf_pkg_free(client, pkg);
-	}
 
 	PKGCONF_TRACE(client, "cleared package cache");
 }

libpkgconf/client.c

@@ -98,6 +98,8 @@ pkgconf_client_init(pkgconf_client_t *client, pkgconf_error_handler_func_t error
 	client->error_handler_data = error_handler_data;
 	client->error_handler = error_handler;
 	client->auditf = NULL;
+	client->cache_table = NULL;
+	client->cache_count = 0;
 
 #ifndef PKGCONF_LITE
 	if (client->trace_handler == NULL)

libpkgconf/dependency.c

@@ -113,9 +113,16 @@ add_or_replace_dependency_node(pkgconf_client_t *client, pkgconf_dependency_t *d
 	}
 
 	PKGCONF_TRACE(client, "added dependency [%s] to list @%p; flags=%x", dependency_to_str(dep, depbuf, sizeof depbuf), list, dep->flags);
-	pkgconf_node_insert_tail(&dep->iter, dep, list);
+	pkgconf_node_insert_tail(&dep->iter, pkgconf_dependency_ref(dep->owner, dep), list);
 
-	return pkgconf_dependency_ref(dep->owner, dep);
+	/* This dependency is intentionally unowned.
+	 *
+	 * Internally we have no use for the returned type, and usually just
+	 * discard it. However, there is a publig pkgconf_dependency_add
+	 * function, which references this return value before returning it,
+	 * giving ownership at that point.
+	 */
+	return dep;
 }
 
 static inline pkgconf_dependency_t *
@@ -156,10 +163,10 @@ pkgconf_dependency_addraw(pkgconf_client_t *client, pkgconf_list_t *list, const
 pkgconf_dependency_t *
 pkgconf_dependency_add(pkgconf_client_t *client, pkgconf_list_t *list, const char *package, const char *version, pkgconf_pkg_comparator_t compare, unsigned int flags)
 {
-	if (version != NULL)
+	pkgconf_dependency_t *dep;
-		return pkgconf_dependency_addraw(client, list, package, strlen(package), version, strlen(version), compare, flags);
+	dep = pkgconf_dependency_addraw(client, list, package, strlen(package), version,
-
+					version != NULL ? strlen(version) : 0, compare, flags);
-	return pkgconf_dependency_addraw(client, list, package, strlen(package), NULL, 0, compare, flags);
+	return pkgconf_dependency_ref(dep->owner, dep);
 }
 
 /*
@@ -222,6 +229,7 @@ pkgconf_dependency_ref(pkgconf_client_t *client, pkgconf_dependency_t *dep)
 		return NULL;
 
 	dep->refcount++;
+	PKGCONF_TRACE(client, "%s refcount@%p: %d", dep->package, dep, dep->refcount);
 	return dep;
 }
 
@@ -242,7 +250,10 @@ pkgconf_dependency_unref(pkgconf_client_t *client, pkgconf_dependency_t *dep)
 	if (client != dep->owner)
 		return;
 
-	if (--dep->refcount <= 0)
+	--dep->refcount;
+	PKGCONF_TRACE(client, "%s refcount@%p: %d", dep->package, dep, dep->refcount);
+
+	if (dep->refcount <= 0)
 		pkgconf_dependency_free_one(dep);
 }
 
@@ -268,6 +279,8 @@ pkgconf_dependency_free(pkgconf_list_t *list)
 		pkgconf_node_delete(&dep->iter, list);
 		pkgconf_dependency_unref(dep->owner, dep);
 	}
+
+	pkgconf_list_zero(list);
 }
 
 /*

libpkgconf/pkg.c

@@ -451,7 +451,8 @@ pkgconf_pkg_new_from_file(pkgconf_client_t *client, const char *filename, FILE *
 		return NULL;
 	}
 
-	pkgconf_dependency_add(client, &pkg->provides, pkg->id, pkg->version, PKGCONF_CMP_EQUAL, 0);
+	pkgconf_dependency_t *dep = pkgconf_dependency_add(client, &pkg->provides, pkg->id, pkg->version, PKGCONF_CMP_EQUAL, 0);
+	pkgconf_dependency_unref(dep->owner, dep);
 
 	return pkgconf_pkg_ref(client, pkg);
 }
@@ -536,7 +537,7 @@ pkgconf_pkg_ref(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 		PKGCONF_TRACE(client, "WTF: client %p refers to package %p owned by other client %p", client, pkg, pkg->owner);
 
 	pkg->refcount++;
-	PKGCONF_TRACE(client, "refcount@%p: %d", pkg, pkg->refcount);
+	PKGCONF_TRACE(client, "%s refcount@%p: %d", pkg->id, pkg, pkg->refcount);
 
 	return pkg;
 }
@@ -559,7 +560,7 @@ pkgconf_pkg_unref(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 		PKGCONF_TRACE(client, "WTF: client %p unrefs package %p owned by other client %p", client, pkg, pkg->owner);
 
 	pkg->refcount--;
-	PKGCONF_TRACE(pkg->owner, "refcount@%p: %d", pkg, pkg->refcount);
+	PKGCONF_TRACE(pkg->owner, "%s refcount@%p: %d", pkg->id, pkg, pkg->refcount);
 
 	if (pkg->refcount <= 0)
 		pkgconf_pkg_free(pkg->owner, pkg);
@@ -1444,9 +1445,9 @@ pkgconf_pkg_walk_list(pkgconf_client_t *client,
 	unsigned int skip_flags)
 {
 	unsigned int eflags = PKGCONF_PKG_ERRF_OK;
-	pkgconf_node_t *node;
+	pkgconf_node_t *node, *next;
 
-	PKGCONF_FOREACH_LIST_ENTRY(deplist->head, node)
+	PKGCONF_FOREACH_LIST_ENTRY_SAFE(deplist->head, next, node)
 	{
 		unsigned int eflags_local = PKGCONF_PKG_ERRF_OK;
 		pkgconf_dependency_t *depnode = node->data;
@@ -1469,21 +1470,26 @@ pkgconf_pkg_walk_list(pkgconf_client_t *client,
 		if (pkgdep->serial == client->serial)
 		{
 			pkgdep->hits++;
-			pkgconf_pkg_unref(client, pkgdep);
+			/* In this case we have a circular reference.
-			continue;
+			 * We break that by deleteing the circular node from the
+			 * the list, so that we dont create a situation where
+			 * memory is leaked due to circular ownership.
+			 * i.e: A owns B owns A
+			 */
+			pkgconf_node_delete(node, deplist);
+			pkgconf_dependency_unref(client, depnode);
+			goto next;
 		}
 
 		if (skip_flags && (depnode->flags & skip_flags) == skip_flags)
-		{
+			goto next;
-			pkgconf_pkg_unref(client, pkgdep);
-			continue;
-		}
 
 		pkgconf_audit_log_dependency(client, pkgdep, depnode);
 
 		pkgdep->hits++;
 		pkgdep->serial = client->serial;
 		eflags |= pkgconf_pkg_traverse_main(client, pkgdep, func, data, depth - 1, skip_flags);
+next:
 		pkgconf_pkg_unref(client, pkgdep);
 	}
 

libpkgconf/queue.c

@@ -116,6 +116,8 @@ pkgconf_queue_collect_dependents(pkgconf_client_t *client, pkgconf_pkg_t *pkg, v
 	if (pkg == world)
 		return;
 
+	if (!(pkg->flags & PKGCONF_PKG_PKGF_SEARCH_PRIVATE))
+	{
 		PKGCONF_FOREACH_LIST_ENTRY(pkg->required.head, node)
 		{
 			pkgconf_dependency_t *flattened_dep;
@@ -124,7 +126,9 @@ pkgconf_queue_collect_dependents(pkgconf_client_t *client, pkgconf_pkg_t *pkg, v
 
 			pkgconf_node_insert(&flattened_dep->iter, flattened_dep, &world->required);
 		}
-
+	}
+	else
+	{
 		PKGCONF_FOREACH_LIST_ENTRY(pkg->requires_private.head, node)
 		{
 			pkgconf_dependency_t *flattened_dep;
@@ -133,6 +137,7 @@ pkgconf_queue_collect_dependents(pkgconf_client_t *client, pkgconf_pkg_t *pkg, v
 
 			pkgconf_node_insert(&flattened_dep->iter, flattened_dep, &world->requires_private);
 		}
+	}
 }
 
 static int
@@ -147,11 +152,11 @@ dep_sort_cmp(const void *a, const void *b)
 static inline void
 flatten_dependency_set(pkgconf_client_t *client, pkgconf_list_t *list)
 {
-	pkgconf_node_t *node;
+	pkgconf_node_t *node, *next;
 	pkgconf_dependency_t **deps = NULL;
 	size_t dep_count = 0, i;
 
-	PKGCONF_FOREACH_LIST_ENTRY(list->head, node)
+	PKGCONF_FOREACH_LIST_ENTRY_SAFE(list->head, next, node)
 	{
 		pkgconf_dependency_t *dep = node->data;
 		pkgconf_pkg_t *pkg = pkgconf_pkg_verify_dependency(client, dep, NULL);
@@ -160,7 +165,11 @@ flatten_dependency_set(pkgconf_client_t *client, pkgconf_list_t *list)
 			continue;
 
 		if (pkg->serial == client->serial)
-			continue;
+		{
+			pkgconf_node_delete(node, list);
+			pkgconf_dependency_unref(client, dep);
+			goto next;
+		}
 
 		if (dep->match == NULL)
 		{
@@ -190,10 +199,13 @@ flatten_dependency_set(pkgconf_client_t *client, pkgconf_list_t *list)
 		deps[dep_count - 1] = dep;
 
 		PKGCONF_TRACE(client, "added %s to dep table", dep->package);
-
+next:
-next:;
+		pkgconf_pkg_unref(client, pkg);
 	}
 
+	if (deps == NULL)
+		return;
+
 	qsort(deps, dep_count, sizeof (void *), dep_sort_cmp);
 
 	/* zero the list and start readding */
@@ -261,6 +273,7 @@ pkgconf_queue_verify(pkgconf_client_t *client, pkgconf_pkg_t *world, pkgconf_lis
 bool
 pkgconf_queue_apply(pkgconf_client_t *client, pkgconf_list_t *list, pkgconf_queue_apply_func_t func, int maxdepth, void *data)
 {
+	bool ret = false;
 	pkgconf_pkg_t world = {
 		.id = "virtual:world",
 		.realname = "virtual world package",
@@ -272,18 +285,17 @@ pkgconf_queue_apply(pkgconf_client_t *client, pkgconf_list_t *list, pkgconf_queu
 		maxdepth = -1;
 
 	if (pkgconf_queue_verify(client, &world, list, maxdepth) != PKGCONF_PKG_ERRF_OK)
-		return false;
+		goto cleanup;
 
 	/* the world dependency set is flattened after it is returned from pkgconf_queue_verify */
 	if (!func(client, &world, data, maxdepth))
-	{
+		goto cleanup;
-		pkgconf_pkg_free(client, &world);
-		return false;
-	}
 
-	pkgconf_pkg_free(client, &world);
+	ret = true;
 
-	return true;
+cleanup:
+	pkgconf_pkg_free(client, &world);
+	return ret;
 }
 
 /*