git-send-pack segfaulting on DebianPPC (was: Re: cg-clone, tag objects and cg-push/git-push don't play nice)

On 10/19/05, Junio C Hamano <junkio@cox.net> wrote:

Ok -- I can now shed some light on this. This was happening on a DebianPPC machine, but I didn't have clear evidence of that being a factor (specially when Linus is widely known to use a PPC, and I hadn't seen problems in other ppc boxen around). Now I do.

For all my finger-pointing, cg-fetch was trying to fetch the refernces properly. However, git-fetch-pack is segfaulting on this Debian PPC (etch). This is true of the current git "master" and 0.99.8.c. I don'thave strace on the box until tomorrow, so I can't tell you more about it.

On Debian i386 and Ubuntu i386, the exact same versions work correctly.

I'll post more info as soon as I can get my hands on strace,

cheers,

martin

Martin Langhoff <martin.langhoff@gmail.com> writes:

Thanks, that is good to know. As I said in another message, I looked at cg-fetch's current tag completion code while working on the ref^{tree} thing, and saw it used commit walker with '-a' option (IOW, not producing an incomplete tag) to download everything.

Even after fetch-pack segfaulted, since it does not update any refs itself (it only writes them to stdout for the wrapper to process), it should not have resulted in the repository that has refs pointing at objects it does not have. In fact, it writes its "SHA1 - name" list only after seeing unpack-objects exit successfully, that should not have resulted in a corrupt repository that records incomplete refs. I suspect there is something else going on here.

In cg-fetch, fetch-pack is used only in one location, and it does not have anything to do with the tag completion code. Presumably that explains why removing tags did not have any effect, although it does not explain why removing some objects did -- maybe those object files you needed to remove were corrupt?

On 10/19/05, Junio C Hamano <junkio@cox.net> wrote:

Ok -- I'm at home now so I can't test it any further, but reading cg-fetch, it would actually be running git-ssh-fetch which was failing (but cogito hides STDERR). When I tried it by hand, I used git-fetch-pack, which segfaulted on Debian PPC and worked on i386.

Hmmm. You're right, it only spits out a list of things to retrieve...

Well, I'll strace the execution tomorrow and let you know. The tag objects, however, are there. I can reliably git-cat-file tag <sha1> them and they look pretty normal. The commit objects they refer to are missing, though.

cheers,

martin

On Wed, 19 Oct 2005, Martin Langhoff wrote:

Interesting.

As you say, I'm obviously testing on ppc all the time, and if git-fetch-pack has problems on ppc, I haven't seen them. And I fetch a lot.

It might be just the repo you're using. I can't test, since I obviously cannot ssh into the repo you pointed at (I tried to see if you had a git-daemon running, but no such luck).

Even more interesting than strace might be for you to try to run git-send-pack (even on an x86) under valgrind. valgrind really is a wonderful tool.

One other potential issue: I'm not running debian. I've run YDL, but right now the machine I use is FC4. There are likely _lots_ of library differences etc there.

		Linus

On 10/20/05, Linus Torvalds <torvalds@osdl.org> wrote:

Ok -- I seem to be wrong in blaming PPC, so it's back to cg-fetch, and I think I have a patch that does the right thing.

To recap: repo is slightly broken because cg-fetch has fetched tag objects, but hasn't followed them through to the commit objects they refer to.

Internally cg-fetch is actually using git-ssh-fetch (I misreported it using git-fetch-pack), which is working correctly. However, cg-fetch attempts to optimize the fetch process, by not calling git-ssh-fetch if it has the tagobj that the ref points to. How those tag objects get there without commits in the first place I don't know. So I've removed the optimization and life is much better.

There is a second bug during the tag fetch. Some of the references (created by git-cvsimport) are multiline, and break cg-fetch. It's probably a bug in git-cvsimport, but I'm fixing cg-fetch to use head -n1 instead of cat. I'll deal with git-cvsimport later.

On this broken-ish repo, git-fetch-pack segfaults as I've reported. Running it under strace shows that it dies walking the local repo, trying to find a missing commit. Unlikely that this is PPC only ;-)

