Memory overrun in http-push.c

Good day!

Spotted the memory overrun in the http-push.c. Exists at least in 1.5.0.x, not sure about latest development branch. The patch is attached.

On Wednesday 2007 February 28 15:15, Eygene Ryabinkin wrote:

A patch for git that wasn't made with git.

Don't take this the wrong way Eygene, but why?

Andy

Hi,

On Wed, 28 Feb 2007, Andy Parkins wrote:

Old habits die hard?

Ciao, Dscho

Andy, good day!

Wed, Feb 28, 2007 at 03:41:38PM +0000, Andy Parkins wrote:

Because I am trying to set up Git repository to be used over HTTPS, but it is failing now: seems like CURL library provokes the SIGSEGV. The actual patch was made for the FreeBSD ports, so I've used plain old 'diff -u'.

As Johannes pointed out, old habits are still alive. And when some tool serves me the right way, I am happy with it. To make the patch with Git I should create repository, hack there, merge it to the FreeBSD port, install and so on. A bit boring, do not take me wrong ;))

On 3/1/07, Eygene Ryabinkin <rea-git@codelabs.ru> wrote:

hacking on Git you already have a git repo, don't you? It should have been as simple as:

$ vi http-push.c ... $ git diff

:)

BTW, git diff works outside of a git repositories (well, very recent git should).

No, I was doing FreeBSD port of Git 1.5.0, so it was just the tarball. Old habits, you know ;))

Once I will get Git working for me I will get used to it and will do many things in Git.

OK, I've forgot to ask in my previous letter: will the issue with '+11' instead of '+1' be fixed (or its already fixed)? Or 11 is 10 + 1, where 10 it strlen("git") trice plus one just to be sure? ;)))

On 3/1/07, Eygene Ryabinkin <rea-git@codelabs.ru> wrote:

Your patch is already commited.

http://www.kernel.org/git/?p=git/git.git;a=commit;h=2c46759db757eb742590e8547cb0c63e8bdb1da1

Thu, Mar 01, 2007 at 10:21:47AM +0100, Alex Riesen wrote:

Thank you!

"Alex Riesen" <raa.lkml@gmail.com> writes:

You do not have to insist on using git to hack on git itself. I was more annoyed by the fact that the patch was an attachment, than it was generated by diff without leading a/ and b/ paths.

Being able to accept a patch that was made against freshly extracted tarball distribution is a good thing. I can take diffs generated by GNU diff against released tarballs just fine.

If you make it a requirement to send even an trivially obvious patch from controlled source, it would make the barrier to contribute much higher. Once you make it hard enough to submit patches, you will end up seeing only whining without patches on this list. I do not think we want to go in that direction.

On 3/1/07, Junio C Hamano <junkio@cox.net> wrote:

that wasn't it. It just haven't occurred to me at all, that someone could hack on git without using it. I feel very uncomfortable changing anything not under version control (specifically - not in git repository ;) Just haven't though of tarballs.

On Thursday 2007 March 01 09:32, Junio C Hamano wrote:

I certainly wasn't suggesting that it should be a requirement. I just thought there was a certain irony. Perhaps even a lesson to be learned - find out why someone has chosen (even with git in front of them and deep enough inside to find bugs in it), not to use git.

I think there is a usability lesson to be pulled from this situation. Why didn't Eygene just say:

 $ tar zxvf git-tarball.tgz
 $ cd git
 $ git init
 $ vim http-push.c
 $ git diff

Note, that was not an attack on Eygene - I genuinely am interested to know where we should put the documentation that tells a new user what they need to know quickly.

Somehow, git has failed this person.  I was simply wondering in what way.

However, Eygene's response made it sound like a situational thing - it was inconvenient to actually install git. Fair enough.

Andy

Good day!

So, git is unable to use this type of patches directly and I will need to do $ patch < patchfile $ rm file.orig $ git commit

Just wondering what is bad with the diffs without leading a/ and b/.

I've just realized that I need git because I need disconnected commits. That's why I left my beloved SVN and started to hack GIT ;))

By the way, I am missing one thing: the Id keyword in the file. The problem is that when some user is telling me: there is a bug in the function a() that is inside the file b.c, then I can ask him to give me the $Id$ tag of the file and I will have the full information about the file version. Can I have it with git if user has just the sources without any signs of the file versions? I've glanced over the git manual, but was unable to find anything usable.

Not incovinient to install git, but it was just convinient to me to use FreeBSD port. The other way round ;))

Eygene Ryabinkin <rea-git@codelabs.ru> writes:

