From: Theo Buehler Subject: Re: rpki-client: add error checks for pthread_* calls To: Philip Guenther Cc: David Gwynne , Job Snijders , tech@openbsd.org Date: Tue, 24 Jun 2025 10:12:30 +0200 On Tue, Jun 24, 2025 at 10:06:18AM +0200, Theo Buehler wrote: > On Tue, Jun 24, 2025 at 10:01:47AM +0200, Theo Buehler wrote: > > > or > > > errx(1, "pthread_create: %s", strerror(error)); > > > > Alright. I'm not going to do errc myself and if anyone really wants it, > > please go and fix portable first. > > Bah. Disregard. Need to use err. > This time with errx. I also added error checks for two cond_wait. Some checks were pulled out of the if clauses since there's no sensible wrapping possible. Index: cert.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v diff -u -p -r1.168 cert.c --- cert.c 24 Jun 2025 05:21:55 -0000 1.168 +++ cert.c 24 Jun 2025 07:48:39 -0000 @@ -1426,15 +1426,16 @@ auth_find(struct auth_tree *auths, int i { struct auth a, *f; struct cert c; + int error; c.certid = id; a.cert = &c; - if (pthread_rwlock_rdlock(&cert_lk) != 0) - errx(1, "pthread_rwlock_rdlock"); + if ((error = pthread_rwlock_rdlock(&cert_lk)) != 0) + errx(1, "pthread_rwlock_rdlock: %s", strerror(error)); f = RB_FIND(auth_tree, auths, &a); - if (pthread_rwlock_unlock(&cert_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&cert_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); return f; } @@ -1443,13 +1444,14 @@ auth_insert(const char *fn, struct auth_ struct auth *issuer) { struct auth *na; + int error; na = calloc(1, sizeof(*na)); if (na == NULL) err(1, NULL); - if (pthread_rwlock_wrlock(&cert_lk) != 0) - errx(1, "pthread_rwlock_wrlock"); + if ((error = pthread_rwlock_wrlock(&cert_lk)) != 0) + errx(1, "pthread_rwlock_wrlock: %s", strerror(error)); if (issuer == NULL) { cert->certid = cert->talid; } else { @@ -1474,14 +1476,14 @@ auth_insert(const char *fn, struct auth_ if (RB_INSERT(auth_tree, auths, na) != NULL) errx(1, "auth tree corrupted"); - if (pthread_rwlock_unlock(&cert_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&cert_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); return na; fail: - if (pthread_rwlock_unlock(&cert_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&cert_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); free(na); return NULL; } Index: crl.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v diff -u -p -r1.46 crl.c --- crl.c 23 Jun 2025 22:01:14 -0000 1.46 +++ crl.c 24 Jun 2025 07:39:57 -0000 @@ -305,6 +305,7 @@ struct crl * crl_get(struct crl_tree *crlt, const struct auth *a) { struct crl find, *crl; + int error; /* XXX - this should be removed, but filemode relies on it. */ if (a == NULL) @@ -313,24 +314,24 @@ crl_get(struct crl_tree *crlt, const str find.aki = a->cert->ski; find.mftpath = a->cert->mft; - if (pthread_rwlock_rdlock(&crl_lk) != 0) - errx(1, "pthread_rwlock_rdlock"); + if ((error = pthread_rwlock_rdlock(&crl_lk)) != 0) + errx(1, "pthread_rwlock_rdlock: %s", strerror(error)); crl = RB_FIND(crl_tree, crlt, &find); - if (pthread_rwlock_unlock(&crl_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&crl_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); return crl; } int crl_insert(struct crl_tree *crlt, struct crl *crl) { - int rv; + int error, rv; - if (pthread_rwlock_wrlock(&crl_lk) != 0) - errx(1, "pthread_rwlock_wrlock"); + if ((error = pthread_rwlock_wrlock(&crl_lk)) != 0) + errx(1, "pthread_rwlock_wrlock: %s", strerror(error)); rv = RB_INSERT(crl_tree, crlt, crl) == NULL; - if (pthread_rwlock_unlock(&crl_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&crl_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); return rv; } Index: parser.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v diff -u -p -r1.158 parser.c --- parser.c 23 Jun 2025 22:01:14 -0000 1.158 +++ parser.c 24 Jun 2025 08:04:15 -0000 @@ -75,12 +75,13 @@ static struct parse_repo * repo_get(unsigned int id) { struct parse_repo needle = { .id = id }, *r; + int error; - if (pthread_rwlock_rdlock(&repos_lk) != 0) - errx(1, "pthread_rwlock_rdlock"); + if ((error = pthread_rwlock_rdlock(&repos_lk)) != 0) + errx(1, "pthread_rwlock_rdlock: %s", strerror(error)); r = RB_FIND(repo_tree, &repos, &needle); - if (pthread_rwlock_unlock(&repos_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&repos_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); return r; } @@ -88,6 +89,7 @@ static void repo_add(unsigned int id, char *path, char *validpath) { struct parse_repo *rp; + int error; if ((rp = calloc(1, sizeof(*rp))) == NULL) err(1, NULL); @@ -99,12 +101,12 @@ repo_add(unsigned int id, char *path, ch if ((rp->validpath = strdup(validpath)) == NULL) err(1, NULL); - if (pthread_rwlock_wrlock(&repos_lk) != 0) - errx(1, "pthread_rwlock_wrlock"); + if ((error = pthread_rwlock_wrlock(&repos_lk)) != 0) + errx(1, "pthread_rwlock_wrlock: %s", strerror(error)); if (RB_INSERT(repo_tree, &repos, rp) != NULL) errx(1, "repository already added: id %d, %s", id, path); - if (pthread_rwlock_unlock(&repos_lk) != 0) - errx(1, "pthread_rwlock_unlock"); + if ((error = pthread_rwlock_unlock(&repos_lk)) != 0) + errx(1, "pthread_rwlock_unlock: %s", strerror(error)); } /* @@ -1072,7 +1074,7 @@ parse_worker(void *arg) struct ibufqueue *myq; X509_STORE_CTX *ctx; BN_CTX *bn_ctx; - int n; + int error, n; if ((ctx = X509_STORE_CTX_new()) == NULL) err(1, "X509_STORE_CTX_new"); @@ -1082,10 +1084,14 @@ parse_worker(void *arg) err(1, "ibufqueue_new"); while (!quit) { - if (pthread_mutex_lock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_lock"); - while (TAILQ_EMPTY(&globalq) && !quit) - pthread_cond_wait(&globalq_cond, &globalq_mtx); + if ((error = pthread_mutex_lock(&globalq_mtx)) != 0) + errx(1, "pthread_mutex_lock: %s", strerror(error)); + while (TAILQ_EMPTY(&globalq) && !quit) { + error = pthread_cond_wait(&globalq_cond, &globalq_mtx); + if (error != 0) + errx(1, "pthread_cond_wait: %s", + strerror(error)); + } n = 0; while ((entp = TAILQ_FIRST(&globalq)) != NULL) { TAILQ_REMOVE(&globalq, entp, entries); @@ -1093,23 +1099,30 @@ parse_worker(void *arg) if (n++ > 16) break; } - if (pthread_mutex_unlock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_unlock"); + if ((error = pthread_mutex_unlock(&globalq_mtx)) != 0) + errx(1, "pthread_mutex_unlock: %s", + strerror(error)); if (n > 16) { - if (pthread_cond_signal(&globalq_cond) != 0) - errx(1, "pthread_cond_signal"); + if ((error = pthread_cond_signal(&globalq_cond)) != 0) + errx(1, "pthread_cond_signal: %s", + strerror(error)); } parse_entity(&q, myq, ctx, bn_ctx); if (ibufq_queuelen(myq) > 0) { - if (pthread_mutex_lock(&globalmsgq_mtx) != 0) - errx(1, "pthread_mutex_lock"); + if ((error = pthread_mutex_lock(&globalmsgq_mtx)) != 0) + errx(1, "pthread_mutex_lock: %s", + strerror(error)); ibufq_concat(globalmsgq, myq); - if (pthread_cond_signal(&globalmsgq_cond) != 0) - errx(1, "pthread_cond_signal"); - if (pthread_mutex_unlock(&globalmsgq_mtx) != 0) - errx(1, "pthread_mutex_unlock"); + error = pthread_cond_signal(&globalmsgq_cond); + if (error != 0) + errx(1, "pthread_cond_signal: %s", + strerror(error)); + error = pthread_mutex_unlock(&globalmsgq_mtx); + if (error != 0) + errx(1, "pthread_mutex_unlock: %s", + strerror(error)); } } @@ -1124,21 +1137,30 @@ parse_writer(void *arg) { struct msgbuf *myq; struct pollfd pfd; + int error; if ((myq = msgbuf_new()) == NULL) err(1, NULL); pfd.fd = *(int *)arg; while (!quit) { if (msgbuf_queuelen(myq) == 0) { - if (pthread_mutex_lock(&globalmsgq_mtx) != 0) - errx(1, "pthread_mutex_lock"); - while (ibufq_queuelen(globalmsgq) == 0 && !quit) - pthread_cond_wait(&globalmsgq_cond, + error = pthread_mutex_lock(&globalmsgq_mtx); + if (error != 0) + errx(1, "pthread_mutex_lock: %s", + strerror(error)); + while (ibufq_queuelen(globalmsgq) == 0 && !quit) { + error = pthread_cond_wait(&globalmsgq_cond, &globalmsgq_mtx); + if (error != 0) + errx(1, "pthread_cond_wait: %s", + strerror(error)); + } /* enqueue messages to local msgbuf */ msgbuf_concat(myq, globalmsgq); - if (pthread_mutex_unlock(&globalmsgq_mtx) != 0) - errx(1, "pthread_mutex_lock"); + error = pthread_mutex_unlock(&globalmsgq_mtx); + if (error != 0) + errx(1, "pthread_mutex_lock: %s", + strerror(error)); } if (msgbuf_queuelen(myq) > 0) { @@ -1190,7 +1212,7 @@ proc_parser(int fd, int nthreads) struct entity *entp; struct ibuf *b; pthread_t writer, *workers; - int i; + int error, i; /* Only allow access to the cache directory. */ if (unveil(".", "r") == -1) @@ -1213,11 +1235,12 @@ proc_parser(int fd, int nthreads) if ((workers = calloc(nthreads, sizeof(*workers))) == NULL) err(1, NULL); - if (pthread_create(&writer, NULL, &parse_writer, &fd) != 0) - errx(1, "pthread_create"); + if ((error = pthread_create(&writer, NULL, &parse_writer, &fd)) != 0) + errx(1, "pthread_create: %s", strerror(error)); for (i = 0; i < nthreads; i++) { - if (pthread_create(&workers[i], NULL, &parse_worker, NULL) != 0) - errx(1, "pthread_create"); + error = pthread_create(&workers[i], NULL, &parse_worker, NULL); + if (error != 0) + errx(1, "pthread_create: %s", strerror(error)); } pfd.fd = fd; @@ -1262,37 +1285,44 @@ proc_parser(int fd, int nthreads) TAILQ_INSERT_TAIL(&myq, entp, entries); } if (!TAILQ_EMPTY(&myq)) { - if (pthread_mutex_lock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_lock"); + error = pthread_mutex_lock(&globalq_mtx); + if (error != 0) + errx(1, "pthread_mutex_lock: %s", + strerror(error)); TAILQ_CONCAT(&globalq, &myq, entries); - if (pthread_mutex_unlock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_unlock"); - if (pthread_cond_signal(&globalq_cond) != 0) - errx(1, "pthread_cond_signal"); + error = pthread_mutex_unlock(&globalq_mtx); + if (error != 0) + errx(1, "pthread_mutex_unlock: %s", + strerror(error)); + error = pthread_cond_signal(&globalq_cond); + if (error != 0) + errx(1, "pthread_cond_signal: %s", + strerror(error)); } } } /* signal all threads */ - if (pthread_cond_broadcast(&globalq_cond) != 0) - errx(1, "pthread_cond_broadcast"); - if (pthread_cond_broadcast(&globalmsgq_cond) != 0) - errx(1, "pthread_cond_broadcast"); + if ((error = pthread_cond_broadcast(&globalq_cond)) != 0) + errx(1, "pthread_cond_broadcast: %s", strerror(error)); + if ((error = pthread_cond_broadcast(&globalmsgq_cond)) != 0) + errx(1, "pthread_cond_broadcast: %s", strerror(error)); - if (pthread_mutex_lock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_lock"); + if ((error = pthread_mutex_lock(&globalq_mtx)) != 0) + errx(1, "pthread_mutex_lock: %s", strerror(error)); while ((entp = TAILQ_FIRST(&globalq)) != NULL) { TAILQ_REMOVE(&globalq, entp, entries); entity_free(entp); } - if (pthread_mutex_unlock(&globalq_mtx) != 0) - errx(1, "pthread_mutex_unlock"); + if ((error = pthread_mutex_unlock(&globalq_mtx)) != 0) + errx(1, "pthread_mutex_unlock: %s", strerror(error)); - if (pthread_join(writer, NULL) != 0) - errx(1, "pthread_join writer"); + if ((error = pthread_join(writer, NULL)) != 0) + errx(1, "pthread_join writer: %s", strerror(error)); for (i = 0; i < nthreads; i++) { - if (pthread_join(workers[i], NULL) != 0) - errx(1, "pthread_join worker %d", i); + if ((error = pthread_join(workers[i], NULL)) != 0) + errx(1, "pthread_join worker %d: %s", + i, strerror(error)); } free(workers); /* karl marx */