[PATCH v3] compat: Fix read() of 2GB and more on Mac OS X

2013-08-19 Thread Steffen Prohaska
Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

error: read from external filter cat failed
error: cannot feed the input to external filter cat
error: cat died of signal 13
error: external filter cat failed 141
error: external filter cat failed

The reason was that read() immediately returns with EINVAL if nbyte =
2GB.  According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined.  The write
function has the same restriction [2].  Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions.  For write, the problem has been addressed in a earlier
commit [6c642a].

The problem for read() is addressed in a similar way by introducing
a wrapper function in compat that always reads less than 2GB.
Unfortunately, '#undef read' is needed at a few places to avoid
expanding the compat macro in constructs like 'vtbl-read(...)'.

Note that 'git add' exits with 0 even if it prints filtering errors to
stderr.  The test, therefore, checks stderr.  'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails.  The test could then be changed to use test_must_fail.

Thanks to the following people for their suggestions:

Johannes Sixt j...@kdbg.org
John Keeping j...@keeping.me.uk
Jonathan Nieder jrnie...@gmail.com
Kyle J. McKay mack...@gmail.com
Torsten Bögershausen tbo...@web.de

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

[6c642a] 6c642a878688adf46b226903858b53e2d31ac5c3
compate/clipped-write.c: large write(2) fails on Mac OS X/XNU

Signed-off-by: Steffen Prohaska proha...@zib.de
---
 Makefile  |  8 
 builtin/var.c |  1 +
 compat/clipped-read.c | 13 +
 config.mak.uname  |  1 +
 git-compat-util.h |  5 +
 streaming.c   |  1 +
 t/t0021-conversion.sh | 14 ++
 7 files changed, 43 insertions(+)
 create mode 100644 compat/clipped-read.c

diff --git a/Makefile b/Makefile
index 3588ca1..0f69e24 100644
--- a/Makefile
+++ b/Makefile
@@ -69,6 +69,9 @@ all::
 # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
 # doesn't support GNU extensions like --check and --statistics
 #
+# Define NEEDS_CLIPPED_READ if your read(2) cannot read more than
+# INT_MAX bytes at once (e.g. MacOS X).
+#
 # Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
 # INT_MAX bytes at once (e.g. MacOS X).
 #
@@ -1493,6 +1496,11 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
MSGFMT += --check --statistics
 endif
 
+ifdef NEEDS_CLIPPED_READ
+   BASIC_CFLAGS += -DNEEDS_CLIPPED_READ
+   COMPAT_OBJS += compat/clipped-read.o
+endif
+
 ifdef NEEDS_CLIPPED_WRITE
BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
COMPAT_OBJS += compat/clipped-write.o
diff --git a/builtin/var.c b/builtin/var.c
index aedbb53..e59f5ba 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -38,6 +38,7 @@ static struct git_var git_vars[] = {
{ , NULL },
 };
 
+#undef read
 static void list_vars(void)
 {
struct git_var *ptr;
diff --git a/compat/clipped-read.c b/compat/clipped-read.c
new file mode 100644
index 000..6962f67
--- /dev/null
+++ b/compat/clipped-read.c
@@ -0,0 +1,13 @@
+#include ../git-compat-util.h
+#undef read
+
+/*
+ * Version of read that will write at most INT_MAX bytes.
+ * Workaround a xnu bug on Mac OS X
+ */
+ssize_t clipped_read(int fd, void *buf, size_t nbyte)
+{
+   if (nbyte  INT_MAX)
+   nbyte = INT_MAX;
+   return read(fd, buf, nbyte);
+}
diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..5c10726 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -95,6 +95,7 @@ ifeq ($(uname_S),Darwin)
NO_MEMMEM = YesPlease
USE_ST_TIMESPEC = YesPlease
HAVE_DEV_TTY = YesPlease
+   NEEDS_CLIPPED_READ = YesPlease
NEEDS_CLIPPED_WRITE = YesPlease
COMPAT_OBJS += compat/precompose_utf8.o
BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..a227127 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -185,6 +185,11 @@ typedef unsigned long uintptr_t;
 #define probe_utf8_pathname_composition(a,b)
 #endif
 
+#ifdef NEEDS_CLIPPED_READ
+ssize_t clipped_read(int fd, void *buf, size_t nbyte);
+#define read(x,y,z) clipped_read((x),(y),(z))
+#endif
+
 #ifdef NEEDS_CLIPPED_WRITE
 ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
 #define write(x,y,z) clipped_write((x),(y),(z))
diff --git a/streaming.c b/streaming.c
index debe904..c1fe34a 100644
--- a/streaming.c
+++ b/streaming.c
@@ -99,6 +99,7 @@ int close_istream(struct git_istream *st)
return r;
 }
 

