[BUG] git cat-file does not terminate

[BUG] git cat-file does not terminate

[Robert Wruck]

Hi,

this is some strange behaviour of cat-file:
On a certain file, `git cat-file blob <objectname>` writes an endless 
stream repeating the first 4096 byte of the original file.
cat-file -s and cat-file -t produce correct results.

Even stranger: This only happens with cygwin-git (1.7.4.1).
msysgit (same machine, same repository): works
linux-git (same machine, same repository): works

Even more strange: This only happens with cygwin on a particular machine 
(recent cygwin1.dll 1.7.8) under WinXP/32bit. On another machine, recent 
cygwin, Windows7/64bit it works...

Debugging a bit, I found that the following happens:
In xwrite (wrapper.c), write() is called with the total file size - in 
my case about 87 MB. This call returns -1 and EAGAIN but nevertheless 
writes 4096 byte to the output fd. I don't think that's expected 
behaviour...

I "fixed" it by limiting each write to 64k (thus looping in 
write_in_full) but maybe somebody knows about that cygwin behaviour?

This seems to be the cause of the dreaded "No newline found after blob" 
when running `git svn clone` under cygwin on a repository with large files.

You could argue that this is a cygwin bug but maybe limiting each write 
to a maximum size is a simple workaround.


-Robert

Re: [BUG] git cat-file does not terminate

[Peter Baumann]

On Fri, Mar 04, 2011 at 02:04:00PM +0100, Robert Wruck wrote:
> Hi,
> 
> this is some strange behaviour of cat-file:
> On a certain file, `git cat-file blob <objectname>` writes an
> endless stream repeating the first 4096 byte of the original file.
> cat-file -s and cat-file -t produce correct results.
> 
> Even stranger: This only happens with cygwin-git (1.7.4.1).
> msysgit (same machine, same repository): works
> linux-git (same machine, same repository): works
> 
> Even more strange: This only happens with cygwin on a particular
> machine (recent cygwin1.dll 1.7.8) under WinXP/32bit. On another
> machine, recent cygwin, Windows7/64bit it works...
> 
> Debugging a bit, I found that the following happens:
> In xwrite (wrapper.c), write() is called with the total file size -
> in my case about 87 MB. This call returns -1 and EAGAIN but
> nevertheless writes 4096 byte to the output fd. I don't think that's
> expected behaviour...
> 
> I "fixed" it by limiting each write to 64k (thus looping in
> write_in_full) but maybe somebody knows about that cygwin behaviour?
> 
> This seems to be the cause of the dreaded "No newline found after
> blob" when running `git svn clone` under cygwin on a repository with
> large files.
> 
> You could argue that this is a cygwin bug but maybe limiting each
> write to a maximum size is a simple workaround.
> 
Maybe you could post a patch, so everyone can see the technical implications
and discuss the fix?

-Peter

Re: [BUG] git cat-file does not terminate

[Jeff King]

On Fri, Mar 04, 2011 at 04:40:14PM +0100, Peter Baumann wrote:

> > I "fixed" it by limiting each write to 64k (thus looping in
> > write_in_full) but maybe somebody knows about that cygwin behaviour?
> > 
> > This seems to be the cause of the dreaded "No newline found after
> > blob" when running `git svn clone` under cygwin on a repository with
> > large files.
> > 
> > You could argue that this is a cygwin bug but maybe limiting each
> > write to a maximum size is a simple workaround.
> > 
> Maybe you could post a patch, so everyone can see the technical implications
> and discuss the fix?

It would probably look like the patch below, though it really feels like
the right solution is to fix the cygwin bug.

-Peff

---
diff --git a/Makefile b/Makefile
index 4c31d1a..e7d3285 100644
--- a/Makefile
+++ b/Makefile
@@ -167,6 +167,9 @@ all::
 # Define NO_ST_BLOCKS_IN_STRUCT_STAT if your platform does not have st_blocks
 # field that counts the on-disk footprint in 512-byte blocks.
 #
+# Define MAX_WRITE_SIZE to N if your platform has unpredictable results for
+# write() calls larger than N (e.g., cygwin).
+#
 # Define ASCIIDOC7 if you want to format documentation with AsciiDoc 7
 #
 # Define DOCBOOK_XSL_172 if you want to format man pages with DocBook XSL v1.72
@@ -928,6 +931,7 @@ ifeq ($(uname_O),Cygwin)
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
 	NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
 	NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease
+	MAX_WRITE_SIZE=65536
 	# There are conflicting reports about this.
 	# On some boxes NO_MMAP is needed, and not so elsewhere.
 	# Try commenting this out if you suspect MMAP is more efficient
@@ -1495,6 +1499,10 @@ ifdef NO_POSIX_GOODIES
 	BASIC_CFLAGS += -DNO_POSIX_GOODIES
 endif
 