cheers,

martin

On Thu, 20 Oct 2005, Martin Langhoff wrote:

Ok, that's not horribly surprising, but it's wrong.

Since I'm a lazy bastard, and couldn't necessarily re-create the right kind of broken repository even if I weren't, could you just enable core-dumps, and run gdb on the thing and report what "where" says is the call-chain.

It's almost certainly something that looks up an object and dereferences it without checking whether the object lookup succeeded or not. But if you can say where the SIGSEGV happens, it will be easier to figure out where.

Of course, what I really _should_ do is improve "sparse" to the point where I can mark functions as returning "unsafe" pointers, and then have sparse statically find all the dangerous users. I should do that anyway, since it would be useful for the kernel too, but it's actually quite nontrivial.

		Linus "being lazy is not a state of
			mind - it's a way of life" Torvalds

Martin Langhoff <martin.langhoff@gmail.com> writes:

When you update git-cvsimport, please make the tags it generates git-check-ref-format happy. The rules are:

 - No pathname component starting with a dot '.' (cg-fetch uses
   refs/heads/.$name-fetching to store the SHA1 it has not fully
   completed; for_each_ref() deliberately ignores path component
   that start with a dot to ignore them, probably to work this
   around).

 - No double-dots anywhere (ref1..ref2 notation is used on the
   command line and ref with embedded double-dots introduces
   ambiguities in parsing).

 - No byte with value lower than \040 (SP) nor \177 (DEL).

 - No tilde '~', caret '^' or colon ':' (the first two are used
   in extended SHA1 expressions; colon is used as src:dst
   notation in push and pull).

 - Not ending with a slash '/'.

Dear diary, on Wed, Oct 19, 2005 at 10:56:35PM CEST, I got a letter where Martin Langhoff <martin.langhoff@gmail.com> told me that...

That must be leftover of some old fetch, before cg-fetch got fixed wrt. this issue.

The "optimization" or something alike needs to be there at least for the user interface, so that we can actually say what tags are we downloading; besides, there can be a *lot* of tags. But I'm not sure how moot will this all be after Cogito moves to the remote peeking (Real Soon Now, promise ;-).

Eek. Did I miss something and are multiline refs meaningful now? If not, they are pretty bad and I'd imagine other parts of Cogito would be pretty confused about that. I'd call this a corrupted repository, tough. Perhaps a check should be added to fsck.

On 10/20/05, Petr Baudis <pasky@suse.cz> wrote:

Yes, I'm trying to figure out if I can fix it...

Actually, all the tagsrefs have only one line, but something is going weird around tagid=$(cat $tag) as $tag ends up containing many filenames. So I undid my initial change to "head -n1", and I'm trying to fix the loop.

cheers,

martin

Dear diary, on Thu, Oct 20, 2005 at 01:23:11AM CEST, I got a letter where Martin Langhoff <martin.langhoff@gmail.com> told me that...

Hmm, what bash version are you using? It's enclosed in

	for tag in *

and perhaps your bash misunderstood.

Anyway, this should've been quoted. I've fixed that in my tree, will push out soon.

Petr Baudis <pasky@suse.cz> writes:

It probably is your cat not quoting $tag, which you just fixed. Martin's repo has a conversion from CVS, with bunch of tags whose names are like "Foo **INVALID**" (that's whitespace and asterisks).

Dear diary, on Thu, Oct 20, 2005 at 02:03:52AM CEST, I got a letter where Junio C Hamano <junkio@cox.net> told me that...

Eww. Well, I pushed out the fix.

BTW (for anyone reading this), in the near future, I would like to do a complete quoting review of Cogito, since while I'm trying to be careful now, it wasn't always so, especially early on. Of course, *I* would also like to do many other things... *hint* *hint* ;-)

On 10/20/05, Petr Baudis <pasky@suse.cz> wrote:

After a bit of head-scratching, I found out what was causing this one: legacy tagnames from git-cvsimport that contain '*' in the tagname. So

   tagid=$(cat SOME_TAG **INVALID**)

gives us some really nasty surprises.

cheers,

martin