From: Mark Jamsek <mark@jamsek.com>
Subject: Re: fix gotd failing to protect refs
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 27 Jan 2025 17:00:37 +1100
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