+ifdef MAX_WRITE_SIZE
+	BASIC_CFLAGS += -DMAX_WRITE_SIZE=$(MAX_WRITE_SIZE)
+endif
+
 ifdef BLK_SHA1
 	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
diff --git a/wrapper.c b/wrapper.c
index 056e9d6..a7a2437 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -133,6 +133,10 @@ ssize_t xread(int fd, void *buf, size_t len)
 ssize_t xwrite(int fd, const void *buf, size_t len)
 {
 	ssize_t nr;
+#ifdef MAX_WRITE_SIZE
+	if (len > MAX_WRITE_SIZE)
+		len = MAX_WRITE_SIZE;
+#endif
 	while (1) {
 		nr = write(fd, buf, len);
 		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))

Re: [BUG] git cat-file does not terminate

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It would probably look like the patch below, though it really feels like
> the right solution is to fix the cygwin bug.

Thanks for a quick analysis and fix, folks.

We would need to assume certain things that the platform would give its
users are reliable, and system calls are fundamental part; otherwise we
would end up tying our hands behind our back saying "we cannot use this
and that as these are unreliable in certain places" and litter our
codebase with full of ifdefs and workarounds.

If this were merely of a breakage in a test release or a nightly build, I
would be happier to see us ignore this, but the problematic one is widely
in the wild, a workaround would be necessary, and more importantly, if it
is harder for a casual end-user to tell if the platform is affected (I am
assuming that most releases of Cygwin is without this bug, and most users
who build git themselves wouldn't bother reading README or Makefile even
if we said a MAX_WRITE_SIZE definition is necessary only for this and that
version), I would rather see a change that covers the problem a bit more
widely than necessary.

How prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did
this bug exist, in other words, if we were to make a table of problematic
versions, would we have only just a handful entries in it?  Also can we at
runtime find out what version we are running?

The reason I am asking these questions is because I think, assuming that
this would affect many unsuspecting Cygwin users, the best fix would be to
add a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround
is necessary at runtime, and do something like this:

	ssize_t xwrite(int fd, const void *buf, size_t len)
        {
        	ssize_t nr;
                static size_t max_write_size = platform_max_write_size();

                if (max_write_size && max_write_size < len)
                	len = max_write_size;
		...
	}

And then we would have in git-compat-util.h something like:

	#define platform_max_write_size() 0

on sane platforms, so that the fix will be optimized away by the compiler.

By the way, does the same version of Cygwin have similar issue on the read
side?

Re: [BUG] git cat-file does not terminate

[Robert Wruck]

> By the way, does the same version of Cygwin have similar issue on the read
> side?

Actually I've written a small test program on the problematic cygwin 
machine that seems to show that the EAGAIN is somehow related to pipes 
(e.g. stdout & stuff) and does not occur when fd refers to a file.
I will test this for read() as well but I don't know enough cygwin 
internals to tell what other versions may be affected.
It might even be related to 32/64 bit or different Windows versions 
since I didn't test more constellations.

-Robert

Re: [BUG] git cat-file does not terminate

[Robert Wruck]

> By the way, does the same version of Cygwin have similar issue on the read
> side?

Here some quick results:

read(fd, buffer, 90000000)
Returns total file size for a file and 65536 (errno=0) for pipes (cat 
file | readtest). When repeating the read, the whole file is read until 
read() returns 0. No problem here on any cygwin I tested.

write(fd, buffer, 90000000)
Returns 90000000 for a file.
Returns 90000000 for redirection (writetest > outfile)
Returns 90000000 for pipes (writetest | cat > outfile) on sane cygwins 
and -1 / EAGAIN on the machine with the original problem.

Re: [BUG] git cat-file does not terminate

[Jeff King]

On Fri, Mar 04, 2011 at 09:16:30AM -0800, Junio C Hamano wrote:

> How prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did
> this bug exist, in other words, if we were to make a table of problematic
> versions, would we have only just a handful entries in it?  Also can we at
> runtime find out what version we are running?
> 
> The reason I am asking these questions is because I think, assuming that
> this would affect many unsuspecting Cygwin users, the best fix would be to
> add a hook in the compat/ layer that decides if MAX_WRITE_SIZE workaround
> is necessary at runtime, and do something like this:
> 
> 	ssize_t xwrite(int fd, const void *buf, size_t len)
>         {
>         	ssize_t nr;
>                 static size_t max_write_size = platform_max_write_size();
> 
>                 if (max_write_size && max_write_size < len)
>                 	len = max_write_size;
> 		...
> 	}

How are we doing the runtime test for platform max write?

