{"thread":{"id":"65462","subject":"[PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","startedAt":"2026-04-09T12:48:09Z","lastAt":"2026-04-10T05:20:07Z","messageCount":8,"participants":["Patrick Steinhardt","Ben Knoble","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"541258","messageId":"20260409-b4-pks-writev-max-io-size-v1-1-81730e8f35df@pks.im","threadId":"65462","inReplyTo":null,"subject":"[PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-09T12:47:59Z","receivedAt":"2026-04-09T12:48:09Z","isPatch":true,"body":"Some systems like NonStop set a comparatively small `MAX_IO_SIZE`, which\nlimits the maximum number of bytes we're allowed to write in a single\ncall. We already handle this limit properly in `xwrite()`, but we have\nrecently introduced wrappers for writev(3p) where we don't. This will\ncause the syscall to return EINVAL in case somebody passes an iovec\nentry to writev(3p) that is larger than `MAX_IO_SIZE`.\n\nIntroduce a new function `xwritev()` that is similar to `xwrite()` in\nthat it handles such platform-specific nuances. The logic is rather\nsimple: we simply coalesce all iovecs that don't exceed `MAX_IO_SIZE`\nand pass those to writev(3p). If the first iovec already exceeds the\nlimit, we'll instead pass it to `xwrite()`, which handles the limit for\nus.\n\nAdapt `writev_in_full()` to use this new wrapper. As this wrapper\nalready knows to to call writev(3p) in a loop already it doesn't need\nany further adjustment.\n\nReported-by: Randall Becker <randall.becker@nexbridge.ca>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\nHi,\n\nthis fixes the issue reported by Randall in [1].\n\nI mostly wanted to get this patch out there so that we can discuss a\nproposed fix, but as said in the thread I'm also happy to revise course\nand instead set NO_WRITEV on NonStop for now. I think we'll want to\neventually land a fix like the one proposed here though, and at that\npoint the workaround would not be required anymore.\n\nThanks!\n\nPatrick\n\n[1]: <00f401dcc6e6$7183c0f0$548b42d0$@nexbridge.com>\n---\n wrapper.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------\n wrapper.h |  1 +\n 2 files changed, 46 insertions(+), 6 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex be8fa575e6..d989c78b4b 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -323,21 +323,60 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n \treturn total;\n }\n \n+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)\n+{\n+\tssize_t bytes_written;\n+\tsize_t total_length;\n+\tint i;\n+\n+\t/*\n+\t * We need to make sure that writev(3p) call does not write more than\n+\t * `MAX_IO_SIZE` many bytes. If we do exceed that limit, we only pass\n+\t * those iovecs to writev(3p) that sum up to less than the limit.\n+\t *\n+\t * If on the other hand the first iovec entry already exceeds this\n+\t * limit we'll instead use xwrite() to write it, which knows to handle\n+\t * `MAX_IO_SIZE` for us.\n+\t */\n+\tfor (i = 0, total_length = 0; i < iovcnt; i++) {\n+\t\tif (unsigned_add_overflows(total_length, iov[i].iov_len))\n+\t\t\tbreak;\n+\n+\t\ttotal_length += iov[i].iov_len;\n+\t\tif (total_length > MAX_IO_SIZE)\n+\t\t\tbreak;\n+\t}\n+\n+\tif (i < iovcnt) {\n+\t\t/*\n+\t\t * The first entry exceeds MAX_IO_SIZE, so we pass it to\n+\t\t * xwrite, which knows to handle this case.\n+\t\t */\n+\t\tif (!i)\n+\t\t\treturn xwrite(fd, iov->iov_base, iov->iov_len);\n+\t\tiovcnt = i;\n+\t}\n+\n+\tbytes_written = writev(fd, iov, iovcnt);\n+\tif (!bytes_written) {\n+\t\terrno = ENOSPC;\n+\t\treturn -1;\n+\t}\n+\n+\treturn bytes_written;\n+}\n+\n ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)\n {\n \tssize_t total_written = 0;\n \n \twhile (iovcnt) {\n-\t\tssize_t bytes_written = writev(fd, iov, iovcnt);\n-\t\tif (bytes_written < 0) {\n+\t\tssize_t bytes_written = xwritev(fd, iov, iovcnt);\n+\t\tif (bytes_written <= 0) {\n \t\t\tif (errno == EINTR || errno == EAGAIN)\n \t\t\t\tcontinue;\n \t\t\treturn -1;\n \t\t}\n-\t\tif (!bytes_written) {\n-\t\t\terrno = ENOSPC;\n-\t\t\treturn -1;\n-\t\t}\n \n \t\ttotal_written += bytes_written;\n \ndiff --git a/wrapper.h b/wrapper.h\nindex 27519b32d1..a6287d7f4d 100644\n--- a/wrapper.h\n+++ b/wrapper.h\n@@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_\n int xopen(const char *path, int flags, ...);\n ssize_t xread(int fd, void *buf, size_t len);\n ssize_t xwrite(int fd, const void *buf, size_t len);\n+ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);\n ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n int xdup(int fd);\n FILE *xfopen(const char *path, const char *mode);\n\n---\nbase-commit: b15384c06f77bc2d34d0d3623a8a58218313a561\nchange-id: 20260409-b4-pks-writev-max-io-size-e9b803439ae8\n\n"},{"id":"541275","messageId":"98E6F739-4ECA-44A4-8645-0B153C969E36@gmail.com","threadId":"65462","inReplyTo":"20260409-b4-pks-writev-max-io-size-v1-1-81730e8f35df@pks.im","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-04-09T15:46:24Z","receivedAt":"2026-04-09T15:46:38Z","isPatch":true,"body":"Admitting I am out of my depth here…\n\n> Le 9 avr. 2026 à 08:52, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> Some systems like NonStop set a comparatively small `MAX_IO_SIZE`, which\n> limits the maximum number of bytes we're allowed to write in a single\n> call. We already handle this limit properly in `xwrite()`, but we have\n> recently introduced wrappers for writev(3p) where we don't. This will\n> cause the syscall to return EINVAL in case somebody passes an iovec\n> entry to writev(3p) that is larger than `MAX_IO_SIZE`.\n> \n> Introduce a new function `xwritev()` that is similar to `xwrite()` in\n> that it handles such platform-specific nuances. The logic is rather\n> simple: we simply coalesce all iovecs that don't exceed `MAX_IO_SIZE`\n> and pass those to writev(3p). If the first iovec already exceeds the\n> limit, we'll instead pass it to `xwrite()`, which handles the limit for\n> us.\n> \n> Adapt `writev_in_full()` to use this new wrapper. As this wrapper\n> already knows to to call writev(3p) in a loop already it doesn't need\n> any further adjustment.\n> \n> Reported-by: Randall Becker <randall.becker@nexbridge.ca>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n> Hi,\n> \n> this fixes the issue reported by Randall in [1].\n> \n> I mostly wanted to get this patch out there so that we can discuss a\n> proposed fix, but as said in the thread I'm also happy to revise course\n> and instead set NO_WRITEV on NonStop for now. I think we'll want to\n> eventually land a fix like the one proposed here though, and at that\n> point the workaround would not be required anymore.\n> \n> Thanks!\n> \n> Patrick\n> \n> [1]: <00f401dcc6e6$7183c0f0$548b42d0$@nexbridge.com>\n> ---\n> wrapper.c | 51 +++++++++++++++++++++++++++++++++++++++++++++------\n> wrapper.h |  1 +\n> 2 files changed, 46 insertions(+), 6 deletions(-)\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index be8fa575e6..d989c78b4b 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -323,21 +323,60 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n>    return total;\n> }\n> \n> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)\n> +{\n> +    ssize_t bytes_written;\n> +    size_t total_length;\n> +    int i;\n> +\n> +    /*\n> +     * We need to make sure that writev(3p) call does not write more than\n> +     * `MAX_IO_SIZE` many bytes. If we do exceed that limit, we only pass\n> +     * those iovecs to writev(3p) that sum up to less than the limit.\n> +     *\n> +     * If on the other hand the first iovec entry already exceeds this\n> +     * limit we'll instead use xwrite() to write it, which knows to handle\n> +     * `MAX_IO_SIZE` for us.\n> +     */\n> +    for (i = 0, total_length = 0; i < iovcnt; i++) {\n> +        if (unsigned_add_overflows(total_length, iov[i].iov_len))\n> +            break;\n> +\n> +        total_length += iov[i].iov_len;\n> +        if (total_length > MAX_IO_SIZE)\n> +            break;\n> +    }\n> +\n> +    if (i < iovcnt) {\n> +        /*\n> +         * The first entry exceeds MAX_IO_SIZE, so we pass it to\n> +         * xwrite, which knows to handle this case.\n> +         */\n> +        if (!i)\n> +            return xwrite(fd, iov->iov_base, iov->iov_len);\n\nIt took me starting to write this email wondering “but i could be >= 1?” to realize that this comment applies to the !i case below. Darn.\n\nStill, I find the declaration (“the first entry exceeds”) before the check a bit confusing. Is that typical of our style (in which case leave it be)?\n\n> +        iovcnt = i;\n> +    }\n> +\n> +    bytes_written = writev(fd, iov, iovcnt);\n> +    if (!bytes_written) {\n> +        errno = ENOSPC;\n> +        return -1;\n> +    }\n> +\n> +    return bytes_written;\n> +}\n> +\n> ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)\n> {\n>    ssize_t total_written = 0;\n> \n>    while (iovcnt) {\n> -        ssize_t bytes_written = writev(fd, iov, iovcnt);\n> -        if (bytes_written < 0) {\n> +        ssize_t bytes_written = xwritev(fd, iov, iovcnt);\n> +        if (bytes_written <= 0) {\n>            if (errno == EINTR || errno == EAGAIN)\n>                continue;\n>            return -1;\n>        }\n> -        if (!bytes_written) {\n> -            errno = ENOSPC;\n> -            return -1;\n> -        }\n> \n>        total_written += bytes_written;\n> \n> diff --git a/wrapper.h b/wrapper.h\n> index 27519b32d1..a6287d7f4d 100644\n> --- a/wrapper.h\n> +++ b/wrapper.h\n> @@ -16,6 +16,7 @@ void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_\n> int xopen(const char *path, int flags, ...);\n> ssize_t xread(int fd, void *buf, size_t len);\n> ssize_t xwrite(int fd, const void *buf, size_t len);\n> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt);\n> ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n> int xdup(int fd);\n> FILE *xfopen(const char *path, const char *mode);\n> \n> ---\n> base-commit: b15384c06f77bc2d34d0d3623a8a58218313a561\n> change-id: 20260409-b4-pks-writev-max-io-size-e9b803439ae8\n> \n> \n"},{"id":"541277","messageId":"xmqqika0ultp.fsf@gitster.g","threadId":"65462","inReplyTo":"20260409-b4-pks-writev-max-io-size-v1-1-81730e8f35df@pks.im","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-09T16:42:42Z","receivedAt":"2026-04-09T16:42:45Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Some systems like NonStop set a comparatively small `MAX_IO_SIZE`, which\n> limits the maximum number of bytes we're allowed to write in a single\n> call. We already handle this limit properly in `xwrite()`, but we have\n> recently introduced wrappers for writev(3p) where we don't. This will\n> cause the syscall to return EINVAL in case somebody passes an iovec\n> entry to writev(3p) that is larger than `MAX_IO_SIZE`.\n>\n> Introduce a new function `xwritev()` that is similar to `xwrite()` in\n> that it handles such platform-specific nuances. The logic is rather\n> simple: we simply coalesce all iovecs that don't exceed `MAX_IO_SIZE`\n> and pass those to writev(3p). If the first iovec already exceeds the\n> limit, we'll instead pass it to `xwrite()`, which handles the limit for\n> us.\n\nOK, so the idea is just like xwrite(), whose original purpose was to\nhide the details of having to restart write() system call, pretends\na short write on IO limited platforms, xwritev() pretends that the\nunderlying writev() gave a short write, instead of a failure with\nEINVAL when ssize_t is unusually small.  That mirrors an established\npattern that is proven to work with write_in_full() code path, which\nis a good thing.\n\nBy the way, I see that EINTR and EWOULDBLOCK are handled somewhat\ndifferently from how xwrite() handles it.  As the original change\nthat introduced use of writev() were to replace write_in_full() that\neventually called into xwrite(), shouldn't we be closer to what\nxwrite() used to do?\n\n> this fixes the issue reported by Randall in [1].\n>\n> I mostly wanted to get this patch out there so that we can discuss a\n> proposed fix, but as said in the thread I'm also happy to revise course\n> and instead set NO_WRITEV on NonStop for now. I think we'll want to\n> eventually land a fix like the one proposed here though, and at that\n> point the workaround would not be required anymore.\n\nIt is too late to make this kind of \"let's wrap writev()\" effort\nbefore the final, and Git 2.54 should ship without any such change.\nIf your platform does not have a writev() that works with write size\nup to half of maximum of size_t (or at least 64kB), use NO_WRITEV to\nbuild your Git.\n\nBut let's discuss to prepare for an update post release.\n\nIt would be nice to see minority platforms including NonStop test\nand notice problems that appear only on their system much earlier in\nthe cycle next time.  A report at -rc0, while better than not seeing\nany, is a bit too late.\n\n> diff --git a/wrapper.c b/wrapper.c\n> index be8fa575e6..d989c78b4b 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -323,21 +323,60 @@ ssize_t write_in_full(int fd, const void *buf, size_t count)\n>  \treturn total;\n>  }\n>  \n> +ssize_t xwritev(int fd, struct iovec *iov, int iovcnt)\n> +{\n> +\tssize_t bytes_written;\n> +\tsize_t total_length;\n> +\tint i;\n> +\n> +\t/*\n> +\t * We need to make sure that writev(3p) call does not write more than\n> +\t * `MAX_IO_SIZE` many bytes. If we do exceed that limit, we only pass\n> +\t * those iovecs to writev(3p) that sum up to less than the limit.\n> +\t *\n> +\t * If on the other hand the first iovec entry already exceeds this\n> +\t * limit we'll instead use xwrite() to write it, which knows to handle\n> +\t * `MAX_IO_SIZE` for us.\n> +\t */\n> +\tfor (i = 0, total_length = 0; i < iovcnt; i++) {\n> +\t\tif (unsigned_add_overflows(total_length, iov[i].iov_len))\n> +\t\t\tbreak;\n\nWe add .iov_len up in this first loop, because we do not want to\nbust writev(3p)'s limit.\n\n\tEINVAL The sum of the iov_len values in the iov array would\n              overflow an ssize_t.\n\ncf. https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html\n\nAs the width of ssize_t in bits can be a lot smaller than size_t,\nthe above \"unsigned_add_overflows() triggers way too late for the\ncheck to matter, no?\n\n> +\t\ttotal_length += iov[i].iov_len;\n\nThen we have total_length computed.\n\n> +\t\tif (total_length > MAX_IO_SIZE)\n> +\t\t\tbreak;\n\nAnd it is capped to MAX_IO_SIZE, which is set way lower than\nSSIZE_MAX even on mainstream platforms (8MB, IIRC).  This does not\nmatter only because we are currently using writev() only for\nsideband communication and its payload is limited to 64kB, but if a\ncaller gave us a list of buffers whose total size ranges in a few\nhundred megabytes (e.g., packfiles to clone a small project like\ngit.git itself), on a not-so-I/O-limited platform, do we still want\nto chomp the original request into multiple writev(3p) system calls?\n\nI personally think it is an OK thing to do, simply because we also\nchomp a large xwrite() request into chunks and make multiple\nwrite(2) system calls, but we should explain the reason behind \"We\nneed to make sure\" in the beginning of the above comment well--the\ncurrent text has no explanation on the reason.\n\n\n> +\t}\n> +\n> +\tif (i < iovcnt) {\n> +\t\t/*\n> +\t\t * The first entry exceeds MAX_IO_SIZE, so we pass it to\n> +\t\t * xwrite, which knows to handle this case.\n> +\t\t */\n> +\t\tif (!i)\n> +\t\t\treturn xwrite(fd, iov->iov_base, iov->iov_len);\n\nBen has a similar comment, but it would be easier to see the\ncorrespondence if you rephrase the comment perhaps like\n\n\t\t/*\n                 * If the first buffer is larger than MAX_IO_SIZE,\n                 * let xwrite() deal with it.\n                 */\n\nxwrite() can return a short write, but the caller is counting how\nmany bytes among what it passed to xwritev() are consumed by each\ncall to this function, so the next call we may be seeing may have\niov->iov_base pointing into the buffer we threw at xwrite() with\nreduced iov->iov_len, and that is expected and everything will even\nout.  Quite nice.\n\n> +\t\tiovcnt = i;\n\nAnd if our iov[0].iov_len is smaller than MAX_IO_SIZE, then i would\nbe at least 1 here and shows the index in iov[] array that busts the\nlimit.  IOW, we know iov[0]..iov[i-1] (inclusive) can be written\nwithout busting MAX_IO_SIZE in one go.  So se reduce iovcnt down to\nthat number here, in preparation for the next call.\n\n> +\t}\n\nBy the way, if the total of iov[] is reaonably small, then the\ninitial loop runs to the end of it, the above if() statement will be\nskipped, and iovcnt is not reduced.\n\nEither way, we now pass the initial part (which may be the entirety)\nof iov[] up to iovcnt to writev().\n\n> +\tbytes_written = writev(fd, iov, iovcnt);\n> +\tif (!bytes_written) {\n> +\t\terrno = ENOSPC;\n> +\t\treturn -1;\n> +\t}\n> +\n> +\treturn bytes_written;\n> +}\n\nOK.  So except for \"is unsigned_add_overflows() doing what we want?\"\nquestion, I think the above is reasonable.\n\nIf we used \"let's make sure sum of iov[].iov_len does not overflow\nan ssize_t\" at the beginning of the loop, it does change the\ncontract between the callers and this function, as they can no\nlonger get EINVAL due to such overflow (instead this function will\nchomp the request into smaller pieces, just like MAX_IO_SIZE is used\nhere).  And I think that is a reasonable semantics.\n\n>  ssize_t writev_in_full(int fd, struct iovec *iov, int iovcnt)\n>  {\n>  \tssize_t total_written = 0;\n>  \n>  \twhile (iovcnt) {\n> -\t\tssize_t bytes_written = writev(fd, iov, iovcnt);\n> -\t\tif (bytes_written < 0) {\n> +\t\tssize_t bytes_written = xwritev(fd, iov, iovcnt);\n> +\t\tif (bytes_written <= 0) {\n>  \t\t\tif (errno == EINTR || errno == EAGAIN)\n>  \t\t\t\tcontinue;\n\nI am not sure if this is how we want to handle EINTR, given\nespecially that xwritev() may have called xwrite() under the hood in\nsome case but not in others.  If we are doing anything to these\nsignals, I think it should be done where we call underlying writev(),\nand we should be close to whatever is done in xwrite() where it\ncalls write().\n\n>  \t\t\treturn -1;\n>  \t\t}\n> -\t\tif (!bytes_written) {\n> -\t\t\terrno = ENOSPC;\n> -\t\t\treturn -1;\n> -\t\t}\n\nOn the other hand, I think moving this into xwritev() is a mistake.\nWe shoudl try to be as close to what these functions are replacing\n(i.e. write_in_full and xwrite combo) in the code paths that are\nrewritten to use them.\n"},{"id":"541298","messageId":"20260409202329.GA3076846@coredump.intra.peff.net","threadId":"65462","inReplyTo":"xmqqika0ultp.fsf@gitster.g","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-09T20:23:29Z","receivedAt":"2026-04-09T20:23:31Z","isPatch":true,"body":"On Thu, Apr 09, 2026 at 09:42:42AM -0700, Junio C Hamano wrote:\n\n> > +\tfor (i = 0, total_length = 0; i < iovcnt; i++) {\n> > +\t\tif (unsigned_add_overflows(total_length, iov[i].iov_len))\n> > +\t\t\tbreak;\n> \n> We add .iov_len up in this first loop, because we do not want to\n> bust writev(3p)'s limit.\n> \n> \tEINVAL The sum of the iov_len values in the iov array would\n>               overflow an ssize_t.\n> \n> cf. https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html\n> \n> As the width of ssize_t in bits can be a lot smaller than size_t,\n> the above \"unsigned_add_overflows() triggers way too late for the\n> check to matter, no?\n\nI think it is correct as-is.\n\nThe real check against ssize_t is later, when we compare total_length to\nMAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure\nwe do not overflow size_t when counting up the total (and if we do, we\n_know_ we are going to overflow ssize_t, which must be smaller).\n\nI think this can be made more clear by counting down allowable bytes\ninstead of up. I'll show an example in a second.\n\n> > +\t}\n> > +\n> > +\tif (i < iovcnt) {\n> > +\t\t/*\n> > +\t\t * The first entry exceeds MAX_IO_SIZE, so we pass it to\n> > +\t\t * xwrite, which knows to handle this case.\n> > +\t\t */\n> > +\t\tif (!i)\n> > +\t\t\treturn xwrite(fd, iov->iov_base, iov->iov_len);\n> \n> Ben has a similar comment, but it would be easier to see the\n> correspondence if you rephrase the comment perhaps like\n> [...]\n\nMe three. I think this can be made more clear if we bail to xwrite()\nimmediately in the loop. So together with the count-down, something\nlike:\n\n   ssize_t allowed = MAX_IO_SIZE;\n   int i;\n\n   for (i = 0; i < iovcnt; i++) {\n\tif (iov[i].iov_len > allowed) {\n\t\tif (!i)\n\t\t\treturn xwrite(fd, iov->iov_base, iov_len);\n\t\tbreak;\n\t}\n\tallowed -= iov[i].iov_len;\n  }\n  return writev(fd, iov, i);\n\nYou can also directly return writev() instead of breaking out of the\nloop. That makes it even more clear that we are doing a partial write,\nbut means duplicating the \"return writev()\" line.\n\nAnd I think the whole thing would still deserve comments, but I omitted\nthem here since the point was to show the rearranged structure.\n\n-Peff\n"},{"id":"541303","messageId":"xmqq5x5zsw8r.fsf@gitster.g","threadId":"65462","inReplyTo":"20260409202329.GA3076846@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-09T20:40:36Z","receivedAt":"2026-04-09T20:40:40Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Apr 09, 2026 at 09:42:42AM -0700, Junio C Hamano wrote:\n>\n>> > +\tfor (i = 0, total_length = 0; i < iovcnt; i++) {\n>> > +\t\tif (unsigned_add_overflows(total_length, iov[i].iov_len))\n>> > +\t\t\tbreak;\n>> \n>> We add .iov_len up in this first loop, because we do not want to\n>> bust writev(3p)'s limit.\n>> \n>> \tEINVAL The sum of the iov_len values in the iov array would\n>>               overflow an ssize_t.\n>> \n>> cf. https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html\n>> \n>> As the width of ssize_t in bits can be a lot smaller than size_t,\n>> the above \"unsigned_add_overflows() triggers way too late for the\n>> check to matter, no?\n>\n> I think it is correct as-is.\n>\n> The real check against ssize_t is later, when we compare total_length to\n> MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure\n> we do not overflow size_t when counting up the total (and if we do, we\n> _know_ we are going to overflow ssize_t, which must be smaller).\n\nBut then what happens after it breaks out of the loop?  We cannot be\nat i==0, so let's say we have a reasonably small iov[0] and iov[1]\nthat is so large and makes size_t wraparound.  We break out here,\nand then send the iov[0] with writev().  But have we checked if\niov[0] is under MAX_IO_SIZE in that case before calling writev()?\n\n> I think this can be made more clear by counting down allowable bytes\n> instead of up. I'll show an example in a second.\n>\n>> > +\t}\n>> > +\n>> > +\tif (i < iovcnt) {\n>> > +\t\t/*\n>> > +\t\t * The first entry exceeds MAX_IO_SIZE, so we pass it to\n>> > +\t\t * xwrite, which knows to handle this case.\n>> > +\t\t */\n>> > +\t\tif (!i)\n>> > +\t\t\treturn xwrite(fd, iov->iov_base, iov->iov_len);\n>> \n>> Ben has a similar comment, but it would be easier to see the\n>> correspondence if you rephrase the comment perhaps like\n>> [...]\n>\n> Me three. I think this can be made more clear if we bail to xwrite()\n> immediately in the loop. So together with the count-down, something\n> like:\n>\n>    ssize_t allowed = MAX_IO_SIZE;\n>    int i;\n>\n>    for (i = 0; i < iovcnt; i++) {\n> \tif (iov[i].iov_len > allowed) {\n> \t\tif (!i)\n> \t\t\treturn xwrite(fd, iov->iov_base, iov_len);\n> \t\tbreak;\n> \t}\n> \tallowed -= iov[i].iov_len;\n>   }\n>   return writev(fd, iov, i);\n\nI agree that this is much simpler to follow.\n"},{"id":"541305","messageId":"20260409205928.GD3076846@coredump.intra.peff.net","threadId":"65462","inReplyTo":"xmqq5x5zsw8r.fsf@gitster.g","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-04-09T20:59:28Z","receivedAt":"2026-04-09T20:59:30Z","isPatch":true,"body":"On Thu, Apr 09, 2026 at 01:40:36PM -0700, Junio C Hamano wrote:\n\n> >> As the width of ssize_t in bits can be a lot smaller than size_t,\n> >> the above \"unsigned_add_overflows() triggers way too late for the\n> >> check to matter, no?\n> >\n> > I think it is correct as-is.\n> >\n> > The real check against ssize_t is later, when we compare total_length to\n> > MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure\n> > we do not overflow size_t when counting up the total (and if we do, we\n> > _know_ we are going to overflow ssize_t, which must be smaller).\n> \n> But then what happens after it breaks out of the loop?  We cannot be\n> at i==0, so let's say we have a reasonably small iov[0] and iov[1]\n> that is so large and makes size_t wraparound.  We break out here,\n> and then send the iov[0] with writev().  But have we checked if\n> iov[0] is under MAX_IO_SIZE in that case before calling writev()?\n\nI think so. Either:\n\n  - We completed the first iteration of the loop successfully (and i >=\n    1), in which case we added iov[0].iov_len to total_length, and then\n    compared total_length against MAX_IO_SIZE, but did not break out of\n    the loop. So we know iov[0] is within the limits.\n\n  - We bailed at i==0 either because of addition overflow, or because of\n    the MAX_IO_SIZE check. Either way, we will bail to xwrite() because\n    i is 0.\n\n-Peff\n"},{"id":"541306","messageId":"xmqqzf3brgbt.fsf@gitster.g","threadId":"65462","inReplyTo":"20260409205928.GD3076846@coredump.intra.peff.net","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-09T21:09:42Z","receivedAt":"2026-04-09T21:09:46Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Apr 09, 2026 at 01:40:36PM -0700, Junio C Hamano wrote:\n>\n>> >> As the width of ssize_t in bits can be a lot smaller than size_t,\n>> >> the above \"unsigned_add_overflows() triggers way too late for the\n>> >> check to matter, no?\n>> >\n>> > I think it is correct as-is.\n>> >\n>> > The real check against ssize_t is later, when we compare total_length to\n>> > MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure\n>> > we do not overflow size_t when counting up the total (and if we do, we\n>> > _know_ we are going to overflow ssize_t, which must be smaller).\n>> \n>> But then what happens after it breaks out of the loop?  We cannot be\n>> at i==0, so let's say we have a reasonably small iov[0] and iov[1]\n>> that is so large and makes size_t wraparound.  We break out here,\n>> and then send the iov[0] with writev().  But have we checked if\n>> iov[0] is under MAX_IO_SIZE in that case before calling writev()?\n>\n> I think so. Either:\n>\n>   - We completed the first iteration of the loop successfully (and i >=\n>     1), in which case we added iov[0].iov_len to total_length, and then\n>     compared total_length against MAX_IO_SIZE, but did not break out of\n>     the loop. So we know iov[0] is within the limits.\n>\n>   - We bailed at i==0 either because of addition overflow, or because of\n>     the MAX_IO_SIZE check. Either way, we will bail to xwrite() because\n>     i is 0.\n\nYup, you're right.\n\nThere is no addition overflow at i==0, but I do not think we can\nconstruct a case where the sum is not checked against MAX_IO_SIZE\nbefore the vector is passed to underlying writev().\n\niov[0].iov_len that is slightly smaller than MAX_IO_SIZE would allow\nus to keep looping to i==1 at which time iov[1].iov_len is so big\nthat we may trigger unsigned_add_overflows() check, but then what we\nsend to writev() is the first segment, which is smaller than\nMAX_IO_SIZE, so we are OK.\n\niov[0].iov_len that is slightly larger than MAX_IO_SIZE would stop\nus moving to i==1 at the end of the loop, and directly punt to\nxwrite(), so we are OK, too.\n"},{"id":"541327","messageId":"adiIfQjobtK3MDPW@pks.im","threadId":"65462","inReplyTo":"xmqqzf3brgbt.fsf@gitster.g","subject":"Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-10T05:19:57Z","receivedAt":"2026-04-10T05:20:07Z","isPatch":true,"body":"On Thu, Apr 09, 2026 at 02:09:42PM -0700, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n> > On Thu, Apr 09, 2026 at 01:40:36PM -0700, Junio C Hamano wrote:\n> >\n> >> >> As the width of ssize_t in bits can be a lot smaller than size_t,\n> >> >> the above \"unsigned_add_overflows() triggers way too late for the\n> >> >> check to matter, no?\n> >> >\n> >> > I think it is correct as-is.\n> >> >\n> >> > The real check against ssize_t is later, when we compare total_length to\n> >> > MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure\n> >> > we do not overflow size_t when counting up the total (and if we do, we\n> >> > _know_ we are going to overflow ssize_t, which must be smaller).\n> >> \n> >> But then what happens after it breaks out of the loop?  We cannot be\n> >> at i==0, so let's say we have a reasonably small iov[0] and iov[1]\n> >> that is so large and makes size_t wraparound.  We break out here,\n> >> and then send the iov[0] with writev().  But have we checked if\n> >> iov[0] is under MAX_IO_SIZE in that case before calling writev()?\n> >\n> > I think so. Either:\n> >\n> >   - We completed the first iteration of the loop successfully (and i >=\n> >     1), in which case we added iov[0].iov_len to total_length, and then\n> >     compared total_length against MAX_IO_SIZE, but did not break out of\n> >     the loop. So we know iov[0] is within the limits.\n> >\n> >   - We bailed at i==0 either because of addition overflow, or because of\n> >     the MAX_IO_SIZE check. Either way, we will bail to xwrite() because\n> >     i is 0.\n> \n> Yup, you're right.\n> \n> There is no addition overflow at i==0, but I do not think we can\n> construct a case where the sum is not checked against MAX_IO_SIZE\n> before the vector is passed to underlying writev().\n> \n> iov[0].iov_len that is slightly smaller than MAX_IO_SIZE would allow\n> us to keep looping to i==1 at which time iov[1].iov_len is so big\n> that we may trigger unsigned_add_overflows() check, but then what we\n> send to writev() is the first segment, which is smaller than\n> MAX_IO_SIZE, so we are OK.\n> \n> iov[0].iov_len that is slightly larger than MAX_IO_SIZE would stop\n> us moving to i==1 at the end of the loop, and directly punt to\n> xwrite(), so we are OK, too.\n\nLet's drop this patch for now. I'll pick it up again in the next release\ncycle when reintroducing writev(3p). Thanks, all!\n\nPatrick\n"}]}