On Fri, Jun 3, 2022 at 9:56 AM Bruce Richardson
<bruce.richard...@intel.com> wrote:
>
> On Fri, Jun 03, 2022 at 09:45:45AM +0200, David Marchand wrote:
> > Hello Vladimir,
> >
> > On Thu, Jun 2, 2022 at 8:42 PM Medvedkin, Vladimir
> > <vladimir.medved...@intel.com> wrote:
> > > > if (!dst) {
> > > > rte_pktmbuf_free(m);
> > > > return NULL;
> > > > }
> > > > - if (string != NULL)
> > > > - rte_memcpy(dst, string, t_len);
> > > > - else
> > > > - memset(dst, 0, t_len);
> > > > + if (string != NULL) {
> > > > + size_t off;
> > > > +
> > > > + for (off = 0; off + string_len < len; off +=
> > > > string_len)
> > >
> > > I think it should be off + string_len <= len here, because otherwise, if
> > > len is a multiple of string_len, the last ret_memcpy (after this loop)
> > > will copy 0 bytes.
> >
> > Changing to off + string_len <= len would trigger an oob access to dst
> > (by one extra byte)?
> > Otoh, I don't think it is an issue to have a 0-length call to rte_memcpy.
> >
> Given this is test code, do we need rte_memcpy for performance over regular
> libc memcpy? Does fixing the warning become any easier or clearer if libc
> memcpy is used?
There was a similar proposal in vhost/crypto code.
I am not a fan to switching to libc memcpy.
We would be waiving a potential issue in rte_memcpy itself (which
could also be a problem in how gcc understands this inlined code) or
in the rte_memcpy caller code.
Here, gcc is probably too picky.
No path currently leads to oob access on the src string.
Adding a simple hint (see simplified hunk below) seems to help gcc enough:
@@ -554,12 +554,14 @@ struct rte_ipv4_hdr ipv4_outer = {
};
static struct rte_mbuf *
-setup_test_string(struct rte_mempool *mpool,
- const char *string, size_t len, uint8_t blocksize)
+setup_test_string(struct rte_mempool *mpool, const char *string,
+ size_t string_len, size_t len, uint8_t blocksize)
{
struct rte_mbuf *m = rte_pktmbuf_alloc(mpool);
size_t t_len = len - (blocksize ? (len % blocksize) : 0);
+ RTE_VERIFY(len <= string_len);
+
if (m) {
memset(m->buf_addr, 0, m->buf_len);
--
David Marchand
