main: unref another pkg

Which ensures that memory handling is done correctly.

.github/workflows/test.yml

@@ -53,7 +53,7 @@ jobs:
 
       - name: Build
         run: |
-          meson _build -Dwerror=true
+          meson _build
           meson compile -C _build
 
       - name: Run tests
@@ -122,7 +122,7 @@ jobs:
 
       - name: Build
         run: |
-          meson _build -Dwerror=true
+          meson _build
           meson compile -C _build
 
       - name: Run tests

.woodpecker.yml

@@ -4,7 +4,7 @@ pipeline:
     commands:
       - apt-get update
       - apt-get install -y kyua atf-sh build-essential meson
-      - meson _build -Dwerror=true
+      - meson _build
       - meson compile -C _build
       - meson test -v -C _build
     when:
@@ -45,7 +45,7 @@ pipeline:
     image: alpine
     commands:
       - apk add -U --no-cache kyua atf build-base meson
-      - meson _build -Dwerror=true
+      - meson _build
       - meson compile -C _build
       - meson test -v -C _build
     when:

cli/main.c

@@ -1162,7 +1162,7 @@ main(int argc, char *argv[])
 
 	if (required_module_version != NULL)
 	{
-		pkgconf_pkg_t *pkg;
+		pkgconf_pkg_t *pkg = NULL;
 		pkgconf_node_t *node;
 		pkgconf_list_t deplist = PKGCONF_LIST_INITIALIZER;
 
@@ -1202,7 +1202,7 @@ cleanup:
 	}
 	else if (required_exact_module_version != NULL)
 	{
-		pkgconf_pkg_t *pkg;
+		pkgconf_pkg_t *pkg = NULL;
 		pkgconf_node_t *node;
 		pkgconf_list_t deplist = PKGCONF_LIST_INITIALIZER;
 
@@ -1242,7 +1242,7 @@ cleanup2:
 	}
 	else if (required_max_module_version != NULL)
 	{
-		pkgconf_pkg_t *pkg;
+		pkgconf_pkg_t *pkg = NULL;
 		pkgconf_node_t *node;
 		pkgconf_list_t deplist = PKGCONF_LIST_INITIALIZER;
 

libpkgconf/cache.c

@@ -16,8 +16,6 @@
 #include <libpkgconf/stdinc.h>
 #include <libpkgconf/libpkgconf.h>
 
-#include <assert.h>
-
 /*
  * !doc
  *
@@ -155,10 +153,10 @@ 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)
+	if (pkg == NULL)
 		return;
 
-	if (pkg == NULL)
+	if (client->cache_table == NULL)
 		return;
 
 	if (!(pkg->flags & PKGCONF_PKG_PROPF_CACHED))
@@ -175,8 +173,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);
+	pkg->flags &= ~PKGCONF_PKG_PROPF_CACHED;
+	pkgconf_pkg_unref(client, pkg);
 	*slot = NULL;
 
 	qsort(client->cache_table, client->cache_count,
@@ -191,15 +189,14 @@ 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 *));
+	if (client->cache_count == 0) {
+		free(client->cache_table);
+		client->cache_table = NULL;
 	}
 	else
 	{
-		free(client->cache_table);
-		client->cache_table = NULL;
+		client->cache_table = pkgconf_reallocarray(client->cache_table,
+			client->cache_count, sizeof(void *));
 	}
 }
 
@@ -218,14 +215,11 @@ void
 pkgconf_cache_free(pkgconf_client_t *client)
 {
 	if (client->cache_table == NULL)
+		PKGCONF_TRACE(client, "cache is empty, not clearing");
 		return;
 
 	while (client->cache_count > 0)
 		pkgconf_cache_remove(client, client->cache_table[0]);
 
-	free(client->cache_table);
-	client->cache_table = NULL;
-	client->cache_count = 0;
-
 	PKGCONF_TRACE(client, "cleared package cache");
 }

libpkgconf/client.c

@@ -98,8 +98,6 @@ 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

@@ -16,6 +16,8 @@
 #include <libpkgconf/stdinc.h>
 #include <libpkgconf/libpkgconf.h>
 
+#include <assert.h>
+
 /*
  * !doc
  *
@@ -115,14 +117,7 @@ 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, pkgconf_dependency_ref(dep->owner, dep), list);
 
-	/* 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;
+	return pkgconf_dependency_ref(dep->owner, dep);
 }
 
 static inline pkgconf_dependency_t *
@@ -163,10 +158,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)
 {
-	pkgconf_dependency_t *dep;
-	dep = pkgconf_dependency_addraw(client, list, package, strlen(package), version,
-					version != NULL ? strlen(version) : 0, compare, flags);
-	return pkgconf_dependency_ref(dep->owner, dep);
+	if (version != NULL)
+		return pkgconf_dependency_addraw(client, list, package, strlen(package), version, strlen(version), compare, flags);
+
+	return pkgconf_dependency_addraw(client, list, package, strlen(package), NULL, 0, compare, flags);
 }
 
 /*
@@ -202,6 +197,9 @@ pkgconf_dependency_free_one(pkgconf_dependency_t *dep)
 	if (dep->match != NULL)
 		pkgconf_pkg_unref(dep->match->owner, dep->match);
 
+	if (dep->parent != NULL)
+		pkgconf_pkg_unref(dep->match->owner, dep->parent);
+
 	if (dep->package != NULL)
 		free(dep->package);
 
@@ -229,7 +227,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);
+	PKGCONF_TRACE(client, "%s: refcount@%p: %d", dep->package, dep, dep->refcount);
 	return dep;
 }
 
@@ -250,10 +248,10 @@ pkgconf_dependency_unref(pkgconf_client_t *client, pkgconf_dependency_t *dep)
 	if (client != dep->owner)
 		return;
 
-	--dep->refcount;
-	PKGCONF_TRACE(client, "%s refcount@%p: %d", dep->package, dep, dep->refcount);
-
-	if (dep->refcount <= 0)
+	dep->refcount--;
+	assert(dep->refcount >= 0);
+	PKGCONF_TRACE(client, "%s: refcount@%p: %d", dep->package, dep, dep->refcount);
+	if (dep->refcount == 0)
 		pkgconf_dependency_free_one(dep);
 }
 
@@ -279,8 +277,6 @@ pkgconf_dependency_free(pkgconf_list_t *list)
 		pkgconf_node_delete(&dep->iter, list);
 		pkgconf_dependency_unref(dep->owner, dep);
 	}
-
-	pkgconf_list_zero(list);
 }
 
 /*
@@ -367,7 +363,8 @@ pkgconf_dependency_parse_str(pkgconf_client_t *client, pkgconf_list_t *deplist_h
 
 			if (state == OUTSIDE_MODULE)
 			{
-				pkgconf_dependency_addraw(client, deplist_head, package, package_sz, NULL, 0, compare, flags);
+				pkgconf_dependency_t *dep = pkgconf_dependency_addraw(client, deplist_head, package, package_sz, NULL, 0, compare, flags);
+				pkgconf_dependency_unref(dep->owner, dep);
 
 				compare = PKGCONF_CMP_ANY;
 				package_sz = 0;
@@ -411,7 +408,8 @@ pkgconf_dependency_parse_str(pkgconf_client_t *client, pkgconf_list_t *deplist_h
 				version_sz = ptr - vstart;
 				state = OUTSIDE_MODULE;
 
-				pkgconf_dependency_addraw(client, deplist_head, package, package_sz, version, version_sz, compare, flags);
+				pkgconf_dependency_t *dep = pkgconf_dependency_addraw(client, deplist_head, package, package_sz, version, version_sz, compare, flags);
+				pkgconf_dependency_unref(dep->owner, dep);
 
 				compare = PKGCONF_CMP_ANY;
 				cnameptr = cmpname;

libpkgconf/libpkgconf.h

@@ -298,11 +298,11 @@ PKGCONF_API bool pkgconf_default_error_handler(const char *msg, const pkgconf_cl
 #if defined(__GNUC__) || defined(__INTEL_COMPILER)
 #define PKGCONF_TRACE(client, ...) do { \
 		pkgconf_trace(client, __FILE__, __LINE__, __PRETTY_FUNCTION__, __VA_ARGS__); \
-	} while (0)
+	} while (0);
 #else
 #define PKGCONF_TRACE(client, ...) do { \
 		pkgconf_trace(client, __FILE__, __LINE__, __func__, __VA_ARGS__); \
-	} while (0)
+	} while (0);
 #endif
 #else
 #define PKGCONF_TRACE(client, ...)

libpkgconf/pkg.c

@@ -17,6 +17,8 @@
 #include <libpkgconf/stdinc.h>
 #include <libpkgconf/libpkgconf.h>
 
+#include <assert.h>
+
 /*
  * !doc
  *
@@ -515,6 +517,8 @@ pkgconf_pkg_free(pkgconf_client_t *client, pkgconf_pkg_t *pkg)
 	if (pkg->pc_filedir != NULL)
 		free(pkg->pc_filedir);
 
+	/* We shouldn't be trying to free this if someone is still holding a reference to it */
+	assert(pkg->refcount == 0);
 	free(pkg);
 }
 
