All of lore.kernel.org
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jeff King <peff@peff.net>,
	git@vger.kernel.org,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Randall S. Becker" <randall.becker@nexbridge.ca>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()`
Date: Fri, 10 Apr 2026 07:19:57 +0200	[thread overview]
Message-ID: <adiIfQjobtK3MDPW@pks.im> (raw)
In-Reply-To: <xmqqzf3brgbt.fsf@gitster.g>

On Thu, Apr 09, 2026 at 02:09:42PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
> > On Thu, Apr 09, 2026 at 01:40:36PM -0700, Junio C Hamano wrote:
> >
> >> >> As the width of ssize_t in bits can be a lot smaller than size_t,
> >> >> the above "unsigned_add_overflows() triggers way too late for the
> >> >> check to matter, no?
> >> >
> >> > I think it is correct as-is.
> >> >
> >> > The real check against ssize_t is later, when we compare total_length to
> >> > MAX_IO_SIZE (which is clamped to SSIZE_MAX). So this is just making sure
> >> > we do not overflow size_t when counting up the total (and if we do, we
> >> > _know_ we are going to overflow ssize_t, which must be smaller).
> >> 
> >> But then what happens after it breaks out of the loop?  We cannot be
> >> at i==0, so let's say we have a reasonably small iov[0] and iov[1]
> >> that is so large and makes size_t wraparound.  We break out here,
> >> and then send the iov[0] with writev().  But have we checked if
> >> iov[0] is under MAX_IO_SIZE in that case before calling writev()?
> >
> > I think so. Either:
> >
> >   - We completed the first iteration of the loop successfully (and i >=
> >     1), in which case we added iov[0].iov_len to total_length, and then
> >     compared total_length against MAX_IO_SIZE, but did not break out of
> >     the loop. So we know iov[0] is within the limits.
> >
> >   - We bailed at i==0 either because of addition overflow, or because of
> >     the MAX_IO_SIZE check. Either way, we will bail to xwrite() because
> >     i is 0.
> 
> Yup, you're right.
> 
> There is no addition overflow at i==0, but I do not think we can
> construct a case where the sum is not checked against MAX_IO_SIZE
> before the vector is passed to underlying writev().
> 
> iov[0].iov_len that is slightly smaller than MAX_IO_SIZE would allow
> us to keep looping to i==1 at which time iov[1].iov_len is so big
> that we may trigger unsigned_add_overflows() check, but then what we
> send to writev() is the first segment, which is smaller than
> MAX_IO_SIZE, so we are OK.
> 
> iov[0].iov_len that is slightly larger than MAX_IO_SIZE would stop
> us moving to i==1 at the end of the loop, and directly punt to
> xwrite(), so we are OK, too.

Let's drop this patch for now. I'll pick it up again in the next release
cycle when reintroducing writev(3p). Thanks, all!

Patrick

      reply	other threads:[~2026-04-10  5:20 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-04-09 12:47 [PATCH] wrapper: properly handle MAX_IO_SIZE in `write_in_full()` Patrick Steinhardt
2026-04-09 15:46 ` Ben Knoble
2026-04-09 16:42 ` Junio C Hamano
2026-04-09 20:23   ` Jeff King
2026-04-09 20:40     ` Junio C Hamano
2026-04-09 20:59       ` Jeff King
2026-04-09 21:09         ` Junio C Hamano
2026-04-10  5:19           ` Patrick Steinhardt [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=adiIfQjobtK3MDPW@pks.im \
    --to=ps@pks.im \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=randall.becker@nexbridge.ca \
    --cc=sandals@crustytoothpaste.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.