From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: clean up the mutexes, conditional variables, rwlock structures, and repos RB tree
To: Theo Buehler <tb@theobuehler.org>
Cc: Job Snijders <job@openbsd.org>, tech@openbsd.org
Date: Wed, 25 Jun 2025 17:23:18 +0200

On Wed, Jun 25, 2025 at 05:03:49PM +0200, Theo Buehler wrote:
> 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.

I agree with all of the above. On top of this cleanup trees with many
objects before exit will slow down the exit path.
 
> > 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);
> > 
> 

-- 
:wq Claudio