Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> In a subsequent commit we're going to add the first caller to
> writev(3p). Introduce a compatibility wrapper for this syscall that we
> can use on systems that don't have this syscall.
>
> The syscall exists on modern Unixes like Linux and macOS, and seemingly
> even for NonStop according to [1]. It doesn't seem to exist on Windows
> though.



>
> [1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf
> [2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Makefile         |  4 ++++
>  compat/posix.h   | 14 ++++++++++++++
>  compat/writev.c  | 29 +++++++++++++++++++++++++++++
>  config.mak.uname |  2 ++
>  meson.build      |  1 +
>  5 files changed, 50 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index 7f37ad8f58..cb95ff2daf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2021,6 +2021,10 @@ ifdef NO_PREAD
>  	COMPAT_CFLAGS += -DNO_PREAD
>  	COMPAT_OBJS += compat/pread.o
>  endif
> +ifdef NO_WRITEV
> +	COMPAT_CFLAGS += -DNO_WRITEV
> +	COMPAT_OBJS += compat/writev.o
> +endif
>  ifdef NO_FAST_WORKING_DIRECTORY
>  	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
>  endif
> diff --git a/compat/posix.h b/compat/posix.h
> index 245386fa4a..3c611d2736 100644
> --- a/compat/posix.h
> +++ b/compat/posix.h
> @@ -137,6 +137,9 @@
>  #include <sys/socket.h>
>  #include <sys/ioctl.h>
>  #include <sys/statvfs.h>
> +#ifndef NO_WRITEV
> +#include <sys/uio.h>
> +#endif
>  #include <termios.h>
>  #ifndef NO_SYS_SELECT_H
>  #include <sys/select.h>
> @@ -323,6 +326,17 @@ int git_lstat(const char *, struct stat *);
>  ssize_t git_pread(int fd, void *buf, size_t count, off_t offset);
>  #endif
>  
> +#ifdef NO_WRITEV
> +#define writev git_writev
> +#define iovec git_iovec
> +struct git_iovec {
> +	void *iov_base;
> +	size_t iov_len;
> +};
> +
> +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt);
> +#endif
> +
>  #ifdef NO_SETENV
>  #define setenv gitsetenv
>  int gitsetenv(const char *, const char *, int);
> diff --git a/compat/writev.c b/compat/writev.c
> new file mode 100644
> index 0000000000..b77e534d5d
> --- /dev/null
> +++ b/compat/writev.c
> @@ -0,0 +1,29 @@
> +#include "../git-compat-util.h"
> +#include "../wrapper.h"
> +
> +ssize_t git_writev(int fd, const struct iovec *iov, int iovcnt)
> +{
> +	size_t total_written = 0;
> +
> +	for (int i = 0; i < iovcnt; i++) {
> +		const char *bytes = iov[i].iov_base;
> +		size_t iovec_written = 0;
> +
> +		while (iovec_written < iov[i].iov_len) {
> +			ssize_t bytes_written = xwrite(fd, bytes + iovec_written,
> +						       iov[i].iov_len - iovec_written);
> +			if (bytes_written < 0) {
> +				if (total_written)
> +					goto out;
> +				return bytes_written;
> +			}
> +			if (!bytes_written)
> +				goto out;
> +			iovec_written += bytes_written;
> +			total_written += bytes_written;
> +		}
> +	}
> +
> +out:
> +	return cast_size_t_to_ssize_t(total_written);
> +}

Because we do not check the accumulation of bytes_written in the two
accumulator variables inside the inner loop, it is very possible for
total_written to wraparound size_t and end up below the largest
value possible to be stored in ssize_t type.

IOW, the cast_size_t_to_ssize_t() introduced in the previous step is
pointless, isn't it?

According to [*1*], the real

    ssize_t writev(int fd, const struct iovec *iov, int iovcnt)

is supposed to report error with errno set to EINVAL when the sum of
iov_len member of the iov[] array elements exceed half of the
maximum size_t.

       EINVAL The sum of the iov_len values overflows an ssize_t value.

So instead of dying with cast_size_t_to_ssize_t(), we probably would
want the check done in a more stupid and straight-forward way?
Adding up iov[i].iov_len while the addition would not wraparound in
each and every step, and return error with EINVAL before attempting
even a single call to xwrite(), and then have the above double loop
that does not care about integer wraparound at all, and return
total_written with simple cast to (ssize_t)?


[Reference]

 *1* https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html


> diff --git a/config.mak.uname b/config.mak.uname
> index 5feb582558..ccb3f71881 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -459,6 +459,7 @@ ifeq ($(uname_S),Windows)
>  	SANE_TOOL_PATH ?= $(msvc_bin_dir_msys)
>  	HAVE_ALLOCA_H = YesPlease
>  	NO_PREAD = YesPlease
> +	NO_WRITEV = YesPlease
>  	NEEDS_CRYPTO_WITH_SSL = YesPlease
>  	NO_LIBGEN_H = YesPlease
>  	NO_POLL = YesPlease
> @@ -674,6 +675,7 @@ ifeq ($(uname_S),MINGW)
>  	pathsep = ;
>  	HAVE_ALLOCA_H = YesPlease
>  	NO_PREAD = YesPlease
> +	NO_WRITEV = YesPlease
>  	NEEDS_CRYPTO_WITH_SSL = YesPlease
>  	NO_LIBGEN_H = YesPlease
>  	NO_POLL = YesPlease
> diff --git a/meson.build b/meson.build
> index 762e2d0fc0..63514b6b84 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1409,6 +1409,7 @@ checkfuncs = {
>    'initgroups' : [],
>    'strtoumax' : ['strtoumax.c', 'strtoimax.c'],
>    'pread' : ['pread.c'],
> +  'writev' : ['writev.c'],
>  }
>  
>  if host_machine.system() == 'windows'