On Wed, Jun 25, 2025 at 02:34:21PM +0000, Job Snijders wrote:
> Ensure the mutexes, conditional variables, rwlock structures, and repos
> RB tree are freed before exiting the parser subprocess.
> 
> My hope is that when the time comes we're wondering where the heck
> memory is leaking, we'll hopefully be more ready to find the issue at
> hand.

Not sure this is all that convincing an argument. Tooling like valgrind
and ASAN is good at distinguishing memory that is actually leaked and
memory that is still 'reachable' at process exit, e.g., hanging off a
global structure like these trees.

> Feedback? OK?

I guess... Either we do this for consistency or we drop all that freeing
in the exit path. If we go with this, I think the function should be
called repo_tree_free(). The others you touch aren't called auths_free()
and crlt_free() either.

... and since pthread stuff is an excellent reason for bikeshedding, I
can't resist pointing out that it looks slightly odd that we don't hold
the locks while we destroy the trees.

All that said, I see no real harm in this diff, so I'm ok with it.

> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.169 cert.c
> --- cert.c	24 Jun 2025 15:47:48 -0000	1.169
> +++ cert.c	25 Jun 2025 14:26:26 -0000
> @@ -1413,12 +1413,16 @@ void
>  auth_tree_free(struct auth_tree *auths)
>  {
>  	struct auth	*auth, *tauth;
> +	int		 error;
>  
>  	RB_FOREACH_SAFE(auth, auth_tree, auths, tauth) {
>  		RB_REMOVE(auth_tree, auths, auth);
>  		cert_free(auth->cert);
>  		free(auth);
>  	}
> +
> +	if ((error = pthread_rwlock_destroy(&cert_lk)) != 0)
> +		errx(1, "pthread_rwlock_destroy: %s", strerror(error));
>  }
>  
>  struct auth *
> Index: crl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> diff -u -p -r1.47 crl.c
> --- crl.c	24 Jun 2025 15:47:48 -0000	1.47
> +++ crl.c	25 Jun 2025 14:26:26 -0000
> @@ -351,9 +351,13 @@ void
>  crl_tree_free(struct crl_tree *crlt)
>  {
>  	struct crl	*crl, *tcrl;
> +	int		 error;
>  
>  	RB_FOREACH_SAFE(crl, crl_tree, crlt, tcrl) {
>  		RB_REMOVE(crl_tree, crlt, crl);
>  		crl_free(crl);
>  	}
> +
> +	if ((error = pthread_rwlock_destroy(&crl_lk)) != 0)
> +		errx(1, "pthread_rwlock_destroy: %s", strerror(error));
>  }
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> diff -u -p -r1.160 parser.c
> --- parser.c	24 Jun 2025 15:47:48 -0000	1.160
> +++ parser.c	25 Jun 2025 14:26:26 -0000
> @@ -1196,6 +1196,23 @@ parse_writer(void *arg)
>  	return NULL;
>  }
>  
> +static void
> +repos_tree_free(struct repo_tree *repos_tree)
> +{
> +	struct parse_repo *repo, *trepo;
> +	int error;
> +
> +	RB_FOREACH_SAFE(repo, repo_tree, repos_tree, trepo) {
> +		RB_REMOVE(repo_tree, repos_tree, repo);
> +		free(repo->path);
> +		free(repo->validpath);
> +		free(repo);
> +	}
> +
> +	if ((error = pthread_rwlock_destroy(&repos_lk)) != 0)
> +		errx(1, "pthread_rwlock_destroy: %s", strerror(error));
> +}
> +
>  /*
>   * Process responsible for parsing and validating content.
>   * All this process does is wait to be told about a file to parse, then
> @@ -1326,8 +1343,18 @@ proc_parser(int fd, int nthreads)
>  	}
>  	free(workers);	/* karl marx */
>  
> +	if ((error = pthread_cond_destroy(&globalq_cond)) != 0)
> +		errx(1, "pthread_cond_destroy: %s", strerror(error));
> +	if ((error = pthread_mutex_destroy(&globalq_mtx)) != 0)
> +		errx(1, "pthread_mutex_destroy: %s", strerror(error));
> +	if ((error = pthread_cond_destroy(&globalmsgq_cond)) != 0)
> +		errx(1, "pthread_cond_destroy: %s", strerror(error));
> +	if ((error = pthread_mutex_destroy(&globalmsgq_mtx)) != 0)
> +		errx(1, "pthread_mutex_destroy: %s", strerror(error));
> +
>  	auth_tree_free(&auths);
>  	crl_tree_free(&crlt);
> +	repos_tree_free(&repos);
>  
>  	msgbuf_free(inbufq);
>  	ibufq_free(globalmsgq);
>