@@ -537,7 +541,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, "%s refcount@%p: %d", pkg->id, pkg, pkg->refcount);
+	PKGCONF_TRACE(client, "%s: refcount@%p: %d", pkg->id, pkg, pkg->refcount);
 
 	return pkg;
 }
@@ -560,7 +564,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, "%s refcount@%p: %d", pkg->id, 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);
@@ -1445,9 +1449,9 @@ pkgconf_pkg_walk_list(pkgconf_client_t *client,
 	unsigned int skip_flags)
 {
 	unsigned int eflags = PKGCONF_PKG_ERRF_OK;
-	pkgconf_node_t *node, *next;
+	pkgconf_node_t *node;
 
-	PKGCONF_FOREACH_LIST_ENTRY_SAFE(deplist->head, next, node)
+	PKGCONF_FOREACH_LIST_ENTRY(deplist->head, node)
 	{
 		unsigned int eflags_local = PKGCONF_PKG_ERRF_OK;
 		pkgconf_dependency_t *depnode = node->data;
@@ -1470,26 +1474,21 @@ pkgconf_pkg_walk_list(pkgconf_client_t *client,
 		if (pkgdep->serial == client->serial)
 		{
 			pkgdep->hits++;
-			/* In this case we have a circular reference.
-			 * 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;
+			pkgconf_pkg_unref(client, pkgdep);
+			continue;
 		}
 
 		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,7 +116,17 @@ 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))
+	if ((pkg->flags & PKGCONF_PKG_PROPF_STATIC) == PKGCONF_PKG_PROPF_STATIC) {
+		PKGCONF_FOREACH_LIST_ENTRY(pkg->requires_private.head, node)
+		{
+			pkgconf_dependency_t *flattened_dep;
+
+			flattened_dep = pkgconf_dependency_copy(client, node->data);
+
+			pkgconf_node_insert(&flattened_dep->iter, flattened_dep, &world->requires_private);
+		}
+	}
+	else
 	{
 		PKGCONF_FOREACH_LIST_ENTRY(pkg->required.head, node)
 		{
@@ -127,17 +137,6 @@ 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;
-
-			flattened_dep = pkgconf_dependency_copy(client, node->data);
-
-			pkgconf_node_insert(&flattened_dep->iter, flattened_dep, &world->requires_private);
-		}
-	}
 }
 
 static int
@@ -166,8 +165,8 @@ flatten_dependency_set(pkgconf_client_t *client, pkgconf_list_t *list)
 
 		if (pkg->serial == client->serial)
 		{
-			pkgconf_node_delete(node, list);
-			pkgconf_dependency_unref(client, dep);
+			PKGCONF_TRACE(client, "dedup %s serials match", pkg->id);
+			pkgconf_dependency_unref(dep->owner, dep);
 			goto next;
 		}
 
@@ -187,6 +186,7 @@ flatten_dependency_set(pkgconf_client_t *client, pkgconf_list_t *list)
 			if (!strcmp(dep->package, other_dep->package))
 			{
 				PKGCONF_TRACE(client, "skipping, %zu deps", dep_count);
+				pkgconf_dependency_unref(dep->owner, dep);
 				goto next;
 			}
 		}
@@ -199,6 +199,7 @@ 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:
 		pkgconf_pkg_unref(client, pkg);
 	}
@@ -273,27 +274,31 @@ 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",
 		.flags = PKGCONF_PKG_PROPF_STATIC | PKGCONF_PKG_PROPF_VIRTUAL,
 	};
+	bool ret = false;
 
 	/* if maxdepth is one, then we will not traverse deeper than our virtual package. */
 	if (!maxdepth)
 		maxdepth = -1;
 
 	if (pkgconf_queue_verify(client, &world, list, maxdepth) != PKGCONF_PKG_ERRF_OK)
-		goto cleanup;
+	{
+		goto out;
+	}
 
 	/* the world dependency set is flattened after it is returned from pkgconf_queue_verify */
 	if (!func(client, &world, data, maxdepth))
-		goto cleanup;
+	{
+		goto out;
+	}
+
 
 	ret = true;
-
-cleanup:
+out:
 	pkgconf_pkg_free(client, &world);
 	return ret;
 }

meson.build

@@ -1,37 +1,27 @@
 project('pkgconf', 'c',
   version : '1.8.0',
   license : 'ISC',
-  meson_version : '>=0.47',
-  default_options : ['c_std=c99'],
-)
+  meson_version : '>=0.47')
+
 
 cc = meson.get_compiler('c')
 
-add_project_arguments(
-  '-D_BSD_SOURCE',
-  '-D_DEFAULT_SOURCE',
-  cc.get_supported_arguments(
-    '-Wimplicit-function-declaration',
-    '-Wmisleading-indentation',
-  ),
-  language : 'c',
-)
 
 cdata = configuration_data()
 
 check_functions = [
-  ['strlcat', 'string.h'],
-  ['strlcpy', 'string.h'],
-  ['strndup', 'string.h'],
-  ['strdup', 'string.h'],
-  ['strncasecmp', 'strings.h'],
-  ['strcasecmp', 'strings.h'],
-  ['reallocarray', 'stdlib.h'],
+  ['HAVE_STRLCAT', 'strlcat', 'string.h'],
+  ['HAVE_STRLCPY', 'strlcpy', 'string.h'],
+  ['HAVE_STRNDUP', 'strndup', 'string.h'],
+  ['HAVE_STRDUP', 'strdup', 'string.h'],
+  ['HAVE_STRNCASECMP', 'strncasecmp', 'strings.h'],
+  ['HAVE_STRCASECMP', 'strcasecmp', 'strings.h'],
+  ['HAVE_REALLOCARRAY', 'reallocarray', 'stdlib.h'],
 ]
 
 foreach f : check_functions
-  if cc.has_function(f[0], prefix : '#define _BSD_SOURCE\n#include <@0@>'.format(f[1])) and cc.has_header_symbol(f[1], f[0], prefix : '#define _BSD_SOURCE')
-    cdata.set('HAVE_@0@'.format(f[0].to_upper().underscorify()), 1)
+  if cc.has_function(f.get(1), prefix : '#define _BSD_SOURCE\n#include <' + f.get(2) + '>') and cc.has_header_symbol(f.get(2), f.get(1), prefix : '#define _BSD_SOURCE')
+    cdata.set(f.get(0), 1)
   endif
 endforeach
 
@@ -52,8 +42,8 @@ cdata.set_quoted('PERSONALITY_PATH', ':'.join(personality_path))
 cdata.set_quoted('PACKAGE_NAME', meson.project_name())
 cdata.set_quoted('PACKAGE_VERSION', meson.project_version())
 cdata.set_quoted('PACKAGE_BUGREPORT', 'https://todo.sr.ht/~kaniini/pkgconf')
-cdata.set('abs_top_srcdir', meson.current_source_dir())
-cdata.set('abs_top_builddir', meson.current_build_dir())
+cdata.set('abs_top_srcdir', meson.source_root())
+cdata.set('abs_top_builddir', meson.build_root())
 
 
 subdir('libpkgconf')
@@ -120,8 +110,10 @@ pkgconf_exe = executable('pkgconf',
 if get_option('tests')
   kyua_exe = find_program('kyua')
   atf_sh_exe = find_program('atf-sh')
-  kyuafile = configure_file(input : 'Kyuafile.in', output : 'Kyuafile', configuration : cdata)
-  test('kyua', kyua_exe, args : ['--config=none', 'test', '--kyuafile', kyuafile, '--build-root', meson.current_build_dir()])
+  test('kyua', kyua_exe, args : ['--config=none', 'test', '--kyuafile=' + join_paths(meson.build_root(), 'Kyuafile'), '--build-root=' + meson.build_root()])
+
+
+  configure_file(input : 'Kyuafile.in', output : 'Kyuafile', configuration : cdata)
   subdir('tests')
 endif
 

tests/meson.build

@@ -18,6 +18,6 @@ tests = [
 
 # yuck
 foreach test : tests
-  test_file = configure_file(input: test + '.sh', output: test, copy: true)
-  run_command('chmod', '755', test_file, check : true)
+  configure_file(input: test + '.sh', output: test, copy: true)
+  run_command('chmod', '755', join_paths(meson.build_root(), 'tests', test))
 endforeach