Re: [PATCH] util: fix confusion between offset in the iovec array and in the entry

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

On Tue, Mar 19, 2024 at 11:13:40AM +0100, Laurent Vivier wrote:
> In write_remainder() 'skip' is the offset to start the operation from
> in the iovec array.
> 
> In iov_skip_bytes(), 'skip' is also the offset in the iovec array but
> 'offset' is the first unskipped byte in the iovec entry.
> 
> As write_remainder() uses 'skip' for both, 'skip' is reset to the
> first unskipped byte in the iovec entry rather to staying the first
> unskipped byte in the iovec array.
> 
> Fix the problem by introducing a new variable not to overwrite 'skip'
> on each loop.
> 
> Fixes: 8bdb0883b441 ("util: Add write_remainder() helper")
> Cc: david@gibson.dropbear.id.au
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Ouch.  *dons paper bag*

I believe this is correct as it stands, but..

> ---
>  util.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/util.c b/util.c
> index 3b2393d6bfa8..eee53aed811b 100644
> --- a/util.c
> +++ b/util.c
> @@ -533,13 +533,14 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>  int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip)
>  {
>  	int i;
> +	size_t offset;
>  
> -	while ((i = iov_skip_bytes(iov, iovcnt, skip, &skip)) < iovcnt) {
> +	while ((i = iov_skip_bytes(iov, iovcnt, skip, &offset)) < iovcnt) {
>  		ssize_t rc;
>  
>  		if (skip) {

.. it would be more optimal if you checked for offset != 0 rather than
skip != 0 here.  Otherwise even once we've written an entire buffer,
we'll continue to write the rest of the buffers one by one, rather
than writing them all with a writev().

> -			rc = write(fd, (char *)iov[i].iov_base + skip,
> -				   iov[i].iov_len - skip);
> +			rc = write(fd, (char *)iov[i].iov_base + offset,
> +				   iov[i].iov_len - offset);
>  		} else {
>  			rc = writev(fd, &iov[i], iovcnt - i);
>  		}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]