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

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

[Laurent Vivier]

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>
---
 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) {
-			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);
 		}
-- 
@@ -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) {
-			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);
 		}
-- 
2.42.0

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

[David Gibson]

[-- 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 --]

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

[Laurent Vivier]

On 3/20/24 05:27, David Gibson wrote:
> 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().
> 
Yes, you're right. I missed that.

Thanks,
Laurent