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

["Medvedkin, Vladimir" <vladimir.medvedkin@intel.com>]

Hi David,

On 03/06/2022 10:41, David Marchand wrote:
> 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.
>>>

The problem here is that if, for example, string_len is 8 bytes and len 
is 16, then it will write only 8 bytes.

>> 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);
> +

RTE_VERIFY looks better here to make picky GCC happy.

> 
>       if (m) {
>           memset(m->buf_addr, 0, m->buf_len);
> 
> 

-- 
Regards,
Vladimir