commit 41e722a1981bbd7e55fed6bdf1a258d2d53939ba
from: Stefan Sperling <stsp@stsp.name>
via: Thomas Adam <thomas@xteddy.org>
date: Sat Jun 21 09:24:15 2025 UTC

fix a race in gotd notification processing

Dequeue notifications from the list only once the notify process has
confirmed that the notification has been sent. This should be more
robust for the following reason:

In some spots were are checking the list head against NULL to see if
any notifications are still pending. By removing notifications from
this list too early, this check did not cover notifications which were
still in flight. We could thus end up deciding to shut down the session
process too early, and the last notification of several would fail.

Issue observed when sending a new commit on a branch and a new tag
which tags this commit to gotd in a single 'got send' operation.
Only the tag notification would make it, while the other notification
never arrived after an 'unexpected EOF' error in the notify process
while trying to send a confirmation to the session process.

ok op@

commit - 689eb4525dcbcd6868eee875279ca6173dc3e891
commit + 41e722a1981bbd7e55fed6bdf1a258d2d53939ba
blob - 1930c2bd5b9cf2e9b5dd501b85080c116e05653c
blob + b56279a1bfaebe0073d471a105a7b4a45a6df6ab
--- gotd/session_write.c
+++ gotd/session_write.c
@@ -661,8 +661,6 @@ forward_notification(struct gotd_session_client *clien
 	notif = STAILQ_FIRST(&notifications);
 	if (notif == NULL)
 		return got_error(GOT_ERR_PRIVSEP_MSG);
-
-	STAILQ_REMOVE(&notifications, notif, gotd_session_notif, entry);
 
 	if (notif->action != icontent.action || notif->fd == -1 ||
 	    strcmp(notif->refname, refname) != 0) {
@@ -746,9 +744,6 @@ forward_notification(struct gotd_session_client *clien
 	imsg_close(&iev->ibuf, wbuf);
 	gotd_imsg_event_add(iev);
 done:
-	if (notif->fd != -1)
-		close(notif->fd);
-	free(notif);
 	free(refname);
 	free(id_str);
 	return err;
@@ -980,6 +975,33 @@ recv_notification_content(struct imsg *imsg)
 	if (datalen < sizeof(inotif))
 		return got_error(GOT_ERR_PRIVSEP_LEN);
 	memcpy(&inotif, imsg->data, sizeof(inotif));
+
+	return NULL;
+}
+
+static const struct got_error *
+report_ref_updates(int do_notify)
+{
+	const struct got_error *err;
+	struct gotd_session_notif *notif;
+
+	err = send_refs_updated(&gotd_session.parent_iev);
+	if (err) {
+		log_warn("%s", err->msg);
+		return err;
+	}
+
+	if (do_notify) {
+		notif = STAILQ_FIRST(&notifications);
+		if (notif == NULL)
+			return NULL;
+
+		err = request_notification(notif);
+		if (err) {
+			log_warn("could not send notification: %s", err->msg);
+			return err;
+		}
+	}
 
 	return NULL;
 }
@@ -1086,8 +1108,6 @@ session_dispatch_repo_child(int fd, short event, void 
 			else
 				disconnect(client);
 		} else {
-			struct gotd_session_notif *notif;
-
 			if (do_packfile_verification) {
 				err = verify_packfile(iev);
 			} else if (do_content_verification) {
@@ -1110,27 +1130,17 @@ session_dispatch_repo_child(int fd, short event, void 
 			if (do_ref_update && client->nref_updates > 0) {
 				client->nref_updates--;
 				if (client->nref_updates == 0) {
-					err = send_refs_updated(
-					    &gotd_session.parent_iev);
-					if (err) {
-						log_warn("%s", err->msg);
+					err = report_ref_updates(do_notify);
+					if (err)
 						shut = 1;
-					}
-				}
-			}
-
-			notif = STAILQ_FIRST(&notifications);
-			if (notif && do_notify) {
-				/* Request content for next notification. */
-				err = request_notification(notif);
-				if (err) {
-					log_warn("could not send notification: "
-					    "%s", err->msg);
-					shut = 1;
 				}
 			}
 		}
+
 		imsg_free(&imsg);
+
+		if (err)
+			break;
 	}
 done:
 	if (!shut) {
@@ -1668,7 +1678,15 @@ session_dispatch_notifier(int fd, short event, void *a
 				log_warn("unexpected imsg %d", imsg.hdr.type);
 				break;
 			}
+
+			/* First notification on our list has now been sent. */
 			notif = STAILQ_FIRST(&notifications);
+			STAILQ_REMOVE_HEAD(&notifications, entry);
+			if (notif->fd != -1)
+				close(notif->fd);
+			free(notif);
+
+			notif = STAILQ_FIRST(&notifications);
 			if (notif == NULL) {
 				disconnect(client);
 				break; /* NOTREACHED */