"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
gotd: fix "bad packfile with zero objects" error
To:
gameoftrees@openbsd.org
Date:
Wed, 18 Jan 2023 20:13:11 +0100

Download raw body.

Thread
Fix the "bad packfile with zero objects" error while creating branches,
as reported by op@ on IRC.

Clients will send an empty pack file if they are only creating new
references and have no objects to upload. Make gotd handle this and
add a regression test which triggers the bug.

The new regression test caught an unrelated issue where the client
connection was left lingering after references had been updated,
which made 'got send' followed by 'got clone -l' fail with the
connection limit configured for the test suite (just one connection
is allowed at a time). Fix this as well.
 
diff fecfd5bc4d412263e1178f9b6edf69709ea6e273 d3b9316ed2dc688af712256ea20f15d43c03c18f
commit - fecfd5bc4d412263e1178f9b6edf69709ea6e273
commit + d3b9316ed2dc688af712256ea20f15d43c03c18f
blob - 7f653f3ebb5d4da89d29058699af3f90cdb7baca
blob + a1082a01690cd74692583d348692d92175800c69
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -89,6 +89,7 @@ static struct repo_write_client {
 	int				 packidx_fd;
 	struct gotd_ref_updates		 ref_updates;
 	int				 nref_updates;
+	int				 nref_new;
 } repo_write_client;
 
 static volatile sig_atomic_t sigint_received;
@@ -257,6 +258,7 @@ list_refs(struct imsg *imsg)
 	client->pack_pipe = -1;
 	client->packidx_fd = -1;
 	client->nref_updates = 0;
+	client->nref_new = 0;
 
 	imsg_init(&ibuf, client_fd);
 
@@ -370,6 +372,7 @@ recv_ref_update(struct imsg *imsg)
 		if (err)
 			goto done;
 		ref_update->ref_is_new = 1;
