git push over http is very dangerous

Hi Git developers,

first of all thanks for the great toolkit :)

We are using git for the mISDN Open Source ISDN Stack (www.misdn.org / git.misdn.org). We are using webdav to push up data from users with write access. Readonly access is granted via the git daemon.

We have moved from CVS to git in the beginning of last week, all went well until this weekend. This weekend one developer wanted to push some of his local modifications, unfortunately during the push his http connection seemed to have broken or so. Unfortunately git does not prove if the push went well. Therefore our repository was broken this morning.

We found out that it was broken, because we could not do a pull or clone from the central repository anymore, but we got an error message.

Then i did a "git log" which worked well. then i wanted to "show" the last 2 commits, so i did: "git show":

error: corrupt loose object 'cd1aac1a43cfdac07118240f75c0ba7662eb8140' <crich1999> error: cd1aac1a43cfdac07118240f75c0ba7662eb8140: object corrupt or missing

i found that i could "git show" all commits, but only the last 2 produced the above error message.

gitster from IRC helped me out and showed me that i could do

git branch -f master master~2 

to remove the 2 commits. i did that and all worked well again!

I wonder if it would be possible to delay a central push until all data was uploaded, then after the upload finished a checksum is transmitted, if the checksum is correct, the push can be done centrally..

cheers,

christian

On Mon, 16 Apr 2007, Christian wrote:

I have to agree: pushing over http really is dangerous. It's not *supposed* to be (it tries to update the refs only after it has uploaded all objects), but it fundamentally cannot do all the validity tests that the "real" git transfer processes do on the receiving side.

And I think git-http-push is pretty fundamentally broken anyway. It doesn't really seem to check for errors. So it doesn't do some of the checks it *could* do.

I would strongly suggest against http pushing (I'd suggest against pulling too, but at least you can't screw up too badly by just reading ;)

I'd also love it if somebody were to actually look into making http-pushing a bit safer. It really needs somebody who cares about it, or it should likely just be disabled entirely (perhaps with a config option that you have to enable to get it - so that people *realize* that it's not maintained and not really supported).

		Linus

Linus Torvalds <torvalds@linux-foundation.org> writes:

Just for the record, I do not think anybody during that #git discussion actually proved that http-push was the culprit. It is a very plausible working conjecture, though.

I do not know if Nick is still using his own http-push (or if he is still using git for that matter), but just in case he may still be interested, I am cc'ing this message to him.

I think the fetch side does the right thing, more or less, by downloading to a temporary file and using move_temp_to_file() after validating the SHA-1 matches. I haven't followed the push side but as we do not have a single line of code on the receiving end, I would not be surprised if there is no error checking beyond HTTP response code would give the pushing end.

I would still love to see the corrupt loose object to see how it is broken.

Christian, can you do this with the first (i.e. older) commit that is broken, and tar up these 7 files for the initial round of inspection?

	$ git cat-file commit $commit >commit-text 2>commit-error
        $ git ls-tree ${commit} >toplevel-tree 2>toplevel-tree-error
        $ git ls-tree -r -t ${commit} >whole-tree 2>whole-tree-error
	$ cp .git/objects/cd/1aac1a43cfdac07118240f75c0ba7662eb8140 corrupt

On Mon, 16 Apr 2007, Junio C Hamano wrote:

I looked at http-push.c once more, and there is a very marked lack of any error testing. It actually tries to be pretty careful, ie it seems that every PUT request is always to a temp-file, and then it does a MOVE request after that, and things seem to properly abort on most errors, but the actual data integrity is obviously impossible to check on the remote, and a quick grep showed that not all errors even set "aborted", which would seem to imply that certain error conditions can happen without the http-push then aborting the ref update.

For example, if "start_active_slot()" fails, aborted isn't generally set. I don't know if that is ever a problem (it can only trigger with USE_CURL_MULTI), but it's an example of what looks pretty fragile.

So we can fix up some of these kinds of things, but considering that we can't really validate the end result on the remote, I'd still personally be quite leery of pushing by http..

Yeah, on the pulling side we are simply much better off, because we can validate things after the operation has finished. On the pushing side, we could obviously try to re-download the objects or something, but basically validation would literally have to involve doubling the network usage, and even then we might get screwed by some caching layer!

		Linus

hi Junio,

today i got access to the machine with the corrupt git blobs, attached are the requested files, but there was no error, so i've not attached the output from stderr.

I hope this helps.

Christian

Junio C Hamano wrote:

Christian <crich-ml@beronet.com> writes:

