| Message ID | pull.1068.git.git.1629203489546.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fixup! propagate errno from failing read | expand |
> diff --git a/refs/files-backend.c b/refs/files-backend.c > index 0d96eeba61b..f546cc3cc3d 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -454,6 +454,7 @@ stat_ref: > } > strbuf_reset(&sb_contents); > if (strbuf_read(&sb_contents, fd, 256) < 0) { > + myerr = errno; > close(fd); > goto out; > } Reviewed-by: Jonathan Tan <jonathantanmy@google.com> Thanks - a straightforward fixup. (I don't think we need the errno from close() in this case.)
"Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Han-Wen Nienhuys <hanwen@google.com> > > This fixes a crash triggered by the BUG() statement. > > This can occur with symlinked .git/refs. To check availability, > refs_verify_refname_available will run refs_read_raw_ref() on each prefix, > leading to a read() from .git/refs (which is a directory). > > When handling the symlink case, it is probably more robust to re-issue the > lstat() as a normal stat(), in which case, we would fall back to the directory > case. > > For now, propagating errno from strbuf_read() is simpler and avoids the crash. > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > --- > fixup! propagate errno from failing read Hmph, I do not see a commit with "propagate errno from failing read" in its title anywhere in 'seen'. I think the convention to assign errno to myerr in this codepath originates in a0731250 (refs: explicitly return failure_errno from parse_loose_ref_contents, 2021-07-20), and it forgot the part of the code being fixed with this patch. The commit being fixed is already is in 'next' as part of the hn/refs-errno-cleanup topic. Usually, a flaw in a topic that is already in 'next' is corrected by a follow-up patch, but then they won't say "fixup!" (none of our bugfix patches do). But a post-release is a special time, as we will soon be rewinding 'next', restarting it from the latest release and we have a choice to tentatively eject a topic, fix it up or even replace it, before merging the corrected topic to 'next'. Do you mean that you want me to squash this change into that commit before the topic graduates to 'master' during the new development cycle? If so please be a bit more explicit next time. Using the title of the commit after "fixup!" would be a good starting point. Thanks.
On Wed, Aug 18, 2021 at 12:38 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Han-Wen Nienhuys via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Han-Wen Nienhuys <hanwen@google.com> > > > > This fixes a crash triggered by the BUG() statement. > > > > This can occur with symlinked .git/refs. To check availability, > > refs_verify_refname_available will run refs_read_raw_ref() on each prefix, > > leading to a read() from .git/refs (which is a directory). > > > > When handling the symlink case, it is probably more robust to re-issue the > > lstat() as a normal stat(), in which case, we would fall back to the directory > > case. > > > > For now, propagating errno from strbuf_read() is simpler and avoids the crash. > > > > Signed-off-by: Han-Wen Nienhuys <hanwen@google.com> > > --- > > fixup! propagate errno from failing read > > Hmph, I do not see a commit with "propagate errno from failing read" > in its title anywhere in 'seen'. > I think the convention to assign errno to myerr in this codepath > originates in a0731250 (refs: explicitly return failure_errno from > parse_loose_ref_contents, 2021-07-20), and it forgot the part of the > code being fixed with this patch. The commit being fixed is already > is in 'next' as part of the hn/refs-errno-cleanup topic. > > Usually, a flaw in a topic that is already in 'next' is corrected by > a follow-up patch, but then they won't say "fixup!" (none of our > bugfix patches do). But a post-release is a special time, as we > will soon be rewinding 'next', restarting it from the latest release > and we have a choice to tentatively eject a topic, fix it up or > even replace it, before merging the corrected topic to 'next'. > > Do you mean that you want me to squash this change into that commit > before the topic graduates to 'master' during the new development > cycle? If so please be a bit more explicit next time. Using the > title of the commit after "fixup!" would be a good starting point. The problem fixed here affects anyone who uses git-repo (ie. does Android development) and runs "git-branch -m", which is a large group of people, so I think it should not be allowed to get into a release. So it could be squashed into commit a0731250, or put on top of next as a separate commit (probably with 'fixup!' removed). Note that, even though commit a0731250 originates from a branch called "hn/XXX" and has me as Author, the BUG() call causing the crash was actually introduced by AEvar when he reworked the series.
Han-Wen Nienhuys <hanwen@google.com> writes: >> I think the convention to assign errno to myerr in this codepath >> originates in a0731250 (refs: explicitly return failure_errno from >> parse_loose_ref_contents, 2021-07-20), and it forgot the part of the >> code being fixed with this patch. The commit being fixed is already >> is in 'next' as part of the hn/refs-errno-cleanup topic. >> >> Usually, a flaw in a topic that is already in 'next' is corrected by >> a follow-up patch, but then they won't say "fixup!" (none of our >> bugfix patches do). But a post-release is a special time, as we >> will soon be rewinding 'next', restarting it from the latest release >> and we have a choice to tentatively eject a topic, fix it up or >> even replace it, before merging the corrected topic to 'next'. >> >> Do you mean that you want me to squash this change into that commit >> before the topic graduates to 'master' during the new development >> cycle? If so please be a bit more explicit next time. Using the >> title of the commit after "fixup!" would be a good starting point. > > The problem fixed here affects anyone who uses git-repo (ie. does > Android development) and runs "git-branch -m", which is a large group > of people, so I think it should not be allowed to get into a release. OK. The problem already is in 'next' and we want to make sure it won't graduate to 'master' for the next release as-is. I agree with that ;-) > So it could be squashed into commit a0731250, or put on top of next as > a separate commit (probably with 'fixup!' removed). I'd try the former first and will fall back on the latter, then. > Note that, even though commit a0731250 originates from a branch called > "hn/XXX" and has me as Author, the BUG() call causing the crash was > actually introduced by AEvar when he reworked the series. Yup, I see his Sob after yours and it is quite understandable if a new bug was introduced by his changes. It also would be understandable if his change was only to add a call to BUG() in order to assert that the original patch used myerr consistently, and it uncovered a bug in the original version he took from you. I do not care too much about how exactly the bug was introduced and uncovered---it matters more that the end result has one fewer bug thanks to the team effort.
Hi, Jonathan Tan wrote: > Han-Wen Nienhuys wrote: >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 0d96eeba61b..f546cc3cc3d 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -454,6 +454,7 @@ stat_ref: >> } >> strbuf_reset(&sb_contents); >> if (strbuf_read(&sb_contents, fd, 256) < 0) { >> + myerr = errno; >> close(fd); >> goto out; >> } > > Reviewed-by: Jonathan Tan <jonathantanmy@google.com> > > Thanks - a straightforward fixup. (I don't think we need the errno from > close() in this case.) This looks good as far as it goes, but how do we know this has covered all the code paths? Let's see. The only nonobvious paths are stat_ref: /* * We might have to loop back here to avoid a race * condition: first we lstat() the file, then we try * to read it as a link or as a file. But if somebody * changes the type of the file (file <-> directory * <-> symlink) between the lstat() and reading, then * we don't want to report that as an error but rather * try again starting with the lstat(). * * We'll keep a count of the retries, though, just to avoid * any confusing situation sending us into an infinite loop. */ if (remaining_retries-- <= 0) goto out; and ret = parse_loose_ref_contents(buf, oid, referent, type, &myerr); out: if (ret && !myerr) BUG("returning non-zero %d, should have set myerr!", ret); For the 'stat_ref' case, we have to check that whenever we 'goto stat_ref', we set myerr moments before. Fortunately, that is the case. For the 'fall through into out' case, we have the check the parse_loose_ref_contents always sets *failure_errno on error. That is also the case. So this indeed covers all our cases, and the BUG now correctly reflects an invariant we can count on. Thanks for the fix, and thanks for looking it over. Sincerely, Jonathan
Junio C Hamano <gitster@pobox.com> writes: >> So it could be squashed into commit a0731250, or put on top of next as >> a separate commit (probably with 'fixup!' removed). > > I'd try the former first and will fall back on the latter, then. I've reverted a0731250 out of 'next', squashed the fix in, rebuilt the topic and merged it back to 'next'. Thanks.
On Tue, Aug 17, 2021 at 12:31:29PM +0000, Han-Wen Nienhuys via GitGitGadget wrote: > diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh > index cc4b10236e2..dd17718160a 100755 > --- a/t/t3200-branch.sh > +++ b/t/t3200-branch.sh > @@ -109,6 +109,20 @@ test_expect_success 'git branch -m n/n n should work' ' > git reflog exists refs/heads/n > ' > > +test_expect_success 'git branch -m with symlinked .git/refs' ' > + git init subdir && > + test_when_finished "rm -rf subdir" && > + (cd subdir && > + for d in refs objects packed-refs ; do > + rm -rf .git/$d && > + ln -s ../../.git/$d .git/$d ; done ) && > + git --git-dir subdir/.git/ branch rename-src && > + expect=$(git rev-parse rename-src) && > + git --git-dir subdir/.git/ branch -m rename-src rename-dest && > + test $(git rev-parse rename-dest) = "$expect" && > + git branch -D rename-dest > +' This test presumably needs the SYMLINKS prerequisite. I noticed that the Windows CI for "next" is now failing. -Peff
On Wed, Aug 18, 2021 at 8:30 PM Junio C Hamano <gitster@pobox.com> wrote: > I do not care too much about how exactly the bug was introduced and > uncovered---it matters more that the end result has one fewer bug > thanks to the team effort. agreed. Just wanted to give context on my bungled "fixup!" subject.
diff --git a/refs/files-backend.c b/refs/files-backend.c index 0d96eeba61b..f546cc3cc3d 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -454,6 +454,7 @@ stat_ref: } strbuf_reset(&sb_contents); if (strbuf_read(&sb_contents, fd, 256) < 0) { + myerr = errno; close(fd); goto out; } diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index cc4b10236e2..dd17718160a 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -109,6 +109,20 @@ test_expect_success 'git branch -m n/n n should work' ' git reflog exists refs/heads/n ' +test_expect_success 'git branch -m with symlinked .git/refs' ' + git init subdir && + test_when_finished "rm -rf subdir" && + (cd subdir && + for d in refs objects packed-refs ; do + rm -rf .git/$d && + ln -s ../../.git/$d .git/$d ; done ) && + git --git-dir subdir/.git/ branch rename-src && + expect=$(git rev-parse rename-src) && + git --git-dir subdir/.git/ branch -m rename-src rename-dest && + test $(git rev-parse rename-dest) = "$expect" && + git branch -D rename-dest +' + # The topmost entry in reflog for branch bbb is about branch creation. # Hence, we compare bbb@{1} (instead of bbb@{0}) with aaa@{0}.