Re: write() _will_ fail on Mac OS X/XNU if nbytes > INT_MAX
- From
Filipe Cabecinhas <filcab@gmail.com>
- Date
- May 9, 2013, 22:58 UTC
- Message-ID
- <CAEDE8504Pa_hRcHBLt4S9CL74noqrX3fWGCSf+x45zCr_43+5A@mail.gmail.com>
- In-Reply-To
- <7v4nffpbct.fsf@alter.siamese.dyndns.org>
Hi Junio,
Sorry for the delay. I've updated the patch to work as you suggested (I think). It's attached.
Thank you,
Filipe F
On Tue, Apr 9, 2013 at 3:50 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 58 quoted lines
> Filipe Cabecinhas <filcab@gmail.com> writes:
>
>>
>> Testing with dd bs=INT_MAX+1 count=1 also gets me an “Invalid
>> argument” error, while bs=INT_MAX will do what's expected.
>>
>> I have a preliminary patch that fixes it, but it may not be the
>> preferred way. The code is not ifdef'ed out and I'm doing the fix in
>> write_in_full(), while it may be preferred to do the fix in xwrite().
>>
>> A radar bug has been submitted to Apple about this, but I think git
>> could tolerate the bug while it isn't fixed, by working around it.
>>
>> Thank you,
>>
>> Filipe
>>
>> diff --git a/wrapper.c b/wrapper.c
>> index bac59d2..474d760 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -187,7 +187,12 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)
>> ssize_t total = 0;
>>
>> while (count > 0) {
>> - ssize_t written = xwrite(fd, p, count);
>> + ssize_t written = 0;
>> + if (count >= INT_MAX)
>> + written = xwrite(fd, p, INT_MAX-1);
>> + else
>> + written = xwrite(fd, p, count);
>
> I think it is better to fix it in much lower level of the stack, as
> other codepaths would call write(2), either thru xwrite() or
> directly.
>
> Ideally the fix should go to the lowest level, i.e. the write(2). I
> do not care if it is done in the kernel or in the libc wrapping
> code; the above does not belong to our code (in an ideal world, that
> is).
>
> Otherwise you would have to patch everything in /usr/bin, no?
>
> But you do not live in an ideal world and neither do we. I think
> the least ugly way may be to add compat/clipped-write.c that
> implements a loop like the above in a helper function:
>
> ssize_t clipped_write(int, const void *, size_t);
>
> and have a
>
> #ifdef NEED_CLIPPED_WRITE
> #define write(x,y,z) clipped_write((x),(y),(z))
> #endif
>
> in git-compat-util.h, or something. Makefile needs to get adjusted
> to link with compat/clipped-write.o when NEED_CLIPPED_WRITE is
> defined as well.diff --git a/Makefile b/Makefile index 0f931a2..e6aff85 100644 --- a/Makefile +++ b/Makefile @@ -1466,6 +1466,11 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS MSGFMT += --check --statistics endif +ifdef NEEDS_CLIPPED_WRITE + BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE + COMPAT_OBJS += compat/write.o +endif + ifneq (,$(XDL_FAST_HASH)) BASIC_CFLAGS += -DXDL_FAST_HASH endif diff --git a/compat/write.c b/compat/write.c new file mode 100644 index 0000000..1e890aa --- /dev/null +++ b/compat/write.c @@ -0,0 +1,11 @@ +#include <limits.h> +#include <unistd.h> + +/* Version of write that will write at most INT_MAX bytes. + * Workaround a xnu bug on Mac OS X */ +ssize_t clipped_write(int fildes, const void *buf, size_t nbyte) { + if (nbyte > INT_MAX) + return write(fildes, buf, INT_MAX); + else + return write(fildes, buf, nbyte); +} diff --git a/config.mak.uname b/config.mak.uname index d78fd3d..174703b 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_WRITE = YesPlease COMPAT_OBJS += compat/precompose_utf8.o BASIC_CFLAGS += -DPRECOMPOSE_UNICODE endif diff --git a/git-compat-util.h b/git-compat-util.h index e955bb5..a96db23 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -185,6 +185,11 @@ int get_st_mode_bits(const char *path, int *mode); #define probe_utf8_pathname_composition(a,b) #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)) +#endif + #ifdef MKDIR_WO_TRAILING_SLASH #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);