Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[皐月]

iPhoneから送信

My git repo is broken, how to fix it ?

[Alexander Litvinov]

Hello,

I use manualy compiled git under cygwin. Some time ago I have imported project 
from CVS and start to us it under git. To emulate 'separate remotes' schema 
for branches from CVS I clone imported git repo and work with it. From time 
to time I incrementaly update imported repo from cvs and sometimes use 
git-cvsexportcommit (from work repo) to export my changes and then get them 
using git-cvsimport.

Some times ago I descide to run fsck and found that by working repo is broken, 
while imported repo is correct. Is there way to fix it ? 

>git version
git version 1.5.0.GIT
(It was actualy compiled from e86d552 commit)
> git fsck
> git fsck --full
error: packed 7f5fed8131fb32972c602dede29b9257a053ba67 
from .git/objects/pack/pack-c4554978bbe079c9a43d6a13546a2fa314fe0884.pack is 
corrupt
sha1 mismatch 7f5fed8131fb32972c602dede29b9257a053ba67

(This is a blob, git cat-file blob 7f5fed813 shows me my c++ header file that 
is partialy broken with ^@ symbols)

The repo I get using git-cvsimport is correct and does not contains that blob. 
I also tried git-log -p for all by branches to force git to show me what is 
the commit was broken but git-log finished without errors.

By the way, several times I interrupt git's commands like commit and pull 
using Ctrl-C.

I tried to unpack all objects:
> git-unpack-objects -r 
< .git/objects/pack/pack-c4554978bbe079c9a43d6a13546a2fa314fe0884.pack; echo 
$?
Unpacking 12868 objects
 100% (12868/12868) done
0

No erorts here. But fsck find that broken blob:
> git fsck 
dangling blob beb992198d4d8813ea51fd1cbbf38313ef490c22

git-cat-file shows me this this is a broken object with correct sha1 sum.


As a cunclusion: my repo has broken file and I don't see there is the brakage. 
Can I reconstruct file by sha1 sum :-) or can I do something to stop fsck 
warn me ?

Re: My git repo is broken, how to fix it ?

[Linus Torvalds]

[ Git list cc'd again - I modified your branch names and commit header 
  line just in case you care about those ]

On Wed, 21 Mar 2007, Alexander Litvinov wrote:
> 
> I have a good news : I got the breakage again. And I can reproduce it :-)
> 
> This is a version of git with three your patches.
> 
> Here is the steps to broke my repo:

So does it break every time if you do this particular sequence with the 
particular state that it has?  If so, wonderful, since it should mean that 
you can also recreate the file that got corrupted as a blob.

> $ git prune
> $ git fsck --full
> dangling commit 50267ccaa820c456bd361db808f99d81714cbce8
> $ git rebase fix-autoxyzzy                                    
> First, rewinding head to replay your work on top of it...
> HEAD is now at 42af3b2... Replace ...
> Applying 'Show ...'
> 
> Wrote tree 851c5d8d2213c60efc1bd081b0012bfcc9e558b5
> Committed: e7117e5637e881368ff04e94a27dca2abdb12d38

And then..

> [lan@ac-7923bb4c6c14 navitel (debug-autoxyzzy)]$ git fsck --full
> error: corrupt loose object 'c01848491b53c3dcfd738149193a14d3c9abe107'
> error: c01848491b53c3dcfd738149193a14d3c9abe107: object corrupt or missing
> missing blob c01848491b53c3dcfd738149193a14d3c9abe107
> dangling commit 50267ccaa820c456bd361db808f99d81714cbce8
> 
> What can I do to debug this ?

Every time there's a corrupt object, if you can send it to me, that would 
be good. If you can tell the source for the corrupt object and can send 
that to me too, that's always even better, but even in the absense of 
that, the more corrupt objects I have, the better the chances that I see 
some pattern. And if it's always the same object that gets corrupted the 
same way when you start from a particular starting point, that would also 
be very interesting to know.

Considering that the "don't use hardlinks on cygwin" thing didn't matter 
for you (and really, I would have only expected it to matter if you used 
^C to kill a process in the middle or something), you migth also be better 
off just trackng the standard git, since it now has Nicos extra 
consistency checks over and beyond those I send you. 

It's also possible that the real bug is that we have some memory scribble 
internally in git, and that it shows up for you just because Cygwin and/or 
WinXp has different allocation patterns than other platforms. Do you know 
if there are any "debugging malloc" libraries for Cygwin? Something like 
ElectricFence/dmalloc under Linux, or running with valgrind.

