Re: [PATCH 12/12] test/ipsec: fix build with GCC 12

[David Marchand <david.marchand@redhat.com>]

On Fri, Jun 3, 2022 at 9:56 AM Bruce Richardson
<bruce.richardson@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.medvedkin@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