Commit Diff

Commit:
0ff2c315fe0f2f0fe4a92cae73c8a4a9fd21a402
From:
Stefan Sperling <stsp@stsp.name>
Date:
Message:
gotd: fix "bad packfile with zero objects" error while creating branches 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. Problem found by op@. 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. ok op@
Actions:
Patch | Tree
commit - fecfd5bc4d412263e1178f9b6edf69709ea6e273
commit + 0ff2c315fe0f2f0fe4a92cae73c8a4a9fd21a402
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 @@ validate_object_type(int obj_type)
 }
 
 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 @@ done:
 }
 
 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) {
@@ -944,8 +961,19 @@ recv_packfile(struct imsg *imsg)
 		goto done;
 
 	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,23 +1313,25 @@ 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;
-			}
-			err = install_packfile(iev);
-			if (err) {
-				log_warnx("%s: install 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 = update_refs(iev);
 			if (err) {
blob - c32e0bf5066a6f1b2b11a59ed1e4200d990d37ba
blob + 0bc48fac834b679822e3314db38590b9da8f3575
--- gotd/session.c
+++ gotd/session.c
@@ -394,8 +394,8 @@ begin_ref_updates(struct gotd_session_client *client, 
 }
 
 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
@@ -251,8 +251,49 @@ 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