Re: [PATCH v2 05/10] compat/posix: introduce writev(3p) wrapper
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2026, 22:01 UTC
- Message-ID
- <xmqqseaf5k5t.fsf@gitster.g>
- In-Reply-To
- <20260303-pks-upload-pack-write-contention-v2-5-7321830f08fe@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
Show 7 quoted lines
> 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.
Show 95 quoted lines
>
> [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
Show 32 quoted lines
> 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'