Thanks. I extracted that "corrupt" object, but all the archived versions of git I have at hand can read that tree just fine.

  $ ~/git-snap-v1.0.0/bin/git ls-tree cd1aac
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN
  $ ~/git-snap-v1.2.0/bin/git ls-tree cd1aac
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN
  $ ~/git-snap-v1.3.0/bin/git ls-tree cd1aac
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN
  $ ~/git-snap-v1.5.0/bin/git ls-tree cd1aac
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN
  $ ~/git-snap-v1.5.1/bin/git ls-tree cd1aac
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN
  $ git ls-tree cd1aac ;# this is 'next'
  100644 blob 660424785bb8deda22ae5055fae1b16c466bc602	Kconfig.v2.6
  100644 blob 29553e41ee8d0941b1c119a3fdd4fcb9fa8cc44b	Makefile.v2.6
  040000 tree 3ba0db3caae4263808c925a598d707aaf4f82d68	mISDN

I am starting to suspect that the loose object is actually Ok and you are suffering from something else (perhaps zlib).

We had a brief period that we had a bogusly strict error check after calling zlib, which Linus fixed with 456cdf and ac54c2 (these fixes are both in v1.5.1). The breakage was 7efbff, which was post v1.5.0.3 breakage, but was never been part of any official release without the two fixes from Linus.

HOWEVER (I mentioned the above paragraph to save people from going wild goose chase), this does not seem to be related to the breakage. I specifically built that revision and ls-tree from it reads the loose object just fine.

There is only one codepath that can emit this error message:

so I am reasonably sure that you are running 7efbff or later version, but now I am out of ideas.

What version of git do you run?  And on what platform?

I've did that ls-tree thingy again additionally an ls-tree on a working blob:

m24s12:/var/git/mISDN.git-backup2# git-ls-tree cd1aac1a43cfdac07118240f75c0ba7662eb8140 error: corrupt loose object 'cd1aac1a43cfdac07118240f75c0ba7662eb8140' fatal: not a tree object m24s12:/var/git/mISDN.git-backup2# ls objects/ Display all 163 possibilities? (y or n) m24s12:/var/git/mISDN.git-backup2# ls objects/aa/ 537a125359b729699b3c011fbf9d71438de90d d38d3dedd00107dc207190b9fa0b22f53888de m24s12:/var/git/mISDN.git-backup2# git-ls-tree aa537a125359b729699b3c011fbf9d71438de90d 100644 blob f6998854080c88903e47d449affc891278184368 CHANGES 100644 blob 0ac604f5c8c3b3a55fd0ce50944bb6f7f85850b3 Makefile 100644 blob 7b90a319e218d962dc453c5a92b8ee11c6bb9b7d Makefile.module 100644 blob 56617b776024936b932601697ccfb548d891dadc Makefile.standalone 100644 blob c12695b2e27a3be524e8e2622f79abf638dd9fb1 Rules.make.ext 100644 blob 1808a7786f86bf2ad9d15d2b7a921474bef00951 add.config 040000 tree f422c9f8d89e2d5c8be05392522d79f064522d64 config 040000 tree c875ea5c0cb3a5597df481f2999f9c6dc1691829 drivers 040000 tree f1cb948bddd3a86d6a1981a903edb102bd8244cf include 100644 blob f11f1a276f4000b7bc703c40c3ad7a959ccf9146 km_mISDN.spec 100755 blob 81c7103e38ef807e84880625ef484d50c4cc9cc3 std2kern 100755 blob 07ba5902616674ddee962e89570c85f9e75fbd4b stddiff m24s12:/var/git/mISDN.git-backup2# git --version git version 1.5.1.1

but i had a 1.5.0.X version before, which emitted the same problem :(

On Friday i had access to the machine which created the corrupt object, then we did a new push from that machine, which again corrupted our central repository, i found that it seemed not http - push related, because i could see the push worked well.

Doing that was very unfortune, because we lost a lot of commits this time :( I told that guy to get a new git version (he had 1.5.0.1 or so) and make a fresh clone of our repository.

So i'm pretty sure this is *not* a http push problem.

Interestingly the git-ls-tree on the machine of that guy worked very well, as it does on my local machine, but it does not work on our central repository.

The central machine is a AMD Sempron, it runs Debian Sarge (3.1) We've installed git from source, because sarge has it not in its archives.

Maybe we did something wrong at building time ? or you're right and the debian sarge zlib does not fit good to the current git versions ?

Junio C Hamano wrote:

Christian <crich-ml@beronet.com> writes:

What I have been suspecting since I got that not-corrupted but reported to be corrupt tree is that the version running on your "unable to fsck" machine -- the one you use as the central machine -- has a dud (or miscompiled) version of git, which somehow reports corruption incorrectly on perfectly good objects when it validates it before using them.

And to that machine, push over http succeeds fine only because push over http is "dangerous" in that it does not "validate" the pushed data on the receiving end like other codepaths do.

It would be interesting to find out what the breakage is on that central machine. I think enough people run Sarge that it is unlikely it's zlib at fault, but the way git uses the zlib API, which was cleaned up in the current tree with the two fixes from Linus I mentioned earlier.

Junio C Hamano wrote:

well if you like i can give you an account to that machine offlist, i could show you in a screen session where everything is.