Since it happens after a single rebase, if it's a git bug (as opposed 
to,for example, a zlib problem or simply a problem in your combination of 
vmware/winxp/cygwin), it would be the recursive merge that screws up. It 
*is* one of the more complex operations (especially if it also ends up 
doing file-level merging, which I assume it does), so some memory 
allocation problem there is not out of the question, although it's strange 
that you see it but the (many more) users on UNIX never seem to - it's not 
like rebase is an uncommon operation!

		Linus

Re: My git repo is broken, how to fix it ?

[Linus Torvalds]

On Thu, 22 Mar 2007, Linus Torvalds wrote:
> 
> It's also possible that the real bug is that we have some memory scribble 
> internally in git, and that it shows up for you just because Cygwin and/or 
> WinXp has different allocation patterns than other platforms. Do you know 
> if there are any "debugging malloc" libraries for Cygwin? Something like 
> ElectricFence/dmalloc under Linux, or running with valgrind.

Yeehaa! I think I'm on the right trail.

Git people: do this:

	yum install ElectricFence

(or similar, apt-get, whatever), and then apply this patch, and do

	make test

and it will fail in "git-apply"! Which (having read Alexander's corruption 
sequence once more) must have been what corrupted things for Alexander 
too!

I've not debugged it any more, but gdb on the core-dump shows:

	(gdb) where
	#0  0x0000003768462331 in SHA1_Update () from /lib64/libcrypto.so.6
	#1  0x0000000000461b0e in write_sha1_file_prepare (buf=0x2ba371737fd0, len=46, type=0x49a50b "blob", sha1=0x7fff395c84a0 "",
	    hdr=0x7fff395c8480 "blob 46", hdrlen=0x7fff395c847c) at sha1_file.c:1823
	#2  0x0000000000461e6f in write_sha1_file (buf=0x2ba371737fd0, len=46, type=0x49a50b "blob", returnsha1=0x2ba371733fe0 "")
	    at sha1_file.c:1962
	#3  0x000000000040aa9a in add_index_file (path=0x2ba371719ffc "one", mode=33188, buf=0x2ba371737fd0, size=46) at builtin-apply.c:2350
	#4  0x000000000040aeb5 in create_file (patch=0x2ba37170cf40) at builtin-apply.c:2451
	#5  0x000000000040af45 in write_out_one_result (patch=0x2ba37170cf40, phase=1) at builtin-apply.c:2475
	#6  0x000000000040b291 in write_out_results (list=0x2ba37170cf40, skipped_patch=0) at builtin-apply.c:2560
	#7  0x000000000040b71c in apply_patch (fd=6, filename=0x7fff395c96ec "patch.file", inaccurate_eof=0) at builtin-apply.c:2676
	#8  0x000000000040bd1b in cmd_apply (argc=3, argv=0x7fff395c8990, unused_prefix=0x0) at builtin-apply.c:2836
	#9  0x0000000000403fbb in handle_internal_command (argc=3, argv=0x7fff395c8990, envp=0x7fff395c89b0) at git.c:322
	#10 0x0000000000404193 in main (argc=3, argv=0x7fff395c8990, envp=0x7fff395c89b0) at git.c:391

so I thought I'd send out this email asap, in case somebody else finds the 
bug before I do.

Anyway, this looks like a real smoking gun..

		Linus
----
diff --git a/Makefile b/Makefile
index 51c1fed..7e20410 100644
--- a/Makefile
+++ b/Makefile
@@ -123,7 +123,7 @@ uname_P := $(shell sh -c 'uname -p 2>/dev/null || echo not')
 
 # CFLAGS and LDFLAGS are for the users to override from the command line.
 
-CFLAGS = -g -O2 -Wall
+CFLAGS = -g -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
@@ -647,7 +647,7 @@ prefix_SQ = $(subst ','\'',$(prefix))
 SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 
-LIBS = $(GITLIBS) $(EXTLIBS)
+LIBS = $(GITLIBS) $(EXTLIBS) -lefence
 
 BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
 	-DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"' $(COMPAT_CFLAGS)

Re: My git repo is broken, how to fix it ?

[Linus Torvalds]

On Thu, 22 Mar 2007, Linus Torvalds wrote:
> 
> Yeehaa! I think I'm on the right trail.

.. and the reason only Alexander sees it, and nobody else does, is that 
this one is a bug in the CR/LF creation. 

Junio: I think it's your git-apply commit 67160271.

In "try_create_file()", we do:

	...
        if (convert_to_working_tree(path, &nbuf, &nsize)) {
                free((char *) buf);
                buf = nbuf;
                size = nsize;
        }
	...