Re: [PATCH v3] compat: Fix read() of 2GB and more on Mac OS X

2013-08-19 Thread Eric Sunshine
On Mon, Aug 19, 2013 at 4:21 AM, Steffen Prohaska proha...@zib.de wrote:
 Previously, filtering 2GB or more through an external filter (see test)
 failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:

 error: read from external filter cat failed
 error: cannot feed the input to external filter cat
 error: cat died of signal 13
 error: external filter cat failed 141
 error: external filter cat failed


 Signed-off-by: Steffen Prohaska proha...@zib.de
 ---
  Makefile  |  8 
  builtin/var.c |  1 +
  compat/clipped-read.c | 13 +
  config.mak.uname  |  1 +
  git-compat-util.h |  5 +
  streaming.c   |  1 +
  t/t0021-conversion.sh | 14 ++
  7 files changed, 43 insertions(+)
  create mode 100644 compat/clipped-read.c

 diff --git a/Makefile b/Makefile
 index 3588ca1..0f69e24 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -69,6 +69,9 @@ all::
  # Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
  # doesn't support GNU extensions like --check and --statistics
  #
 +# Define NEEDS_CLIPPED_READ if your read(2) cannot read more than
 +# INT_MAX bytes at once (e.g. MacOS X).
 +#
  # Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
  # INT_MAX bytes at once (e.g. MacOS X).

Is it likely that we would see a platform requiring only one or the
other CLIPPED? Would it make sense to combine these into a single
NEEDS_CLIPPED_IO?

  #
 @@ -1493,6 +1496,11 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
 MSGFMT += --check --statistics
  endif

 +ifdef NEEDS_CLIPPED_READ
 +   BASIC_CFLAGS += -DNEEDS_CLIPPED_READ
 +   COMPAT_OBJS += compat/clipped-read.o
 +endif
 +
  ifdef NEEDS_CLIPPED_WRITE
 BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
 COMPAT_OBJS += compat/clipped-write.o
 diff --git a/builtin/var.c b/builtin/var.c
 index aedbb53..e59f5ba 100644
 --- a/builtin/var.c
 +++ b/builtin/var.c
 @@ -38,6 +38,7 @@ static struct git_var git_vars[] = {
 { , NULL },
  };
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] compat: Fix read() of 2GB and more on Mac OS X

2013-08-19 Thread Junio C Hamano
Eric Sunshine sunsh...@sunshineco.com writes:

 +# Define NEEDS_CLIPPED_READ if your read(2) cannot read more than
 +# INT_MAX bytes at once (e.g. MacOS X).
 +#
  # Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
  # INT_MAX bytes at once (e.g. MacOS X).

 Is it likely that we would see a platform requiring only one or the
 other CLIPPED? Would it make sense to combine these into a single
 NEEDS_CLIPPED_IO?

I am slightly negative to that suggestion for two reasons.

 - Does MacOS X clip other IO operations?  Do we need to invent yet
   another NEEDS_CLIPPED, e.g. NEEDS_CLIPPED_LSEEK?

   A single NEEDS_CLIPPED_IO may sound attractive for its simplicity
   (e.g. on a system that only needs NEEDS_CLIPPED_WRITE, we will
   unnecessarily chop a big read into multiple reads, but that does
   not affect the correctness of the operation, only performance but
   the actual IO cost will dominate it anyway).  If we know there
   are 47 different IO operations that might need clipping, that
   simplicity is certainly a good thing to have.  I somehow do not
   think the set of operations will grow that large, though.

 - NEEDS_CLIPPED_IO essentially says only those who clip their
   writes would clip their reads (and vice versa), which is not all
   that different from saying only Apple would clip their IO,
   which in turn defeats the notion of let's use a generic
   NEEDS_CLIPPED without limiting the workaround to specific
   platforms somewhat.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html