If I read the original bug report correctly, the problem was that write
would actually write some bytes _and_ return -1, which is terrible. We
can detect "seems to be returning -1 over and over", but we can't handle
a misbehavior like writing and claiming not to have done so.

So I think the test needs to be "is our version of cygwin in the broken
list" and not "let's try a few different writes and see what works".

But it is still not clear to me how many versions have this bug. I think
the next stop is to show the cygwin developers a clear test-case and see
whether it's already fixed, and which versions show the behavior. They
should be able to get that information much more easily than us. I
really don't want to get involved in bisecting bugs in cygwin (according
to cygwin.com, it's kept in CVS. Blech).

Robert, can you try (or have you already tried) submitting a bug report
to Cygwin?

-Peff

Re: [BUG] git cat-file does not terminate

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 04, 2011 at 09:16:30AM -0800, Junio C Hamano wrote:
>
>> How prevalent is the problematic cygwin1.dll 1.7.8?  Also for how long did
>> this bug exist, in other words, if we were to make a table of problematic
>> versions, would we have only just a handful entries in it?  Also can we at
>> runtime find out what version we are running?
>> 
>> The reason I am asking these questions is because I think, assuming that
>> ...
> How are we doing the runtime test for platform max write?

I asked (1) if we can find out at runtime if we are on which version of
cygwin1.dll, and (2) if we can have a small list of "bad" versions of
cygwin1.dll.  If both are true, the runtime test should be trivial, no?

> So I think the test needs to be "is our version of cygwin in the broken
> list" and not "let's try a few different writes and see what works".

Yes, that is exactly what I had in mind.

Re: [BUG] git cat-file does not terminate

[Robert Wruck]

> I asked (1) if we can find out at runtime if we are on which version of
> cygwin1.dll, and (2) if we can have a small list of "bad" versions of
> cygwin1.dll.  If both are true, the runtime test should be trivial, no?

Currently I don't know of a programmatic way to get the cygwin version 
except `cygcheck -c cygwin` or `uname -r` but these utilities seem to 
know where to find it. I'll take a look at the source.

Unfortunately, the same cygwin version works on most platforms except 
WinXP, so its rather a platform issue and I fear that in this case all 
cygwin versions up to a currently unknown fixed version will be subject.
Depending on the machine, the "limit" at which write() fails seems to 
vary as well. In my initial report, it was about 80MB, on another 
machine it was around 200MB...

I submitted a bug report to cygwin over the weekend and tried to debug 
what's going on in cygwin1.dll but haven't gone very far yet.

-Robert

Re: [BUG] git cat-file does not terminate

[Junio C Hamano]

Robert Wruck <wruck@tweerlei.de> writes:

>> I asked (1) if we can find out at runtime if we are on which version of
>> cygwin1.dll, and (2) if we can have a small list of "bad" versions of
>> cygwin1.dll.  If both are true, the runtime test should be trivial, no?
>
> Currently I don't know of a programmatic way to get the cygwin version
> except `cygcheck -c cygwin` or `uname -r` but these utilities seem to
> know where to find it. I'll take a look at the source.

Thanks; it is not very critical so don't spend too much effort trying to
find a way to do so at runtime.  We can always use the approach Jeff's
Makefile patch took to make it safe (and potentially slow) by default on
all Cygwin while still allowing people on an unaffected version to turn
the workaround off while building.

Re: [BUG] git cat-file does not terminate

[Robert Wruck]

> Thanks; it is not very critical so don't spend too much effort trying to
> find a way to do so at runtime.  We can always use the approach Jeff's
> Makefile patch took to make it safe (and potentially slow) by default on
> all Cygwin while still allowing people on an unaffected version to turn
> the workaround off while building.

The write() issue will presumably be fixed in the next cygwin release. I 
already have a version that works on WinXP where it used to fail.

-Robert

Re: [BUG] git cat-file does not terminate

[Jeff King]

On Wed, Mar 09, 2011 at 08:51:46PM +0100, Robert Wruck wrote:

> >Thanks; it is not very critical so don't spend too much effort trying to
> >find a way to do so at runtime.  We can always use the approach Jeff's
> >Makefile patch took to make it safe (and potentially slow) by default on
> >all Cygwin while still allowing people on an unaffected version to turn
> >the workaround off while building.
> 
> The write() issue will presumably be fixed in the next cygwin
> release. I already have a version that works on WinXP where it used
> to fail.

Cool. Does our Makefile know the cygwin version number via "uname -r"?
We could at least tweak the build-time patch to turn it on for the right
versions (though we should perhaps wait for the fixed version to be
released so we know what it is. :) ).

Personally I wouldn't bother much with run-time detection. But maybe
cygwin people tend to download binary packages and run them on top of
arbitrary versions cygwin? I don't know what's normal.

-Peff