Commit Diff

Commit:
41e722a1981bbd7e55fed6bdf1a258d2d53939ba
From:
Stefan Sperling <stsp@stsp.name>
Via:
Thomas Adam <thomas@xteddy.org>
Date:
Message:
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@
Actions:
Patch | Tree
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 */