# Be more careful about updating refs 23 messages from 2008-01-15 to 2008-01-23. Participants: Linus Torvalds, Junio C Hamano, Charles Bailey, Johannes Sixt, Johannes Schindelin, Shawn O. Pearce, Sam Vilain. Thread: https://gitlist.dev/t/11633 ## Linus Torvalds, 2008-01-15 23:50 Subject: Be more careful about updating refs Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801151546560.2806%40woody.linux-foundation.org ``` This makes write_ref_sha1() more careful: it actually checks the SHA1 of the ref it is updating, and refuses to update a ref with an object that it cannot find. Perhaps more importantly, it also refuses to update a branch head with a non-commit object. I don't quite know *how* the stable series maintainers were able to corrupt their repository to have a HEAD that pointed to a tag rather than a commit object, but they did. Which results in a totally broken repository that cannot be cloned or committed on. So make it harder for people to shoot themselves in the foot like that. Signed-off-by: Linus Torvalds --- I'm signing off on this, but I hope people will double-check this: I didn't actually test it very much. refs.c | 19 +++++++++++++++++++ 1 files changed, 19 insertions(+), 0 deletions(-) diff --git a/refs.c b/refs.c index 58f6d17..c3ffe03 100644 --- a/refs.c +++ b/refs.c @@ -1119,10 +1119,16 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, return 0; } +static int is_branch(const char *refname) +{ + return !strcmp(refname, "HEAD") || !prefixcmp(refname, "refs/heads/"); +} + int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg) { static char term = '\n'; + struct object *o; if (!lock) return -1; @@ -1130,6 +1136,19 @@ int write_ref_sha1(struct ref_lock *lock, unlock_ref(lock); return 0; } + o = parse_object(sha1); + if (!o) { + error("Trying to write ref %s with nonexistant object %s", + lock->ref_name, sha1_to_hex(sha1)); + unlock_ref(lock); + return -1; + } + if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) { + error("Trying to write non-commit object %s to branch %s", + sha1_to_hex(sha1), lock->ref_name); + unlock_ref(lock); + return -1; + } if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 || write_in_full(lock->lock_fd, &term, 1) != 1 || close(lock->lock_fd) < 0) { ``` ## Linus Torvalds, 2008-01-16 00:02 Subject: Re: Be more careful about updating refs Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801151600140.2806%40woody.linux-foundation.org In-Reply-To: ``` On Tue, 15 Jan 2008, Linus Torvalds wrote: > > This makes write_ref_sha1() more careful: it actually checks the SHA1 of > the ref it is updating, and refuses to update a ref with an object that it > cannot find. Side note: this breaks some tests, because those tests do things like git update-ref refs/heads/master 1111111111111111111111111111111111111111 && test 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master) which obviously won't work. I didn't update the tests, because I'm an evil person who just finds it very onerous to touch tests, and I particularly hate tests that turn out to be wrong. (Pet peeve on mine: people fixing assert()'s by changing the source-code, without ever asking themselves whether maybe the assert itself was the bug). Linus ``` ## Junio C Hamano, 2008-01-16 00:29 Subject: Re: Be more careful about updating refs Message-ID: <7vd4s2d33x.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vd4s2d33x.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > Signed-off-by: Linus Torvalds > --- > > I'm signing off on this, but I hope people will double-check this: I > didn't actually test it very much. Does "Signed-off-by:" line mean something different for you than for other people these days? I thought that the rule that applies to you (and only you) on this list was that all patches from you are DCO certified and I am free to forge your signature on them, and the other rule that applies to everybody including you is that Signed-off-by: is about DCO certification and not about anything else (e.g. author's confidence level about the patch). I am asking because you did not have S-o-b and did not say anything about the other patch about commit object creation. Although both patches looked sane to me. ``` ## Linus Torvalds, 2008-01-16 00:42 Subject: Re: Be more careful about updating refs Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.00.0801151640530.2806%40woody.linux-foundation.org In-Reply-To: <7vd4s2d33x.fsf@gitster.siamese.dyndns.org> ``` On Tue, 15 Jan 2008, Junio C Hamano wrote: > > Does "Signed-off-by:" line mean something different for you than > for other people these days? I intentionally don't tend to sign off stuff that I just send out as trial balloons, not meant to "really" be applied. Of course, then I *also* forget to sign off emails that I do intend to be applied, so that rule-of-thumb is pretty weak ;) Linus ``` ## Junio C Hamano, 2008-01-16 01:11 Subject: Re: Be more careful about updating refs Message-ID: <7vzlv6bmm3.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzlv6bmm3.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > This makes write_ref_sha1() more careful: it actually checks the SHA1 of > the ref it is updating, and refuses to update a ref with an object that it > cannot find. > > Perhaps more importantly, it also refuses to update a branch head with a > non-commit object. I don't quite know *how* the stable series maintainers > were able to corrupt their repository to have a HEAD that pointed to a tag > rather than a commit object, but they did. Which results in a totally > broken repository that cannot be cloned or committed on. Two questions and a comment: - Do we want to impose the same restriction on refs/remotes/? I think that is a logical thing to do. - What should the receive-pack and git-fetch do if they trigger the check in this codepath while updating the refs under the affected hierarchies? Fail the push and fetch? - I think !strcmp(refname, "HEAD") is not quite a right check to do here. In order to catch the detached head case, it must be checked, but at the same time if the head is not detached, it should look at where the symref points at (i.e. a symref HEAD that points outside refs/heads is an error, and we need to catch that). > +static int is_branch(const char *refname) > +{ > + return !strcmp(refname, "HEAD") || !prefixcmp(refname, "refs/heads/"); > +} ``` ## Junio C Hamano, 2008-01-16 19:52 Subject: Re: Be more careful about updating refs Message-ID: <7vodblpmyc.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vodblpmyc.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > On Tue, 15 Jan 2008, Linus Torvalds wrote: >> >> This makes write_ref_sha1() more careful: it actually checks the SHA1 of >> the ref it is updating, and refuses to update a ref with an object that it >> cannot find. > > Side note: this breaks some tests, because those tests do things like > > git update-ref refs/heads/master 1111111111111111111111111111111111111111 && > test 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master) > > ... > (Pet peeve on mine: people fixing assert()'s by changing the source-code, > without ever asking themselves whether maybe the assert itself was the > bug). The rules for the plumbing used to be that refs can point at anything that get_sha1() accepts. We did not even require it to be parse_object() happy let alone it being parse_commit() kosher. You changed the world order. I agree that the world order was changed in a good way, but saying that the original test did not check the right thing or it was a bug is not quite fair. At worst, we can say that it was very sloppily written by assuming that the commands involved in the particular test would not care about corrupted repositories whose refs point at nonexistant bogus objects. I'll squash the following to your patch. --- t/t1400-update-ref.sh | 19 +++++++++++++------ 1 files changed, 13 insertions(+), 6 deletions(-) diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh index a90824b..71ab2dd 100755 --- a/t/t1400-update-ref.sh +++ b/t/t1400-update-ref.sh @@ -7,12 +7,19 @@ test_description='Test git update-ref and basic ref logging' . ./test-lib.sh Z=0000000000000000000000000000000000000000 -A=1111111111111111111111111111111111111111 -B=2222222222222222222222222222222222222222 -C=3333333333333333333333333333333333333333 -D=4444444444444444444444444444444444444444 -E=5555555555555555555555555555555555555555 -F=6666666666666666666666666666666666666666 + +test_expect_success setup ' + + for name in A B C D E F + do + test_tick && + T=$(git write-tree) && + sha1=$(echo $name | git commit-tree $T) && + eval $name=$sha1 + done + +' + m=refs/heads/master n_dir=refs/heads/gu n=$n_dir/fixes ``` ## Charles Bailey, 2008-01-17 09:15 Subject: Re: Be more careful about updating refs Message-ID: <20080117091558.GA8341@hashpling.org> URL: https://gitlist.dev/e/20080117091558.GA8341%40hashpling.org In-Reply-To: <7vodblpmyc.fsf@gitster.siamese.dyndns.org> ``` On Wed, Jan 16, 2008 at 11:52:43AM -0800, Junio C Hamano wrote: > Linus Torvalds writes: > > > On Tue, 15 Jan 2008, Linus Torvalds wrote: > >> > >> This makes write_ref_sha1() more careful: it actually checks the SHA1 of > >> the ref it is updating, and refuses to update a ref with an object that it > >> cannot find. > > > > Side note: this breaks some tests, because those tests do things like > > > > git update-ref refs/heads/master 1111111111111111111111111111111111111111 && > > test 1111111111111111111111111111111111111111 = $(cat .git/refs/heads/master) > > > > ... > > (Pet peeve on mine: people fixing assert()'s by changing the source-code, > > without ever asking themselves whether maybe the assert itself was the > > bug). > > The rules for the plumbing used to be that refs can point at > anything that get_sha1() accepts. We did not even require it to > be parse_object() happy let alone it being parse_commit() kosher. > > You changed the world order. I agree that the world order was > changed in a good way, but saying that the original test did not > check the right thing or it was a bug is not quite fair. At > worst, we can say that it was very sloppily written by assuming > that the commands involved in the particular test would not care > about corrupted repositories whose refs point at nonexistant > bogus objects. > > I'll squash the following to your patch. I'm assuming that this original patch and the test update turned into the following commit in master: c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit I just thought I should warn you that this seems (git bisect tells me so) to have caused a failure in t9301-fast-export.sh on my Mac OS X 10.4.11 machine although I haven't yet had the time to investigate why. Charles. ``` ## Johannes Sixt, 2008-01-17 10:52 Subject: Re: Be more careful about updating refs Message-ID: <478F3367.8050307@viscovery.net> URL: https://gitlist.dev/e/478F3367.8050307%40viscovery.net In-Reply-To: <20080117091558.GA8341@hashpling.org> ``` Charles Bailey schrieb: > I'm assuming that this original patch and the test update turned into > the following commit in master: > > c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit > > I just thought I should warn you that this seems (git bisect tells me > so) to have caused a failure in t9301-fast-export.sh on my Mac OS X > 10.4.11 machine although I haven't yet had the time to investigate > why. I observed the same (on Windows). The reason is that above-mentioned commit introduces a call to parse_objects(). But by the time that fast-import calls write_ref_sha1() (and, hence, this new parse_objects()) it has not yet written a pack file, and parse_objects() fails. I don't have a clue how to fix this short of reverting the commit. -- Hannes ``` ## Charles Bailey, 2008-01-17 10:56 Subject: Re: Be more careful about updating refs Message-ID: <20080117105635.GA12285@hashpling.org> URL: https://gitlist.dev/e/20080117105635.GA12285%40hashpling.org In-Reply-To: <20080117091558.GA8341@hashpling.org> ``` On Thu, Jan 17, 2008 at 09:15:58AM +0000, Charles Bailey wrote: > > c3b0dec509fe136c5417422f31898b5a4e2d5e02 is first bad commit > > I just thought I should warn you that this seems (git bisect tells me > so) to have caused a failure in t9301-fast-export.sh on my Mac OS X > 10.4.11 machine although I haven't yet had the time to investigate > why. > Further to this, the first problem is that the 'fast-export | fast-import' test is failing. As far as I can tell, fast-export is behaving no differently, the generated export file looks identical in shape. It seems that fast-import is creating a bad temporary packfile for itself. The error that I'm getting is: fatal: unknown object type 0 in .git/objects/pack/pack-6be2b92c2d7485fd5cefbb0d9a68827e4c23d548.pack fast-import: dumping crash report to .git/fast_import_crash_12317 which seems to come from the unpack_entry function in sha1_file.c. I haven't got much deeper into how the refs.c change has caused the issue, or why on earth it seems to be a Mac only issue. ``` ## Charles Bailey, 2008-01-17 11:01 Subject: Re: Be more careful about updating refs Message-ID: <20080117110132.GB12285@hashpling.org> URL: https://gitlist.dev/e/20080117110132.GB12285%40hashpling.org In-Reply-To: <478F3367.8050307@viscovery.net> ``` On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote: > > I observed the same (on Windows). The reason is that above-mentioned > commit introduces a call to parse_objects(). But by the time that > fast-import calls write_ref_sha1() (and, hence, this new parse_objects()) > it has not yet written a pack file, and parse_objects() fails. I don't > have a clue how to fix this short of reverting the commit. > OK, so it's not just Mac OS X, then. From your description and my initial poke at the code, I can't immediately see a good reason why the test should succeed on Linux, though. ``` ## Johannes Sixt, 2008-01-17 12:41 Subject: Re: Be more careful about updating refs Message-ID: <478F4CDC.2090703@viscovery.net> URL: https://gitlist.dev/e/478F4CDC.2090703%40viscovery.net In-Reply-To: <20080117110132.GB12285@hashpling.org> ``` Charles Bailey schrieb: > On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote: >> I observed the same (on Windows). The reason is that above-mentioned >> commit introduces a call to parse_objects(). But by the time that >> fast-import calls write_ref_sha1() (and, hence, this new parse_objects()) >> it has not yet written a pack file, and parse_objects() fails. I don't >> have a clue how to fix this short of reverting the commit. >> > > OK, so it's not just Mac OS X, then. From your description and my > initial poke at the code, I can't immediately see a good reason why > the test should succeed on Linux, though. My analysis is not correct. The pack file is present, but it seems to be incomplete. The reason is the NO_MMAP build flag. If you compile with NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell? -- Hannes ``` ## Johannes Schindelin, 2008-01-17 12:58 Subject: Re: Be more careful about updating refs Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0801171256280.17650%40racer.site In-Reply-To: <478F4CDC.2090703@viscovery.net> ``` Hi, On Thu, 17 Jan 2008, Johannes Sixt wrote: > Charles Bailey schrieb: > > On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote: > >> I observed the same (on Windows). The reason is that above-mentioned > >> commit introduces a call to parse_objects(). But by the time that > >> fast-import calls write_ref_sha1() (and, hence, this new parse_objects()) > >> it has not yet written a pack file, and parse_objects() fails. I don't > >> have a clue how to fix this short of reverting the commit. > >> > > > > OK, so it's not just Mac OS X, then. From your description and my > > initial poke at the code, I can't immediately see a good reason why > > the test should succeed on Linux, though. > > My analysis is not correct. The pack file is present, but it seems to be > incomplete. > > The reason is the NO_MMAP build flag. If you compile with > NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell? Just a wild guess... Could it be that the mmap() happened before contents were written to that fd? The faked mmap() is just a malloc() && pread(), and would thus miss out on changes written after the mmap() call. Ciao, Dscho ``` ## Charles Bailey, 2008-01-17 13:07 Subject: Re: Be more careful about updating refs Message-ID: <20080117130740.GA30090@hashpling.org> URL: https://gitlist.dev/e/20080117130740.GA30090%40hashpling.org In-Reply-To: ``` On Thu, Jan 17, 2008 at 12:58:07PM +0000, Johannes Schindelin wrote: > > Just a wild guess... Could it be that the mmap() happened before contents > were written to that fd? The faked mmap() is just a malloc() && pread(), > and would thus miss out on changes written after the mmap() call. This would be my guess too. As far as I can tell fast-import is creating objects directly in a packfile. I'm guessing here, but the new checking code is probably being triggered to check (for example) the parent ref for commits that fast-import is creating and the check only works if mmap can successfully examine objects in the still-being-written packfile. ``` ## Junio C Hamano, 2008-01-18 01:43 Subject: Re: Be more careful about updating refs Message-ID: <7vbq7jkixg.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbq7jkixg.fsf%40gitster.siamese.dyndns.org In-Reply-To: <478F4CDC.2090703@viscovery.net> ``` Johannes Sixt writes: > Charles Bailey schrieb: >> On Thu, Jan 17, 2008 at 11:52:23AM +0100, Johannes Sixt wrote: >>> I observed the same (on Windows). The reason is that above-mentioned >>> commit introduces a call to parse_objects(). But by the time that >>> fast-import calls write_ref_sha1() (and, hence, this new parse_objects()) >>> it has not yet written a pack file, and parse_objects() fails. I don't >>> have a clue how to fix this short of reverting the commit. >>> >> >> OK, so it's not just Mac OS X, then. From your description and my >> initial poke at the code, I can't immediately see a good reason why >> the test should succeed on Linux, though. > > My analysis is not correct. The pack file is present, but it seems to be > incomplete. > > The reason is the NO_MMAP build flag. If you compile with > NO_MMAP=YesPlease on Linux, t9301 fails as well. Does this ring a bell? I ran strace and found that fast-import retains three windows to the same data that was opened while the pack was still being built (i.e. the filename is still tmp_pack_XXXXXX) when it dies: {next = 0x6f6d20, base = 0x6f77a0 "PACK", offset = 0, len = 907, last_used = 9, inuse_cnt = 0} {next = 0x728630, base = 0x6f7160 "PACK", offset = 0, len = 500, last_used = 5, inuse_cnt = 0} {next = 0x0, base = 0x6f6d50 "PACK", offset = 0, len = 261, last_used = 1, inuse_cnt = 0} When it hits end_packfile() to install the built-pack to its final destination and makes it available to the rest of git, however, it has this clever code to reuse the still open window. The object offset asked when it dies is at offset 887. However, tmp_pack was written after that last window with length 907 has been (re)opened, and reusing the window results in reading a stale data. The following patch seems to fix the issue for me, but this is primarily meant for discussion, as I do not quite understand why the same issue does not manifest itself when NO_MMAP is not used. diff --git a/fast-import.c b/fast-import.c index 3609c24..bd0ddb1 100644 --- a/fast-import.c +++ b/fast-import.c @@ -926,7 +926,13 @@ static void end_packfile(void) new_p = add_packed_git(idx_name, strlen(idx_name), 1); if (!new_p) die("core git rejected index %s", idx_name); - new_p->windows = old_p->windows; + while (old_p->windows) { + struct pack_window *w = old_p->windows; + munmap(w->base, w->len); + old_p->windows = w->next; + free(w); + } + new_p->windows = NULL; all_packs[pack_id] = new_p; install_packed_git(new_p); ``` ## Junio C Hamano, 2008-01-18 02:01 Subject: Re: Be more careful about updating refs Message-ID: <7v63xrki29.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v63xrki29.fsf%40gitster.siamese.dyndns.org In-Reply-To: <7vbq7jkixg.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano writes: > The following patch seems to fix the issue for me, but this is > primarily meant for discussion, as I do not quite understand why > the same issue does not manifest itself when NO_MMAP is not > used. > ... BTW, the lookup of the object that dies is in update_branch(). The call to write_ref_sha1() at the last tries to verify b->sha1 is available and is a commit. static int update_branch(struct branch *b) { static const char *msg = "fast-import"; struct ref_lock *lock; unsigned char old_sha1[20]; if (read_ref(b->name, old_sha1)) hashclr(old_sha1); lock = lock_any_ref_for_update(b->name, old_sha1, 0); if (!lock) return error("Unable to lock %s", b->name); if (!force_update && !is_null_sha1(old_sha1)) { struct commit *old_cmit, *new_cmit; old_cmit = lookup_commit_reference_gently(old_sha1, 0); new_cmit = lookup_commit_reference_gently(b->sha1, 0); if (!old_cmit || !new_cmit) { unlock_ref(lock); return error("Branch %s is missing commits.", b->name); } if (!in_merge_bases(old_cmit, &new_cmit, 1)) { unlock_ref(lock); warning("Not updating %s" " (new tip %s does not contain %s)", b->name, sha1_to_hex(b->sha1), sha1_to_hex(old_sha1)); return -1; } } if (write_ref_sha1(lock, b->sha1, msg) < 0) return error("Unable to update %s", b->name); return 0; } The write_ref_sha1() function previously did not even check if the object pointed at by b->sha1 actually existed, but now it parses the object and makes sure it is a commit, if the updated ref is under ref/heads/ hierarchy. What I do not quite understand is how this can be a new issue. The codepath to allow updating an existing branch shown above (i.e. "if it is not force and old is not NULL") uses the usual lookup_commit_reference_gently() interface to access b->sha1, and does not use gfi-aware gfi_unpack_entry() or anything magical, which means it would have passed the same codepath down to trigger the same issue. IOW, even before this tightening of write_ref_sha1(), we already should have had the issue of not being to able to grab the object b->sha1 refers to out of the newly built packfile. Is it just nobody seriously exercised the codepath yet, or is there a difference between these two calls that is more subtle than that? Quite confused... ``` ## Shawn O. Pearce, 2008-01-18 02:13 Subject: Re: Be more careful about updating refs Message-ID: <20080118021300.GR24004@spearce.org> URL: https://gitlist.dev/e/20080118021300.GR24004%40spearce.org In-Reply-To: <7v63xrki29.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > Junio C Hamano writes: > > > The following patch seems to fix the issue for me, but this is > > primarily meant for discussion, as I do not quite understand why > > the same issue does not manifest itself when NO_MMAP is not > > used. > > ... ... > What I do not quite understand is how this can be a new issue. It isn't. It is indeed a pretty old issue, as far as git issues go. Its probably about as old as fast-import being accepted into your tree is. > The codepath to allow updating an existing branch shown above > (i.e. "if it is not force and old is not NULL") uses the usual > lookup_commit_reference_gently() interface to access b->sha1, > and does not use gfi-aware gfi_unpack_entry() or anything > magical, which means it would have passed the same codepath down > to trigger the same issue. IOW, even before this tightening of > write_ref_sha1(), we already should have had the issue of not > being to able to grab the object b->sha1 refers to out of the > newly built packfile. > > Is it just nobody seriously exercised the codepath yet, or is > there a difference between these two calls that is more subtle > than that? I think the problem is nobody has tested fast-import updating an existing ref while using NO_MMAP. Or if they did, they didn't report the problem as they didn't figure they needed fast-import that badly. Updating an existing ref is not a common operation, but the test suite does test for it. So it must be the NO_MMAP configuration is simply not being tested well enough. -- Shawn. ``` ## Junio C Hamano, 2008-01-18 02:25 Subject: Re: Be more careful about updating refs Message-ID: <7v1w8fkgy9.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v1w8fkgy9.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080118021300.GR24004@spearce.org> ``` "Shawn O. Pearce" writes: > I think the problem is nobody has tested fast-import updating an > existing ref while using NO_MMAP. Or if they did, they didn't report > the problem as they didn't figure they needed fast-import that badly. > > Updating an existing ref is not a common operation, but the test > suite does test for it. So it must be the NO_MMAP configuration > is simply not being tested well enough. Ok, thanks. Now a more important question is how we would properly fix this issue? I suspect that fast-import is the only one that opens windows into an unfinalized pack, and if that is the case, it would be the only program that may be hit by the issue of mmap emulation getting stale data. I do not think the patch I posted was correct at all. Especially, I am not sure if the issue only exists at the end_packfile() boundary. Don't we have the same issue reading from the packfile being built, and isn't the only reason my hack works it around is because access patterns of the testsuite happens to not trigger it? ``` ## Shawn O. Pearce, 2008-01-18 02:30 Subject: Re: Be more careful about updating refs Message-ID: <20080118023005.GS24004@spearce.org> URL: https://gitlist.dev/e/20080118023005.GS24004%40spearce.org In-Reply-To: <7vbq7jkixg.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > I ran strace and found that fast-import retains three windows to > the same data that was opened while the pack was still being > built (i.e. the filename is still tmp_pack_XXXXXX) when it dies: > > {next = 0x6f6d20, base = 0x6f77a0 "PACK", offset = 0, len = 907, > last_used = 9, inuse_cnt = 0} > {next = 0x728630, base = 0x6f7160 "PACK", offset = 0, len = 500, > last_used = 5, inuse_cnt = 0} > {next = 0x0, base = 0x6f6d50 "PACK", offset = 0, len = 261, > last_used = 1, inuse_cnt = 0} Ouch. Hmm, well, maybe it shouldn't keep three windows open to the same part of the file, but with different lengths. :) But that's another issue. > The following patch seems to fix the issue for me, but this is > primarily meant for discussion, I agree the patch should fix this particular issue and the performance difference it will cause is minor enough to not be worth discussing further. But I have one minor comment (please see it below). > as I do not quite understand why > the same issue does not manifest itself when NO_MMAP is not > used. When NO_MMAP is off (we are actually using the OS's true mmap) the data is updated into the window when the file is written, as the window is backed by the OS's filesystem cache. If the data access is outside of the window (past the offset of the longest window, here 907) we would just open a new window to the relevant region of the file. But when it is inside the window, the window being backed by the filesystem cache saved us. In the case of NO_MMAP this doesn't work, so we get a failure later during object access. In particular if you look at gfi_unpack_entry() (which is where we cause windows to be opened on the file being created) we tell sha1_file.c it has 20 bytes available in the window *beyond* the actual end of the file. This is due to an assumption within the windowing code of sha1_file.c that we always have the packfile footer at the end of the file, so all windowing code assumes it has at least 20 bytes from the start of a window that it can access without needing to perform additional bounds checking. So what's happening here is we are adding objects into the file, some of which may have their data appearing in the trailing 20 bytes of some prior window. If that happens the cached window looks like it can be used to access the data, but it really cannot be in the NO_MMAP case as those trailing 20 bytes of the window are all \0's. The astute reader may wonder how gfi_unpack_entry() works when NO_MMAP is being used. It doesn't for that last 20 bytes of any window. Which has me thinking that Junio's patch alone isn't enough to make fast-import work correctly under NO_MMAP. > diff --git a/fast-import.c b/fast-import.c > index 3609c24..bd0ddb1 100644 > --- a/fast-import.c > +++ b/fast-import.c > @@ -926,7 +926,13 @@ static void end_packfile(void) > new_p = add_packed_git(idx_name, strlen(idx_name), 1); > if (!new_p) > die("core git rejected index %s", idx_name); > - new_p->windows = old_p->windows; > + while (old_p->windows) { > + struct pack_window *w = old_p->windows; > + munmap(w->base, w->len); > + old_p->windows = w->next; > + free(w); > + } > + new_p->windows = NULL; This assignment of NULL should not be necessary. add_packed_git() already does this for us on new_p so we do not need to repeat it here in fast-import. > all_packs[pack_id] = new_p; > install_packed_git(new_p); > -- Shawn. ``` ## Shawn O. Pearce, 2008-01-18 02:33 Subject: Re: Be more careful about updating refs Message-ID: <20080118023327.GT24004@spearce.org> URL: https://gitlist.dev/e/20080118023327.GT24004%40spearce.org In-Reply-To: <7v1w8fkgy9.fsf@gitster.siamese.dyndns.org> ``` Junio C Hamano wrote: > "Shawn O. Pearce" writes: > > > I think the problem is nobody has tested fast-import updating an > > existing ref while using NO_MMAP. Or if they did, they didn't report > > the problem as they didn't figure they needed fast-import that badly. > > > > Updating an existing ref is not a common operation, but the test > > suite does test for it. So it must be the NO_MMAP configuration > > is simply not being tested well enough. > > Now a more important question is how we would properly fix this > issue? > > I suspect that fast-import is the only one that opens windows > into an unfinalized pack, and if that is the case, it would be > the only program that may be hit by the issue of mmap emulation > getting stale data. Yes, it is the only program that is foolish enough to access a partial packfile. index-pack uses pread(), and only after it has the entire packfile downloaded to the local system. The only other reader of a partial packfile is unpack-objects, and obviously that doesn't care about seeking backwards within the packfile. So its probably the only user who suffers from the mmap emulation. > I do not think the patch I posted was correct at all. > > Especially, I am not sure if the issue only exists at the > end_packfile() boundary. Don't we have the same issue reading > from the packfile being built, and isn't the only reason my hack > works it around is because access patterns of the testsuite > happens to not trigger it? Yes, that's my take on it as well (see my other email). The testsuite must just be really lucky that its not hitting the boundary condition. I almost said gfi_unpack_entry() was immune from this bug, but I went back and read the code again and determined that it does in fact suffer from this under NO_MMAP, and we're just really damn lucky nobody has caused it. I'll try to work up a patch this evening. -- Shawn. ``` ## Shawn O. Pearce, 2008-01-18 02:58 Subject: Re: Be more careful about updating refs Message-ID: <20080118025837.GU24004@spearce.org> URL: https://gitlist.dev/e/20080118025837.GU24004%40spearce.org In-Reply-To: <20080118023327.GT24004@spearce.org> ``` "Shawn O. Pearce" wrote: > Junio C Hamano wrote: > > > > Especially, I am not sure if the issue only exists at the > > end_packfile() boundary. Don't we have the same issue reading > > from the packfile being built, and isn't the only reason my hack > > works it around is because access patterns of the testsuite > > happens to not trigger it? > > Yes, that's my take on it as well (see my other email). The > testsuite must just be really lucky that its not hitting the > boundary condition. > > I almost said gfi_unpack_entry() was immune from this bug, but > I went back and read the code again and determined that it does > in fact suffer from this under NO_MMAP, and we're just really > damn lucky nobody has caused it. I think this solves the problem. Its based on your first patch, but would replace it. The trick here is we close the cached windows if we are accessing data from the packfile we are appending into and we have increased the file length. This way we don't blow away windows during high read/low write periods, like during branch cache reloads. --8>-- diff --git a/fast-import.c b/fast-import.c index 3609c24..8e7747c 100644 --- a/fast-import.c +++ b/fast-import.c @@ -907,6 +907,16 @@ static void unkeep_all_packs(void) } } +static void close_all_windows(struct packed_git *p) +{ + while (p->windows) { + struct pack_window *w = p->windows; + munmap(w->base, w->len); + p->windows = w->next; + free(w); + } +} + static void end_packfile(void) { struct packed_git *old_p = pack_data, *new_p; @@ -917,6 +927,7 @@ static void end_packfile(void) struct branch *b; struct tag *t; + close_all_windows(pack_data); fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1, pack_data->pack_name, object_count); close(pack_data->pack_fd); @@ -926,7 +937,6 @@ static void end_packfile(void) new_p = add_packed_git(idx_name, strlen(idx_name), 1); if (!new_p) die("core git rejected index %s", idx_name); - new_p->windows = old_p->windows; all_packs[pack_id] = new_p; install_packed_git(new_p); @@ -1129,8 +1139,10 @@ static void *gfi_unpack_entry( { enum object_type type; struct packed_git *p = all_packs[oe->pack_id]; - if (p == pack_data) + if (p == pack_data && p->pack_size < (pack_size + 20)) { + close_all_windows(p); p->pack_size = pack_size + 20; + } return unpack_entry(p, oe->offset, &type, sizep); } -- Shawn. ``` ## Shawn O. Pearce, 2008-01-18 03:18 Subject: Re: Be more careful about updating refs Message-ID: <20080118031831.GW24004@spearce.org> URL: https://gitlist.dev/e/20080118031831.GW24004%40spearce.org In-Reply-To: <20080118025837.GU24004@spearce.org> ``` "Shawn O. Pearce" wrote: > "Shawn O. Pearce" wrote: > > Junio C Hamano wrote: > > > > > > Especially, I am not sure if the issue only exists at the > > > end_packfile() boundary. Don't we have the same issue reading > > > from the packfile being built, and isn't the only reason my hack > > > works it around is because access patterns of the testsuite > > > happens to not trigger it? > > > > Yes, that's my take on it as well (see my other email). The > > testsuite must just be really lucky that its not hitting the > > boundary condition. > > > > I almost said gfi_unpack_entry() was immune from this bug, but > > I went back and read the code again and determined that it does > > in fact suffer from this under NO_MMAP, and we're just really > > damn lucky nobody has caused it. > > I think this solves the problem. Its based on your first patch, but > would replace it. The trick here is we close the cached windows if > we are accessing data from the packfile we are appending into and we > have increased the file length. This way we don't blow away windows > during high read/low write periods, like during branch cache reloads. Junio pointed out my first attempt at this didn't update the memory pressure values, so we could "run out of memory" even if we had plenty free. Try #2... --8>-- diff --git a/cache.h b/cache.h index 24735bd..15388aa 100644 --- a/cache.h +++ b/cache.h @@ -561,6 +561,7 @@ extern struct packed_git *find_sha1_pack(const unsigned char *sha1, extern void pack_report(void); extern int open_pack_index(struct packed_git *); extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *); +extern void close_pack_windows(struct packed_git *, int); extern void unuse_pack(struct pack_window **); extern struct packed_git *add_packed_git(const char *, int, int); extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t); diff --git a/fast-import.c b/fast-import.c index 3609c24..82c82ce 100644 --- a/fast-import.c +++ b/fast-import.c @@ -917,6 +917,7 @@ static void end_packfile(void) struct branch *b; struct tag *t; + close_pack_windows(pack_data, 0); fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1, pack_data->pack_name, object_count); close(pack_data->pack_fd); @@ -926,7 +927,6 @@ static void end_packfile(void) new_p = add_packed_git(idx_name, strlen(idx_name), 1); if (!new_p) die("core git rejected index %s", idx_name); - new_p->windows = old_p->windows; all_packs[pack_id] = new_p; install_packed_git(new_p); @@ -1129,8 +1129,10 @@ static void *gfi_unpack_entry( { enum object_type type; struct packed_git *p = all_packs[oe->pack_id]; - if (p == pack_data) + if (p == pack_data && p->pack_size < (pack_size + 20)) { + close_pack_windows(p, 0); p->pack_size = pack_size + 20; + } return unpack_entry(p, oe->offset, &type, sizep); } diff --git a/sha1_file.c b/sha1_file.c index 6583797..50d1dea 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -611,6 +611,34 @@ void release_pack_memory(size_t need, int fd) ; /* nothing */ } +void close_pack_windows(struct packed_git *p, int retain_fd) +{ + struct pack_window **tail = NULL, *n = p->windows; + while (n) { + struct pack_window *w = p->windows; + + if (w->inuse_cnt) { + *tail = w; + tail = &w->next; + continue; + } + + munmap(w->base, w->len); + pack_mapped -= w->len; + pack_open_windows--; + n = w->next; + free(w); + } + + p->windows = *tail; + if (p->windows) + warning("pack windows still in-use during close attempt"); + else if (!retain_fd && p->pack_fd != -1) { + close(p->pack_fd); + p->pack_fd = -1; + } +} + void unuse_pack(struct pack_window **w_cursor) { struct pack_window *w = *w_cursor; -- Shawn. ``` ## Shawn O. Pearce, 2008-01-18 03:22 Subject: Re: Be more careful about updating refs Message-ID: <20080118032222.GX24004@spearce.org> URL: https://gitlist.dev/e/20080118032222.GX24004%40spearce.org In-Reply-To: <20080118031831.GW24004@spearce.org> ``` "Shawn O. Pearce" wrote: > "Shawn O. Pearce" wrote: > > "Shawn O. Pearce" wrote: > > > Junio C Hamano wrote: > > > > > > > > Especially, I am not sure if the issue only exists at the > > > > end_packfile() boundary. Don't we have the same issue reading > > > > from the packfile being built, and isn't the only reason my hack > > > > works it around is because access patterns of the testsuite > > > > happens to not trigger it? > > > > > > Yes, that's my take on it as well (see my other email). The > > > testsuite must just be really lucky that its not hitting the > > > boundary condition. > > > > > > I almost said gfi_unpack_entry() was immune from this bug, but > > > I went back and read the code again and determined that it does > > > in fact suffer from this under NO_MMAP, and we're just really > > > damn lucky nobody has caused it. > > > > I think this solves the problem. Its based on your first patch, but > > would replace it. The trick here is we close the cached windows if > > we are accessing data from the packfile we are appending into and we > > have increased the file length. This way we don't blow away windows > > during high read/low write periods, like during branch cache reloads. > > Junio pointed out my first attempt at this didn't update the > memory pressure values, so we could "run out of memory" even > if we had plenty free. > > Try #2... OK, that was crap. Don't even try it. I'm holding off sending anything more until I get the test suite to actually run without telling me the bus crashed into the wall. -- Shawn. ``` ## Sam Vilain, 2008-01-23 22:53 Subject: Re: Be more careful about updating refs Message-ID: <4797C551.8020909@vilain.net> URL: https://gitlist.dev/e/4797C551.8020909%40vilain.net In-Reply-To: ``` Linus Torvalds wrote: > Perhaps more importantly, it also refuses to update a branch head with a > non-commit object. I don't quite know *how* the stable series maintainers > were able to corrupt their repository to have a HEAD that pointed to a tag > rather than a commit object, but they did. Which results in a totally > broken repository that cannot be cloned or committed on. I actually used this for a prototype system to allow push over git:// with secure authentication via PGP; basically it used an update hook that disallowed any ref change that wasn't a signed tag, and then in the post-update hook, moved the tag to an audit log refspace and put the referant commit in the refs/heads/* location. The hooks (probably racy etc) can be seen under http://git.utsl.gen.nz/Tangram/hooks Sam. ```