Download raw body.
gotd: received unexpected privsep message
On Sun, Jun 01, 2025 at 09:44:23PM +0200, Martijn van Duren wrote:
> Hello all,
>
> After upgrading my gotd machine to 7.7 I noticed that I couldn't delete
> branches anymore. The weird part is that the problems disappear when
> running in extra verbose mode (-vv), either daemonized, or foreground.
>
> This could be triggered with both `got send -d <branch>`, and
> `git push origin --delete <branch>`.
>
> The "unexpected privsep message" seems to originate from lib/serve.c
> inside serve_write(), where inside the
> while (curstate != STATE_PACKFILE_RECEIVED) {
> loop a GOTD_IMSG_PACKFILE_STATUS is received. Adding the
> GOTD_IMSG_PACKFILE_STATUS case from the
> while (curstate != STATE_REFS_UPDATED && err == NULL) {
> loop seems to make gotd behave as expected.
Thanks a lot for digging into this. This is indeed a race, similar to
the one fixed for reading clients in 0.109. Except that this time the
race affects writing clients.
The patch below should fix this. It applies to the 'main' branch and
will conflict against 0.110 sources.
I will send a follow-up shortly with a patch that can be applied to 0.110.
Both gotd and gotsh need to be rebuilt and reinstalled. If only one of
them is updated then things will break badly.
To ensure both are rebuilt, run:
make server && make server-install
at the top level directory of the source tree.
If you intend to install binaries in /usr/local rather than in ~/bin,
also run 'make clean' first and add GOT_RELEASE=Yes to all further
invocations of make.
Patch against 'main' branch follows:
fix bogus "unexpected privsep message" from gotsh during "got send"
Ensure that gotsh receives its end of the pack file data pipe
before repo_write starts sending pack file status messages.
Messages of type GOTD_IMSG_PACKFILE_StATUS would end up being
received in gotsh's serve_write() function too early.
This race is similar to the one fixed for repo_read back in commit
c2274a511a7415078e2628f969b8459f69432411
Reported by martijn@, who pin-pointed the problematic case in the code, thanks!
M gotd/session_write.c | 48+ 20-
M lib/serve.c | 11+ 2-
2 files changed, 59 insertions(+), 22 deletions(-)
commit - 277ed191b926ae1e6b7ca037f3304903aa162828
commit + 11a27b57d54a5d2b41a1b2b1368311a05f38be53
blob - 21d505ad565f7c05cb872d5c7eda294089cb44db
blob + 2b2318d81f74d2c1bfb5dd384acc1b61b4d31c91
--- gotd/session_write.c
+++ gotd/session_write.c
@@ -91,6 +91,7 @@ static struct gotd_session_write {
size_t num_notification_refs_received;
struct got_pathlist_head *notification_refs_cur;
struct gotd_notification_targets notification_targets;
+ int repo_child_packfd;
} gotd_session;
static struct gotd_session_client {
@@ -1263,9 +1264,6 @@ static const struct got_error *
recv_packfile(struct gotd_session_client *client)
{
const struct got_error *err = NULL;
- struct gotd_imsg_recv_packfile ipack;
- char *basepath = NULL, *pack_path = NULL, *idx_path = NULL;
- int packfd = -1, idxfd = -1;
int pipe[2] = { -1, -1 };
if (client->packfile_path) {
@@ -1276,24 +1274,50 @@ recv_packfile(struct gotd_session_client *client)
if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pipe) == -1)
return got_error_from_errno("socketpair");
- /* Send pack pipe end 0 to repo child process. */
- if (gotd_imsg_compose_event(&gotd_session.repo_child_iev,
- GOTD_IMSG_PACKFILE_PIPE, GOTD_PROC_SESSION_WRITE, pipe[0],
- NULL, 0) == -1) {
- err = got_error_from_errno("imsg compose PACKFILE_PIPE");
- goto done;
- }
- pipe[0] = -1;
-
- /* Send pack pipe end 1 to gotsh(1) (expects just an fd, no data). */
+ /*
+ * Send pack data pipe end 0 to gotsh(1) (expects just an fd, no data).
+ *
+ * We will forward the other pipe end to the repo_write process only
+ * once we have confirmation that gotsh(1) has received its end.
+ */
if (gotd_imsg_compose_event(&client->iev,
- GOTD_IMSG_PACKFILE_PIPE, GOTD_PROC_SESSION_WRITE, pipe[1],
- NULL, 0) == -1) {
+ GOTD_IMSG_PACKFILE_PIPE, GOTD_PROC_GOTD, pipe[0], NULL, 0) == -1) {
err = got_error_from_errno("imsg compose PACKFILE_PIPE");
goto done;
}
+ pipe[0] = -1;
+
+ gotd_session.repo_child_packfd = pipe[1];
pipe[1] = -1;
+done:
+ if (pipe[0] != -1 && close(pipe[0]) == -1 && err == NULL)
+ err = got_error_from_errno("close");
+ if (pipe[1] != -1 && close(pipe[1]) == -1 && err == NULL)
+ err = got_error_from_errno("close");
+ return err;
+}
+static const struct got_error *
+send_packfds_to_repo_child(void)
+{
+ const struct got_error *err = NULL;
+ struct gotd_session_client *client = &gotd_session_client;
+ char *basepath = NULL, *pack_path = NULL, *idx_path = NULL;
+ int packfd = -1, idxfd = -1;
+ struct gotd_imsg_recv_packfile ipack;
+
+ /*
+ * gotsh(1) has received its end of the pack pipe.
+ * Send pack pipe end 1 to repo child process.
+ */
+ if (gotd_imsg_compose_event(
+ &gotd_session.repo_child_iev,
+ GOTD_IMSG_PACKFILE_PIPE, GOTD_PROC_SESSION_WRITE,
+ gotd_session.repo_child_packfd, NULL, 0) == -1)
+ return got_error_from_errno("imsg compose PACKFILE_PIPE");
+
+ gotd_session.repo_child_packfd = -1;
+
if (asprintf(&basepath, "%s/%s/receiving-from-uid-%d.pack",
got_repo_get_path(gotd_session.repo), GOT_OBJECTS_PACK_DIR,
client->euid) == -1) {
@@ -1344,13 +1368,8 @@ recv_packfile(struct gotd_session_client *client)
goto done;
}
packfd = -1;
-
done:
free(basepath);
- if (pipe[0] != -1 && close(pipe[0]) == -1 && err == NULL)
- err = got_error_from_errno("close");
- if (pipe[1] != -1 && close(pipe[1]) == -1 && err == NULL)
- err = got_error_from_errno("close");
if (packfd != -1 && close(packfd) == -1 && err == NULL)
err = got_error_from_errno("close");
if (idxfd != -1 && close(idxfd) == -1 && err == NULL)
@@ -1510,6 +1529,14 @@ session_dispatch_client(int fd, short events, void *ar
break;
}
break;
+ case GOTD_IMSG_PACKFILE_READY:
+ if (gotd_session.state != GOTD_STATE_EXPECT_PACKFILE ||
+ gotd_session.repo_child_packfd == -1) {
+ err = got_error(GOT_ERR_PRIVSEP_MSG);
+ break;
+ }
+ err = send_packfds_to_repo_child();
+ break;
default:
log_debug("unexpected imsg %d", imsg.hdr.type);
err = got_error(GOT_ERR_PRIVSEP_MSG);
@@ -1986,6 +2013,7 @@ session_write_main(const char *title, const char *repo
RB_INIT(&gotd_session.notification_refs);
RB_INIT(&gotd_session.notification_ref_namespaces);
STAILQ_INIT(&gotd_session.notification_targets);
+ gotd_session.repo_child_packfd = -1;
if (imsgbuf_init(&gotd_session.notifier_iev.ibuf, -1) == -1) {
err = got_error_from_errno("imsgbuf_init");
blob - a9c52c79856ada5a27ff4408e73766c864eb939c
blob + 50fce5761d857ada6ca3563695cf4e8bd2e5c89d
--- lib/serve.c
+++ lib/serve.c
@@ -1081,7 +1081,7 @@ done:
}
static const struct got_error *
-recv_packfile(struct imsg *imsg, int infd)
+recv_packfile(struct imsg *imsg, struct imsgbuf *ibuf, int infd)
{
const struct got_error *err = NULL;
size_t datalen;
@@ -1097,6 +1097,15 @@ recv_packfile(struct imsg *imsg, int infd)
if (packfd == -1)
return got_error(GOT_ERR_PRIVSEP_NO_FD);
+ if (imsg_compose(ibuf, GOTD_IMSG_PACKFILE_READY,
+ 0, 0, -1, NULL, 0) == -1) {
+ return got_error_from_errno("imsg_compose PACKFILE_READY");
+
+ }
+ err = gotd_imsg_flush(ibuf);
+ if (err)
+ return err;
+
while (!pack_done) {
ssize_t r = 0;
@@ -1341,7 +1350,7 @@ serve_write(int infd, int outfd, int gotd_sock, const
err = gotd_imsg_recv_error(NULL, &imsg);
goto done;
case GOTD_IMSG_PACKFILE_PIPE:
- err = recv_packfile(&imsg, infd);
+ err = recv_packfile(&imsg, &ibuf, infd);
if (err) {
if (err->code != GOT_ERR_EOF)
goto done;
gotd: received unexpected privsep message