In case it was unclear, I was saying that "attachment was more annoying" than "diff output that does not follow the established convention to be applied with patch -p1". Both I can handle, but dealing with the former is more work for me.

Ah, sorry, got it.

Hi,

On Thu, 1 Mar 2007, Eygene Ryabinkin wrote:

The problem is: Git does not track files, but rather trees. So, there is no sane way to add an Id.

But all is not lost! If your colleague sends you the offending file, you can calculate the object name from it by

	$ git-hash-object -t blob <filename>

If the file is too big, your colleague can run it herself and send you the object name (it is a 40 character hex string, but the first 8 or so should really be sufficient).

Then you can run

	$ git log -p HEAD <filename>

in your repository, and search for "index <object name>". You will find the commit which changed the file _from_ the state your colleague is seeing.

The idea is that the diff shows not only the names, but also a line "index <abbrev1>..<abbrev2>", where the abbrev's are the abbreviated object names of the old and new versions of that file.

BTW we had a similar discussion a while ago, which made me even write a patch for git-name-rev to name objects, too, but it was rejected as the workflow I outlined above is already sufficient for your use case.

Hth, Dscho

On Thursday 2007, March 01, Johannes Schindelin wrote:

So when you say "there is no sane way to add an Id", you meant "here is a perfect string that would act as an Id"? ;-)

Andy

Hi,

On Thu, 1 Mar 2007, Andy Parkins wrote:

Got it.

So, in a very real sense, we have file ids. Even tree ids. And commit ids.

Ciao, Dscho

On Thursday 2007, March 01, Johannes Schindelin wrote:

Absolutely. Moreso than CVS/SVN. Putting $Id$ $Rev$ in a git managed file would have far more meaning that it does in a CVS managed file.

Actually, $Id$ covers the file hash; $Rev$ covers the commit hash. I wonder if the tree id would be useful to finish the set?

The reason it's so hard to do is because putting the file id in the file would change the file, and hence its Id. Ouch. However, I reckon the upcoming gitattributes stuff is going to have no trouble keeping the file contents stable from git's point of view and yet let you mangle/unmangle the contents with this keyword stuff.

Actually once gitattributes exists we almost have to have a $Id$, the hash of the working directory file isn't stable because it might have had it's line endings mangled. On the other hand we could argue that git-hash-object should have unmangling options anyway.

Andy

Hi,

On Thu, 1 Mar 2007, Andy Parkins wrote:

No. My point was that you do not even have to have an id. The hash of the object is the id.

This is obviously much better than the mess of CVS/SVN's file ids. There is an option, even, to switch off key expansion, so you can have erroneous ids. That just cannot happen with hashes.

Of course, it does not give you any hint about when this file was current. But there is no way to tell in distributed development _anyway_. You have to look it up, when, and who, changed the file to the current state.

Ciao, Dscho

On Thursday 2007 March 01 20:43, Johannes Schindelin wrote:

I think you're thinking of these tags in too strict terms. You're right they aren't /needed/ you can query the version control system for the information. However, if you sent them out in a tarball to someone who didn't have or want to have your version control system, then it is much easier for them to be able to tell you the $Id$ field from the file itself than to generate the hash or send you a copy of the file.