but the thing is, the *caller* still uses the old "buf/nsize", so when you 
free it, the caller will now use the free'd data structure, and if it gets 
re-used by - for example - the zlib deflate() buffers, you'll get a 
corrupt object (if it gets re-used *before*, you'll get the *wrong* 
object!). Exactly Alexander's patterns.

Alexander - sorry for all the trouble, this was definitely our bad.

I think the easy temporary fix is to just remove that "free()" and leak a 
bit of memory. That gets it through that test with efence for me.

Does that fix it for you, Alexander?

I can't really say whether there are other problems too - electric fence 
has a few bugs in that it considers zero-length allocations to be 
"probably a bug" and aborts. This makes some of the tests fail with 
efence, when re_compile_internal wants to allocate a zero-length object.

(It also writes crap to stderr, which could make others fail, I didn't 
check).

		Linus

[PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Junio C Hamano]

When we write out the result of patch application, we sometimes
need to munge the data (e.g. under core.autocrlf).  After doing
so, what we should free is the temporary buffer that holds the
converted data returned from convert_to_working_tree(), not the
original one.

This patch also moves the call to open() up in the function, as
the caller expects us to fail cheaply if leading directories
need to be created (and then the caller creates them and calls
us again).  For that calling pattern, attempting conversion
before opening the file adds unnecessary overhead.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

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

  > In "try_create_file()", we do:
  >
  > 	...
  >         if (convert_to_working_tree(path, &nbuf, &nsize)) {
  >                 free((char *) buf);
  >                 buf = nbuf;
  >                 size = nsize;
  >         }
  > 	...
  >
  > but the thing is, the *caller* still uses the old "buf/nsize", so when you 
  > free it, the caller will now use the free'd data structure, and if it gets 
  > re-used by - for example - the zlib deflate() buffers, you'll get a 
  > corrupt object (if it gets re-used *before*, you'll get the *wrong* 
  > object!). Exactly Alexander's patterns.
  >
  > Alexander - sorry for all the trouble, this was definitely our bad.

  I concur.  Sorry for this, Alexander.

  git-apply in general is quite leaky (e.g. it never frees a
  finished patch, and freeing a patch is quite difficult as some
  strings like filenames are shared without refcounting), but
  this part deals with a large amount of data and I would rather
  not add a new one.

 builtin-apply.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index dfa1716..27a182b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2355,7 +2355,7 @@ static void add_index_file(const char *path, unsigned mode, void *buf, unsigned
 
 static int try_create_file(const char *path, unsigned int mode, const char *buf, unsigned long size)
 {
-	int fd;
+	int fd, converted;
 	char *nbuf;
 	unsigned long nsize;
 
@@ -2364,17 +2364,18 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 		 * terminated.
 		 */
 		return symlink(buf, path);
+
+	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
+	if (fd < 0)
+		return -1;
+
 	nsize = size;
 	nbuf = (char *) buf;
-	if (convert_to_working_tree(path, &nbuf, &nsize)) {
-		free((char *) buf);
+	converted = convert_to_working_tree(path, &nbuf, &nsize);
+	if (converted) {
 		buf = nbuf;
 		size = nsize;
 	}
-
-	fd = open(path, O_CREAT | O_EXCL | O_WRONLY, (mode & 0100) ? 0777 : 0666);
-	if (fd < 0)
-		return -1;
 	while (size) {
 		int written = xwrite(fd, buf, size);
 		if (written < 0)
@@ -2386,6 +2387,8 @@ static int try_create_file(const char *path, unsigned int mode, const char *buf,
 	}
 	if (close(fd) < 0)
 		die("closing file %s: %s", path, strerror(errno));
+	if (converted)
+		free(nbuf);
 	return 0;
 }
 

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Linus Torvalds]

On Thu, 22 Mar 2007, Junio C Hamano wrote:
> 
> This patch also moves the call to open() up in the function, as
> the caller expects us to fail cheaply if leading directories
> need to be created (and then the caller creates them and calls
> us again).  For that calling pattern, attempting conversion
> before opening the file adds unnecessary overhead.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>

Ack, looks good.

		Linus

Re: [PATCH] git-apply: Do not free the wrong buffer when we convert the data for writeout

[Alexander Litvinov]

В сообщении от Friday 23 March 2007 02:55 Linus Torvalds написал(a):
> On Thu, 22 Mar 2007, Junio C Hamano wrote:
> > This patch also moves the call to open() up in the function, as
> > the caller expects us to fail cheaply if leading directories
> > need to be created (and then the caller creates them and calls
> > us again).  For that calling pattern, attempting conversion
> > before opening the file adds unnecessary overhead.

I have applied this patch ontop of next (d06644b) and it also fix by repo 
breakage. 

Thanks for help !
Alexander Litvinov.