Stefan Sperling <stsp@stsp.name> wrote:
> martijn@ pointed out on IRC that it is possible to modify a
> protected branch via send -f, which is unexpected behaviour.
> 
> The reason for this is that gotd's reference protection checks only
> happen if the client sends a pack file. In martijn's simple test case
> the client doesn't send a pack file, and protection checks are skipped.

Nice work on the quick solve :)

And thanks martijn for finding and reporting this bug!

> Our test suite didn't catch this because in our test the client has
> always sent a pack file. This is due to an unrelated bug in the pack
> creation code, where a parent commit far down the line is added to
> the pack file (as the only commit) while it should have been dropped.
> This looks like an error in got_pack_paint_commits().
> I will investigate this second issue later.
> For now, shortening the commit history used by our ref protection test
> works around the pack creation bug and allows our test to cover the
> problem martijn@ found.
> 
> ok?

ok

I see the failure with the regress/gotd change applied and a gotd
binary from HEAD, and your change does indeed fix it.

There's a tiny style nit in protect_refs_from_moving::repo_write.c:1540:

+		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branches)
+		{

but there are other instances in repo_write.c of lines exceeding 80 cols
and using Allman instead of K&R style braces, so below is a diff that is
comprised entirely of style(9) changes; if ok, I'll commit after you
send up your fix?


M  gotd/repo_write.c  |  16+  16-

1 file changed, 16 insertions(+), 16 deletions(-)

path + /home/mark/src/got
commit - ef20d7bf45a8d6e280ec3583ef729a4102e5b35a
blob - 1a330d17a4953ab17612922f651ca2ebfae95496
file + gotd/repo_write.c
--- gotd/repo_write.c
+++ gotd/repo_write.c
@@ -840,8 +840,8 @@ copy_ref_delta(int infd, int outfd, off_t *outsize, BU
 }
 
 static const struct got_error *
-copy_offset_delta(int infd, int outfd, off_t *outsize, BUF *buf, size_t *buf_pos,
-    struct got_hash *ctx)
+copy_offset_delta(int infd, int outfd, off_t *outsize, BUF *buf,
+    size_t *buf_pos, struct got_hash *ctx)
 {
 	const struct got_error *err = NULL;
 	uint64_t o = 0;
@@ -1405,7 +1405,8 @@ verify_packfile(void)
 		if (ref_update->delete_ref)
 			continue;
 
-		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_tag_namespaces) {
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_tag_namespaces) {
 			err = protect_tag_namespace(pe->path, &client->pack,
 			    packidx, ref_update);
 			if (err)
@@ -1437,15 +1438,15 @@ verify_packfile(void)
 			}
 		}
 
-		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branch_namespaces)
-		{
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_branch_namespaces) {
 			err = protect_branch_namespace(pe->path,
 			    &client->pack, packidx, ref_update);
 			if (err)
 				goto done;
 		}
-		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branches)
-		{
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_branches) {
 			err = protect_branch(pe->path, &client->pack,
 			    packidx, ref_update);
 			if (err)
@@ -1492,8 +1493,8 @@ protect_refs_from_deletion(void)
 				return err;
 		}
 
-		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branches)
-		{
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_branches) {
 			if (strcmp(refname, pe->path) == 0) {
 				return got_error_fmt(GOT_ERR_REF_PROTECTED,
 				    "%s", refname);
@@ -1537,8 +1538,8 @@ protect_refs_from_moving(void)
 				return err;
 		}
 
-		RB_FOREACH(pe, got_pathlist_head, repo_write.protected_branches)
-		{
+		RB_FOREACH(pe, got_pathlist_head,
+		    repo_write.protected_branches) {
 			if (strcmp(refname, pe->path) == 0) {
 				return got_error_fmt(GOT_ERR_REF_PROTECTED,
 				    "%s", refname);
@@ -1774,8 +1775,8 @@ print_diffstat(struct got_diffstat_cb_arg *dsa, int fd
 		int pad = dsa->max_path_len - pe->path_len + 1;
 
 		dprintf(fd, " %c  %s%*c | %*d+ %*d-\n", cp->status,
-		     pe->path, pad, ' ', dsa->add_cols + 1, cp->add,
-		     dsa->rm_cols + 1, cp->rm);
+		    pe->path, pad, ' ', dsa->add_cols + 1, cp->add,
+		    dsa->rm_cols + 1, cp->rm);
 	}
 	dprintf(fd,
 	    "\n%d file%s changed, %d insertion%s(+), %d deletion%s(-)\n\n",
@@ -2326,9 +2327,8 @@ repo_write_dispatch_session(int fd, short event, void 
 	}
 
 	if (!shut && check_cancelled(NULL) == NULL) {
-		if (err &&
-		    gotd_imsg_send_error_event(iev, PROC_REPO_WRITE,
-		        client->id, err) == -1) {
+		if (err && gotd_imsg_send_error_event(iev, PROC_REPO_WRITE,
+		    client->id, err) == -1) {
 			log_warnx("could not send error to parent: %s",
 			    err->msg);
 		}


-- 
Mark Jamsek <https://bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68