They're purely for information. As I've mentioned before, personally the only place I use them is in inkscape SVG files (thank the lord SVG's are ASCII), where I add a text field that appears on the diagram itself and I write $Id$ as the text. Then whenever I print that diagram out (which makes it much harder to hash), I will know which version of the file I'm looking at. This is the /only/ thing I miss from subversion.

As I said, I don't think anyone ever imagines that these Id fields are absolute guarantees of correctness - nor does any version control system /ever/ read them and use them. They are there only as a convenience for the user. Obviously, I could just edit the $Id$ anyway if I wanted, so it obviously isn't absolute.

What about this though:
 * Tag a release
 * Export the working tree into a fresh directory
 * Edit each source file to put the hash of the tagged revision into
   every file.
 * Edit the makefile to include the tag hash in the released version
 * tar it up
 * Release
It's such a mundane, useless waste of time to edit the hash in by hand - why 
can't the version control system do it?

It's already nearly there in the form of git-describe. It's only purpose is to supply strings that describe the current revision. Well, so would $Id$, but for files instead.

I don't see that keywords are the evil they're made out to be; they're for convenience, not for absolute truth. When some random printout of the source turns up, at least the $Id$ might give you a clue as to which version it is. If it doesn't, well you're no worse off.

Here's another similar idea: generating copyright lines. Let's say we want to copyright every source file - that means writing "(C) Junio, (C) Johannes, etc" at the top of every file. Wouldn't it be nicer if we could put $Copyright$ in the file, then have some git-blame-like machinery fill in the copyrights automatically based on who's made contributions?

e.g.

 git-blame refs.c | sort -f 2 | uniq -c -s 10 -w 10 | sort -nr

Andy

Andy Parkins wrote:

What about this (that is roughtly what git and Linux kernel use):
 * Tag a release
 * Issue "make dist" which would automatically replace @@STH_VERSION@@
   (or ++STH_VERSION++ for example in Perl files) with result of
   "git describe" (although I think that plain old "make" also does this)
   _and_ create proper STH_VERSION file
 * tar it up using "git archive --format=tar" which would add version
   as a tar comment
 * Release

On Friday 2007 March 02 14:46, Jakub Narebski wrote:

Yep; you've improved the workflow for my example. However, it's not quite the same because in git the tokens in the ".perl" files are substituted into files without the suffix. This means that those files aren't the real source files. You can't put that token in the source files themselves because that would change their content and would then show up to git as changes.

Therefore git itself has to be involved with the keyword substitution, because you want it to be blind to that substitution when it's checking for changes.

Of course that's all predicated on you actually wanting keywords. For people that don't want them, I can see why the point is moot.

Andy

Hi,

On Fri, 2 Mar 2007, Andy Parkins wrote:

That's a horrible idea. A typo fix is not copyrightable. You'd add these. An obious fix is not copyrightable. You'd add these cases, too.

Besides, IMHO adding copyright should be a _concious_ act.

Ciao, Dscho

On Friday 2007, March 02, Johannes Schindelin wrote:

Well, I wasn't actually suggesting that as a final solution, it might be that it's done by more than just line count, and that some clever bit of machinery measures real contribution. That wasn't really the point.

Well, IMHO, the concious act was the commital of a change.

A lot of your objection to this seems to be "IMHO". Fine: you don't like keywords, you wouldn't be /forced/ to use them. I can see a use for them. I'm having a lot of trouble understanding why they are seen as inherently evil. Difficult to implement, I would accept, impractical to make work in git would be fine; but not this hand wavery that there is no conceivable use for them.

Andy

On Fri, 2 Mar 2007, Andy Parkins wrote:

Actually, how about doing keyword expansion just like the new CRLF handling? Have the text filter replace $Id...$ with $Id$ going into the database, and $Id$ with $Id <info>$ coming out of the database. The info could even include the commit (although that might be confusing for files that you haven't changed since you checked them out, and when you checked them out, they weren't changed in that commit; the commit would be a sort of arbitrary choice among commits that have that file).

It seems to me like keywords fit the pattern we now have (with CRLF-conversion) of data which isn't tracked, but is present in working directory files. In the CRLF case, it's a constant character (CR), but there's no reason we couldn't have the non-canonical form depend on more information. (And we'd beat CVS/SVN for accuracy: if keyword substitution isn't on, nothing appears in $Id$ tags, rather than being left with something inaccurate).

	-Daniel
*This .sig left intentionally blank*

Daniel Barkalow <barkalow@iabervon.org> writes:

That part is true, but practically speaking the only thing you can honestly replace is $Id$ with blob object name, and nothing else. Trying to put anything that is available via commit would at best telling a lie and at worst ending up denying the index (and denying the index is denying git as somebody said). You could fake the values for $Author$ and $Date$ from HEAD commit, but your index may or may not match HEAD commit when you check out path from it.

On Sun, 4 Mar 2007, Junio C Hamano wrote:

I was somehow expecting the index to be on the "working directory" side of conversions, but that obviously doesn't work very well (since you'd get a ton of unreachable blobs all the time that way). Even so, I think the useful thing would the the "git-describe" for HEAD, if the index entry is actually from HEAD (otherwise, it's a state that nobody else has seen, right? No point in trying to specify further...). I don't think Author and Date from a commit really works, because they wouldn't actually reflect the last change to anything near the tag, necessarily. The commit hash, on the other hand, tends you what to run blame on.

But, in any case, $Id <blob hash>$ would put into the working directory file (and suitable installed derivates thereof) something that would lead people with the repository to the actual blob, with at least a possible context. $Id <commit hash>:<path>$, for <commit hash>=HEAD, if HEAD:<path> is <blob hash>, should be just right.

Is it common to check out files from the index which don't match HEAD? The only case I can think of is resetting to a state stored in the index with update-index. I think my personal workflow is always HEAD->index->working directory->index->new HEAD.

	-Daniel
*This .sig left intentionally blank*

On 3/1/07, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Wont work. The ids in question are really strings embedded into object. Something like:

  $Id 1.2 user filename.c$

They can be found, extracted from the binary and sent to developer with a bug report. It's more of a build signature. IOW, the colleague in question has to have the source, which, I suspect, he wont.

Of course, it's very simple to embed the HEAD into build and tell the tester to send the SHA-1 in case of bug report.

Alex Riesen <raa.lkml@gmail.com> wrote:

Yes. Either the commit SHA-1 or the output of git-describe does the trick. Even better when your program prints this value on demand, like say `git version`. ;-)

