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