commit 9925e94074a73e68c23232d9db686bdee44ebae6
from: Stefan Sperling <stsp@stsp.name>
date: Thu Jun 05 07:22:07 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 - d907758cbe632fc02ea32fa9363f1b427ed21c32
commit + 9925e94074a73e68c23232d9db686bdee44ebae6
blob - 2b2318d81f74d2c1bfb5dd384acc1b61b4d31c91
blob + 6a93d428b645fa7449b875471e7683b83ab8a090
--- 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 */