+		client->nref_new++;
 	}
 	if (got_ref_is_symbolic(ref)) {
 		err = got_error_fmt(GOT_ERR_BAD_REF_TYPE,
@@ -685,12 +688,14 @@ recv_packdata(off_t *outsize, uint8_t *sha1, int infd,
 }
 
 static const struct got_error *
-recv_packdata(off_t *outsize, uint8_t *sha1, int infd, int outfd)
+recv_packdata(off_t *outsize, uint32_t *nobj, uint8_t *sha1,
+    int infd, int outfd)
 {
 	const struct got_error *err;
+	struct repo_write_client *client = &repo_write_client;
 	struct got_packfile_hdr hdr;
 	size_t have;
-	uint32_t nobj, nhave = 0;
+	uint32_t nhave = 0;
 	SHA1_CTX ctx;
 	uint8_t expected_sha1[SHA1_DIGEST_LENGTH];
 	char hex[SHA1_DIGEST_STRING_LENGTH];
@@ -699,6 +704,7 @@ recv_packdata(off_t *outsize, uint8_t *sha1, int infd,
 	ssize_t w;
 
 	*outsize = 0;
+	*nobj = 0;
 	SHA1Init(&ctx);
 
 	err = got_poll_read_full(infd, &have, &hdr, sizeof(hdr), sizeof(hdr));
@@ -715,12 +721,21 @@ recv_packdata(off_t *outsize, uint8_t *sha1, int infd,
 		return got_error_msg(GOT_ERR_BAD_PACKFILE,
 		    "bad packfile version");
 
-	nobj = be32toh(hdr.nobjects);
-	if (nobj == 0)
+	*nobj = be32toh(hdr.nobjects);
+	if (*nobj == 0) {
+		/*
+		 * Clients which are creating new references only
+		 * will send us an empty pack file.
+		 */
+		if (client->nref_updates > 0 &&
+		    client->nref_updates == client->nref_new)
+			return NULL;
+
 		return got_error_msg(GOT_ERR_BAD_PACKFILE,
 		    "bad packfile with zero objects");
+	}
 
-	log_debug("expecting %d objects", nobj);
+	log_debug("expecting %d objects", *nobj);
 
 	err = got_pack_hwrite(outfd, &hdr, sizeof(hdr), &ctx);
 	if (err)
@@ -730,7 +745,7 @@ recv_packdata(off_t *outsize, uint8_t *sha1, int infd,
 	if (err)
 		return err;
 
-	while (nhave != nobj) {
+	while (nhave != *nobj) {
 		uint8_t obj_type;
 		uint64_t obj_size;
 
@@ -762,7 +777,7 @@ recv_packdata(off_t *outsize, uint8_t *sha1, int infd,
 		nhave++;
 	}
 
-	log_debug("received %u objects", nobj);
+	log_debug("received %u objects", *nobj);
 
 	SHA1Final(expected_sha1, &ctx);
 
@@ -859,7 +874,7 @@ recv_packfile(struct imsg *imsg)
 }
 
 static const struct got_error *
-recv_packfile(struct imsg *imsg)
+recv_packfile(int *have_packfile, struct imsg *imsg)
 {
 	const struct got_error *err = NULL, *unpack_err;
 	struct repo_write_client *client = &repo_write_client;
@@ -875,9 +890,11 @@ recv_packfile(struct imsg *imsg)
 	struct got_ratelimit rl;
 	struct got_pack *pack = NULL;
 	off_t pack_filesize = 0;
+	uint32_t nobj = 0;
 
 	log_debug("packfile request received");
 
+	*have_packfile = 0;
 	got_ratelimit_init(&rl, 2, 0);
 
 	datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
@@ -928,8 +945,8 @@ recv_packfile(struct imsg *imsg)
 		goto done;
 
 	log_debug("receiving pack data");
-	unpack_err = recv_packdata(&pack_filesize, client->pack_sha1,
-	    client->pack_pipe, pack->fd);
+	unpack_err = recv_packdata(&pack_filesize, &nobj,
+	    client->pack_sha1, client->pack_pipe, pack->fd);
 	if (ireq.report_status) {
 		err = report_pack_status(unpack_err);
 		if (err) {
@@ -945,7 +962,18 @@ recv_packfile(struct imsg *imsg)
 
 	log_debug("pack data received");
 
+	/*
+	 * Clients which are creating new references only will
+	 * send us an empty pack file.
+	 */
+	if (nobj == 0 &&
+	    pack_filesize == sizeof(struct got_packfile_hdr) &&
+	    client->nref_updates > 0 &&
+	    client->nref_updates == client->nref_new)
+		goto done;
+
 	pack->filesize = pack_filesize;
+	*have_packfile = 1;
 
 	log_debug("begin indexing pack (%lld bytes in size)",
 	    (long long)pack->filesize);
@@ -1226,7 +1254,7 @@ repo_write_dispatch_session(int fd, short event, void 
 	struct imsg imsg;
 	struct repo_write_client *client = &repo_write_client;
 	ssize_t n;
-	int shut = 0;
+	int shut = 0, have_packfile = 0;
 
 	if (event & EV_READ) {
 		if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -1285,24 +1313,26 @@ repo_write_dispatch_session(int fd, short event, void 
 			}
 			break;
 		case GOTD_IMSG_RECV_PACKFILE:
-			err = recv_packfile(&imsg);
+			err = recv_packfile(&have_packfile, &imsg);
 			if (err) {
 				log_warnx("%s: receive packfile: %s",
 				    repo_write.title, err->msg);
 				break;
 			}
-			err = verify_packfile();
-			if (err) {
-				log_warnx("%s: verify packfile: %s",
-				    repo_write.title, err->msg);
-				break;
+			if (have_packfile) {
+				err = verify_packfile();
+				if (err) {
+					log_warnx("%s: verify packfile: %s",
+					    repo_write.title, err->msg);
+					break;
+				}
+				err = install_packfile(iev);
+				if (err) {
+					log_warnx("%s: install packfile: %s",
+					    repo_write.title, err->msg);
+					break;
+				}
 			}
-			err = install_packfile(iev);
-			if (err) {
-				log_warnx("%s: install packfile: %s",
-				    repo_write.title, err->msg);
-				break;
-			}
 			err = update_refs(iev);
 			if (err) {
 				log_warnx("%s: update refs: %s",
blob - c32e0bf5066a6f1b2b11a59ed1e4200d990d37ba
blob + 0bc48fac834b679822e3314db38590b9da8f3575
--- gotd/session.c
+++ gotd/session.c
@@ -394,8 +394,8 @@ update_ref(struct gotd_session_client *client, const c
 }
 
 static const struct got_error *
-update_ref(struct gotd_session_client *client, const char *repo_path,
-    struct imsg *imsg)
+update_ref(int *shut, struct gotd_session_client *client,
+    const char *repo_path, struct imsg *imsg)
 {
 	const struct got_error *err = NULL;
 	struct got_repository *repo = NULL;
@@ -495,8 +495,10 @@ done:
 
 	if (client->nref_updates > 0) {
 		client->nref_updates--;
-		if (client->nref_updates == 0)
+		if (client->nref_updates == 0) {
 			send_refs_updated(client);
+			*shut = 1;
+		}
 
 	}
 	if (locked) {
@@ -600,7 +602,7 @@ session_dispatch_repo_child(int fd, short event, void 
 			else if (do_ref_updates)
 				err = begin_ref_updates(client, &imsg);
 			else if (do_ref_update)
-				err = update_ref(client,
+				err = update_ref(&shut, client,
 				    gotd_session.repo->path, &imsg);
 			if (err)
 				log_warnx("uid %d: %s", client->euid, err->msg);
blob - 907efb48181d5468b2ba8bcf55d39ca905be2686
blob + c9dca58857cb121368aa8d3378609e72787267c1
--- regress/gotd/repo_write.sh
+++ regress/gotd/repo_write.sh
@@ -252,7 +252,48 @@ EOF
 	test_done "$testroot" "$ret"
 }
 
+test_send_new_empty_branch() {
+	local testroot=`test_init send_new_empty_branch 1`
 
+	got clone -q ${GOTD_TEST_REPO_URL} $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+	local commit_id=`git_show_head $testroot/repo-clone`
+
+	got branch -r $testroot/repo-clone -c main newbranch2 >/dev/null
+	got send -b newbranch2 -q -r $testroot/repo-clone
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got send failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	# Verify that the send operation worked fine.
+	got clone -l ${GOTD_TEST_REPO_URL} | grep newbranch2 > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		echo "got clone -l failed unexpectedly" >&2
+		test_done "$testroot" "1"
+		return 1
+	fi
+
+	echo "refs/heads/newbranch2: $commit_id" > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+
+	test_done "$testroot" "$ret"
+}
+
+
 test_parseargs "$@"
 run_test test_send_basic
 run_test test_fetch_more_history
+run_test test_send_new_empty_branch