Index | Thread | Search

From:
Job Snijders <job@openbsd.org>
Subject:
rpki-client: add error checks for pthread_* calls
To:
tech@openbsd.org
Date:
Fri, 20 Jun 2025 12:00:13 +0000

Download raw body.

Thread
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");