Re: [PATCH 1/2] util/iov: Do not assert offset is in iov

[Jason Wang]

On Sat, Jan 11, 2025 at 1:11 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Jason, can you pull this series?

Queued.

Thanks

>
> Regards,
> Akihiko Odaki
>
> On 2024/05/08 23:51, Philippe Mathieu-Daudé wrote:
> > ping?
> >
> > On 28/4/24 13:11, Akihiko Odaki wrote:
> >> iov_from_buf(), iov_to_buf(), iov_memset(), and iov_copy() asserts
> >> that the given offset fits in the iov while tolerating the specified
> >> number of bytes to operate with to be greater than the size of iov.
> >> This is inconsistent so remove the assertions.
> >>
> >> Asserting the offset fits in the iov makes sense if it is expected that
> >> there are other operations that process the content before the offset
> >> and the content is processed in order. Under this expectation, the
> >> offset should point to the end of bytes that are previously processed
> >> and fit in the iov. However, this expectation depends on the details of
> >> the caller, and did not hold true at least one case and required code to
> >> check iov_size(), which is added with commit 83ddb3dbba2e
> >> ("hw/net/net_tx_pkt: Fix overrun in update_sctp_checksum()").
> >>
> >> Adding such a check is inefficient and error-prone. These functions
> >> already tolerate the specified number of bytes to operate with to be
> >> greater than the size of iov to avoid such checks so remove the
> >> assertions to tolerate invalid offset as well. They return the number of
> >> bytes they operated with so their callers can still check the returned
> >> value to ensure there are sufficient space at the given offset.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >>   include/qemu/iov.h | 5 +++--
> >>   util/iov.c         | 5 -----
> >>   2 files changed, 3 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/qemu/iov.h b/include/qemu/iov.h
> >> index 63a1c01965d1..33548058d2ee 100644
> >> --- a/include/qemu/iov.h
> >> +++ b/include/qemu/iov.h
> >> @@ -30,7 +30,7 @@ size_t iov_size(const struct iovec *iov, const
> >> unsigned int iov_cnt);
> >>    * only part of data will be copied, up to the end of the iovec.
> >>    * Number of bytes actually copied will be returned, which is
> >>    *  min(bytes, iov_size(iov)-offset)
> >> - * `Offset' must point to the inside of iovec.
> >> + * Returns 0 when `offset' points to the outside of iovec.
> >>    */
> >>   size_t iov_from_buf_full(const struct iovec *iov, unsigned int iov_cnt,
> >>                            size_t offset, const void *buf, size_t bytes);
> >> @@ -66,11 +66,12 @@ iov_to_buf(const struct iovec *iov, const unsigned
> >> int iov_cnt,
> >>   /**
> >>    * Set data bytes pointed out by iovec `iov' of size `iov_cnt'
> >> elements,
> >>    * starting at byte offset `start', to value `fillc', repeating it
> >> - * `bytes' number of times.  `Offset' must point to the inside of iovec.
> >> + * `bytes' number of times.
> >>    * If `bytes' is large enough, only last bytes portion of iovec,
> >>    * up to the end of it, will be filled with the specified value.
> >>    * Function return actual number of bytes processed, which is
> >>    * min(size, iov_size(iov) - offset).
> >> + * Returns 0 when `offset' points to the outside of iovec.
> >>    */
> >>   size_t iov_memset(const struct iovec *iov, const unsigned int iov_cnt,
> >>                     size_t offset, int fillc, size_t bytes);
> >> diff --git a/util/iov.c b/util/iov.c
> >> index 7e73948f5e3d..a523b406b7f8 100644
> >> --- a/util/iov.c
> >> +++ b/util/iov.c
> >> @@ -36,7 +36,6 @@ size_t iov_from_buf_full(const struct iovec *iov,
> >> unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -55,7 +54,6 @@ size_t iov_to_buf_full(const struct iovec *iov,
> >> const unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -74,7 +72,6 @@ size_t iov_memset(const struct iovec *iov, const
> >> unsigned int iov_cnt,
> >>               offset -= iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(offset == 0);
> >>       return done;
> >>   }
> >> @@ -266,7 +263,6 @@ unsigned iov_copy(struct iovec *dst_iov, unsigned
> >> int dst_iov_cnt,
> >>           bytes -= len;
> >>           offset = 0;
> >>       }
> >> -    assert(offset == 0);
> >>       return j;
> >>   }
> >> @@ -337,7 +333,6 @@ size_t qemu_iovec_concat_iov(QEMUIOVector *dst,
> >>               soffset -= src_iov[i].iov_len;
> >>           }
> >>       }
> >> -    assert(soffset == 0); /* offset beyond end of src */
> >>       return done;
> >>   }
> >>
>