git push over http is very dangerous

git push over http is very dangerous

[Christian]

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

Re: git push over http is very dangerous

[Linus Torvalds]

On Mon, 16 Apr 2007, Christian wrote:
>
> 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.

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

Re: git push over http is very dangerous

[Junio C Hamano]

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

> On Mon, 16 Apr 2007, Christian wrote:
>>
>> 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.
>
> I have to agree: pushing over http really is dangerous.
> ...

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'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).

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

Re: git push over http is very dangerous

[Linus Torvalds]

On Mon, 16 Apr 2007, Junio C Hamano wrote:
> 
> 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 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..

> 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.

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

Re: git push over http is very dangerous

[Christian]

[-- Attachment #1: Type: text/plain, Size: 2630 bytes --]

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:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
>   
>> On Mon, 16 Apr 2007, Christian wrote:
>>     
>>> 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.
>>>       
>> I have to agree: pushing over http really is dangerous.
>> ...
>>     
>
> 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'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).
>>     
>
> 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
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   


[-- Attachment #2: git-debug.tar.gz --]
[-- Type: application/gzip, Size: 1045 bytes --]

Re: git push over http is very dangerous

[Junio C Hamano]

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

> 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

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:

> error: corrupt loose object 'cd1aac1a43cfdac07118240f75c0ba7662eb8140'

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?

Re: git push over http is very dangerous

[Christian]

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:
>
>   
>> 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
>>     
>
> 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:
>
>   
>> error: corrupt loose object 'cd1aac1a43cfdac07118240f75c0ba7662eb8140'
>>     
>
> 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?
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

Re: git push over http is very dangerous

[Junio C Hamano]

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

> 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 ?

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.

Re: git push over http is very dangerous

[Christian]

Junio C Hamano wrote:
> Christian <crich-ml@beronet.com> writes:
>
>   
>> 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 ?
>>     
>
> 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.
>   
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.
> 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.
>
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>