Before programmers got smart and wrote switches to dump the version (aka -v/-V/--version) users were expected to use `strings frob | grep '$Id' | mail baddev@example.com` to send in bug reports. What a world we live in! ;-)

Hello Eygene,

Git and the linux kernel (can) have the commit hash in their version.

For git you can see it with

	$ git version
	git version 1.5.0.2.260.g2eb065

for the linux kernel you have to enable CONFIG_LOCALVERSION_AUTO, then you get:

	$ uname -r
	2.6.20-g509cb37e

Knowing the commit you can determine the corresponding "version" of b.c.

There are some other possiblities to communicate that hash.  E.g.
a tar archive created with
	
	$ git-archive --format=tar ...

allows you do to

	$ git-get-tar-commit-id < $tarfile
	2eb06531e3d53c2604f20c32e5cb791d5044ff02

Best regards Uwe

Eygene Ryabinkin wrote:

 
First, keyword expansion on checkout (CVS-like) is an abomination;
the proper way is to do it on _build_ time, like git and Linux kernel
do, so "git version" (or "git --version") and "uname -a" gives us
the version which can be used with git to go to it.

Second, there were some work on git mailing list to create git command which given a blob (or sha1 of a blob object) would list all commits (in date order) which have this exact version of a file (of a blob).

Third, tar files generated by git-acrhive have version embedded as a comment in them.

Hi,

On Fri, 2 Mar 2007, Jakub Narebski wrote:

It was shot down, and rightly so.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

I think this "rightly" needs a clarification (otherwise Jakub wouldn't have said the above).

It wasn't that identifying the commit that contained the version of the file was not needed. It is just there already is a way to do so, and "to create git command" turns out to be unnecessary. Something like:

	$ hash=`git hash-object --stdin <$file_in_question`
	$ git log -z --raw -r --abbrev=40 |
          grep -z '^:[0-7][0-7]* [0-7][0-7]* [0-9a-f][0-9a-f]* '"$hash"'

On Fri, 2 Mar 2007, Junio C Hamano wrote:

That's not very nice, because if the blob is already in the *top* commit, and isn't changed in a long long time, you'll not see it until it changes (which is potentially at the root commit when it was introduced).

So it's better to do something like

 - get the hash

	hash=`git hash-object --stdin <$file_in_question`

 - see if it is in the current top of the tree

	git ls-tree -r HEAD | cut -d3- | grep ^$hash

 - after that, start doing the backwards searching for things where it 
   went away if it was there before (roughly your "git log -z ...")

which should be fairly simple and straightforward.

For extra bonus points, the script could be trivially written so that it allows the user to specify a path limiter, so that you say "can you tell me which version this blob existed in, but I only want to know about drivers/scsi/ and include/scsi/"

		Linus

On Friday 2007 March 02 14:38, Jakub Narebski wrote:

What about my SVG example?  There is no build time, the file is the file.

Andy

Andy Parkins <andyparkins@gmail.com> writes:

Same with manually written HTML, or even just plain text files, or ...

A typical other example is when you're working with people having some kind of alergy to revision control systems. Then, you have to send the source, and they send you back the modified source. That's very hard to manage without some kind of timestamp within the file.

However, I have to admit that workflows requiring keyword expansions still appear dirty to me.

A (dirty too) solution is to have a pre-commit hook that would update the keywords for each modified file. Then, the keyword is in the source itself.

On Friday 2007 March 02 15:30, Matthieu Moy wrote:

That would be bad for merges though. I think it's got to be filtered out in a pre-commit or equivalent script.

Andy

* Eygene Ryabinkin:

Is this issue security-relevant? After all, the misplaced pointer is dereferenced and written to.

Florian, good day!

It can be relevant: basically, it is the heap overflow, because 'url' is allocated by xmalloc. Did not tried to exploit it, but old sudo exploit proved that even one byte off memory dereference can be exploited. But this particular exploit will work only for the URLs where the 'path' is shorter than 10 bytes. And I doubt that many people are running http-push in the set-uid mode, so the exploit target will be the person running git-push over HTTP, not the root user. But it does not mean that this is not an issue.

Sorry for the long letter.