Download raw body.
rpki-client: add error checks for pthread_* calls
On Fri, Jun 20, 2025 at 03:51:42PM +0200, Theo Buehler wrote:
> On Fri, Jun 20, 2025 at 12:00:13PM +0000, Job Snijders wrote:
> > Perhaps good to add error checking to the pthread_* calls?
>
> Yeah, we should probably just consistently check all the pthread_*
> functions rather than trying to be smart. Also, we seem to be missing a
> bunch of pthread_*_destroy() calls.
>
> > While there, the rwlock_unlock in auth_insert() seemed somewhat
> > superfluous given that the program errors out right after.
>
> I have no opinion on this. Might help grepping to keep it, but I won't
> depend on it.
I would prefer to keep the unlock but it does indeed not matter. Also in
repo_add() this is not done so I was not consistent with this.
So you can choose.
> > OK?
>
> I'm fine with it but please wait for claudio.
The pthread api is just painful. Some stuff just can't fail (or should
blow up if held wrong). This is where I'm not sure if adding all those
checks will actually do any good. Didn't I mention that the pthread API is
painful...
Guess only adding some checks is not great, so OK lest add them all.
> >
> > Index: cert.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> > diff -u -p -r1.164 cert.c
> > --- cert.c 19 Jun 2025 11:06:48 -0000 1.164
> > +++ cert.c 20 Jun 2025 11:54:57 -0000
> > @@ -1426,9 +1426,11 @@ auth_find(struct auth_tree *auths, int i
> > c.certid = id;
> > a.cert = &c;
> >
> > - pthread_rwlock_rdlock(&cert_lk);
> > + if (pthread_rwlock_rdlock(&cert_lk) != 0)
> > + errx(1, "pthread_rwlock_rdlock");
> > f = RB_FIND(auth_tree, auths, &a);
> > - pthread_rwlock_unlock(&cert_lk);
> > + if (pthread_rwlock_unlock(&cert_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> > return f;
> > }
> >
> > @@ -1442,7 +1444,8 @@ auth_insert(const char *fn, struct auth_
> > if (na == NULL)
> > err(1, NULL);
> >
> > - pthread_rwlock_wrlock(&cert_lk);
> > + if (pthread_rwlock_wrlock(&cert_lk) != 0)
> > + errx(1, "pthread_rwlock_wrlock");
> > if (issuer == NULL) {
> > cert->certid = cert->talid;
> > } else {
> > @@ -1464,16 +1467,17 @@ auth_insert(const char *fn, struct auth_
> > na->cert = cert;
> > na->any_inherits = x509_any_inherits(cert->x509);
> >
> > - if (RB_INSERT(auth_tree, auths, na) != NULL) {
> > - pthread_rwlock_unlock(&cert_lk);
> > + if (RB_INSERT(auth_tree, auths, na) != NULL)
> > errx(1, "auth tree corrupted");
> > - }
> > - pthread_rwlock_unlock(&cert_lk);
> > +
> > + if (pthread_rwlock_unlock(&cert_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> >
> > return na;
> >
> > fail:
> > - pthread_rwlock_unlock(&cert_lk);
> > + if (pthread_rwlock_unlock(&cert_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> > free(na);
> > return NULL;
> > }
> > Index: crl.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> > diff -u -p -r1.45 crl.c
> > --- crl.c 4 Jun 2025 09:18:28 -0000 1.45
> > +++ crl.c 20 Jun 2025 11:54:57 -0000
> > @@ -313,9 +313,11 @@ crl_get(struct crl_tree *crlt, const str
> > find.aki = a->cert->ski;
> > find.mftpath = a->cert->mft;
> >
> > - pthread_rwlock_rdlock(&crl_lk);
> > + if (pthread_rwlock_rdlock(&crl_lk) != 0)
> > + errx(1, "pthread_rwlock_rdlock");
> > crl = RB_FIND(crl_tree, crlt, &find);
> > - pthread_rwlock_unlock(&crl_lk);
> > + if (pthread_rwlock_unlock(&crl_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> > return crl;
> > }
> >
> > @@ -324,9 +326,11 @@ crl_insert(struct crl_tree *crlt, struct
> > {
> > int rv;
> >
> > - pthread_rwlock_wrlock(&crl_lk);
> > + if (pthread_rwlock_wrlock(&crl_lk) != 0)
> > + errx(1, "pthread_rwlock_wrlock");
> > rv = RB_INSERT(crl_tree, crlt, crl) == NULL;
> > - pthread_rwlock_unlock(&crl_lk);
> > + if (pthread_rwlock_unlock(&crl_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> >
> > return rv;
> > }
> > Index: parser.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> > diff -u -p -r1.157 parser.c
> > --- parser.c 20 Jun 2025 05:00:01 -0000 1.157
> > +++ parser.c 20 Jun 2025 11:54:58 -0000
> > @@ -76,9 +76,11 @@ repo_get(unsigned int id)
> > {
> > struct parse_repo needle = { .id = id }, *r;
> >
> > - pthread_rwlock_rdlock(&repos_lk);
> > + if (pthread_rwlock_rdlock(&repos_lk) != 0)
> > + errx(1, "pthread_rwlock_rdlock");
> > r = RB_FIND(repo_tree, &repos, &needle);
> > - pthread_rwlock_unlock(&repos_lk);
> > + if (pthread_rwlock_unlock(&repos_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> > return r;
> > }
> >
> > @@ -97,10 +99,12 @@ repo_add(unsigned int id, char *path, ch
> > if ((rp->validpath = strdup(validpath)) == NULL)
> > err(1, NULL);
> >
> > - pthread_rwlock_wrlock(&repos_lk);
> > + if (pthread_rwlock_wrlock(&repos_lk) != 0)
> > + errx(1, "pthread_rwlock_wrlock");
> > if (RB_INSERT(repo_tree, &repos, rp) != NULL)
> > errx(1, "repository already added: id %d, %s", id, path);
> > - pthread_rwlock_unlock(&repos_lk);
> > + if (pthread_rwlock_unlock(&repos_lk) != 0)
> > + errx(1, "pthread_rwlock_unlock");
> > }
> >
> > /*
> > @@ -1078,7 +1082,8 @@ parse_worker(void *arg)
> > err(1, "ibufqueue_new");
> >
> > while (!quit) {
> > - pthread_mutex_lock(&globalq_mtx);
> > + if (pthread_mutex_lock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > while (TAILQ_EMPTY(&globalq) && !quit)
> > pthread_cond_wait(&globalq_cond, &globalq_mtx);
> > n = 0;
> > @@ -1088,17 +1093,23 @@ parse_worker(void *arg)
> > if (n++ > 16)
> > break;
> > }
> > - pthread_mutex_unlock(&globalq_mtx);
> > - if (n > 16)
> > - pthread_cond_signal(&globalq_cond);
> > + if (pthread_mutex_unlock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_unlock");
> > + if (n > 16) {
> > + if (pthread_cond_signal(&globalq_cond) != 0)
> > + errx(1, "pthread_cond_signal");
> > + }
> >
> > parse_entity(&q, myq, ctx, bn_ctx);
> >
> > if (ibufq_queuelen(myq) > 0) {
> > - pthread_mutex_lock(&globalmsgq_mtx);
> > + if (pthread_mutex_lock(&globalmsgq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > ibufq_concat(globalmsgq, myq);
> > - pthread_cond_signal(&globalmsgq_cond);
> > - pthread_mutex_unlock(&globalmsgq_mtx);
> > + if (pthread_cond_signal(&globalmsgq_cond) != 0)
> > + errx(1, "pthread_cond_signal");
> > + if (pthread_mutex_unlock(&globalmsgq_mtx) != 0)
> > + errx(1, "pthread_mutex_unlock");
> > }
> > }
> >
> > @@ -1119,13 +1130,15 @@ parse_writer(void *arg)
> > pfd.fd = *(int *)arg;
> > while (!quit) {
> > if (msgbuf_queuelen(myq) == 0) {
> > - pthread_mutex_lock(&globalmsgq_mtx);
> > + if (pthread_mutex_lock(&globalmsgq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > while (ibufq_queuelen(globalmsgq) == 0 && !quit)
> > pthread_cond_wait(&globalmsgq_cond,
> > &globalmsgq_mtx);
> > /* enqueue messages to local msgbuf */
> > msgbuf_concat(myq, globalmsgq);
> > - pthread_mutex_unlock(&globalmsgq_mtx);
> > + if (pthread_mutex_unlock(&globalmsgq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > }
> >
> > if (msgbuf_queuelen(myq) > 0) {
> > @@ -1200,9 +1213,12 @@ proc_parser(int fd, int nthreads)
> > if ((workers = calloc(nthreads, sizeof(*workers))) == NULL)
> > err(1, NULL);
> >
> > - pthread_create(&writer, NULL, &parse_writer, &fd);
> > - for (i = 0; i < nthreads; i++)
> > - pthread_create(&workers[i], NULL, &parse_worker, NULL);
> > + if (pthread_create(&writer, NULL, &parse_writer, &fd) != 0)
> > + errx(1, "pthread_create");
> > + for (i = 0; i < nthreads; i++) {
> > + if (pthread_create(&workers[i], NULL, &parse_worker, NULL) != 0)
> > + errx(1, "pthread_create");
> > + }
> >
> > pfd.fd = fd;
> > while (!quit) {
> > @@ -1246,24 +1262,31 @@ proc_parser(int fd, int nthreads)
> > TAILQ_INSERT_TAIL(&myq, entp, entries);
> > }
> > if (!TAILQ_EMPTY(&myq)) {
> > - pthread_mutex_lock(&globalq_mtx);
> > + if (pthread_mutex_lock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > TAILQ_CONCAT(&globalq, &myq, entries);
> > - pthread_mutex_unlock(&globalq_mtx);
> > - pthread_cond_signal(&globalq_cond);
> > + if (pthread_mutex_unlock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_unlock");
> > + if (pthread_cond_signal(&globalq_cond) != 0)
> > + errx(1, "pthread_cond_signal");
> > }
> > }
> > }
> >
> > /* signal all threads */
> > - pthread_cond_broadcast(&globalq_cond);
> > - pthread_cond_broadcast(&globalmsgq_cond);
> > + if (pthread_cond_broadcast(&globalq_cond) != 0)
> > + errx(1, "pthread_cond_broadcast");
> > + if (pthread_cond_broadcast(&globalmsgq_cond) != 0)
> > + errx(1, "pthread_cond_broadcast");
> >
> > - pthread_mutex_lock(&globalq_mtx);
> > + if (pthread_mutex_lock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_lock");
> > while ((entp = TAILQ_FIRST(&globalq)) != NULL) {
> > TAILQ_REMOVE(&globalq, entp, entries);
> > entity_free(entp);
> > }
> > - pthread_mutex_unlock(&globalq_mtx);
> > + if (pthread_mutex_unlock(&globalq_mtx) != 0)
> > + errx(1, "pthread_mutex_unlock");
> >
> > if (pthread_join(writer, NULL) != 0)
> > errx(1, "pthread_join writer");
> >
>
--
:wq Claudio
rpki-client: add error checks for pthread_* calls