From: Job Snijders Subject: rpki-client: add error checks for pthread_* calls To: tech@openbsd.org Date: Fri, 20 Jun 2025 12:00:13 +0000 Perhaps good to add error checking to the pthread_* calls? While there, the rwlock_unlock in auth_insert() seemed somewhat superfluous given that the program errors out right after. OK? 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");