test/mbuf: fix access to freed memory
Checks
Commit Message
Seen by ASan.
In the external buffer mbuf test, we check that the buffer is freed
by checking that its refcount is 0. This is not a valid condition,
because it accesses to an already freed area.
Fix this by setting a boolean flag in the callback when rte_free()
is actually called, and check this flag instead.
Bugzilla ID: 867
Fixes: 7b295dceea07 ("test/mbuf: add unit test cases")
Cc: stable@dpdk.org
Reported-by: David Marchand <david.marchand@redhat.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
app/test/test_mbuf.c | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
Comments
On Fri, Oct 29, 2021 at 2:16 PM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> Seen by ASan.
>
> In the external buffer mbuf test, we check that the buffer is freed
> by checking that its refcount is 0. This is not a valid condition,
> because it accesses to an already freed area.
>
> Fix this by setting a boolean flag in the callback when rte_free()
> is actually called, and check this flag instead.
>
> Bugzilla ID: 867
> Fixes: 7b295dceea07 ("test/mbuf: add unit test cases")
> Cc: stable@dpdk.org
>
> Reported-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
On Wed, Nov 3, 2021 at 4:00 PM David Marchand <david.marchand@redhat.com> wrote:
> On Fri, Oct 29, 2021 at 2:16 PM Olivier Matz <olivier.matz@6wind.com> wrote:
> >
> > Seen by ASan.
> >
> > In the external buffer mbuf test, we check that the buffer is freed
> > by checking that its refcount is 0. This is not a valid condition,
> > because it accesses to an already freed area.
> >
> > Fix this by setting a boolean flag in the callback when rte_free()
> > is actually called, and check this flag instead.
> >
> > Bugzilla ID: 867
> > Fixes: 7b295dceea07 ("test/mbuf: add unit test cases")
> > Cc: stable@dpdk.org
> >
> > Reported-by: David Marchand <david.marchand@redhat.com>
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
Applied, thanks.
@@ -2307,16 +2307,16 @@ test_pktmbuf_read_from_chain(struct rte_mempool *pktmbuf_pool)
/* Define a free call back function to be used for external buffer */
static void
-ext_buf_free_callback_fn(void *addr __rte_unused, void *opaque)
+ext_buf_free_callback_fn(void *addr, void *opaque)
{
- void *ext_buf_addr = opaque;
+ bool *freed = opaque;
- if (ext_buf_addr == NULL) {
+ if (addr == NULL) {
printf("External buffer address is invalid\n");
return;
}
- rte_free(ext_buf_addr);
- ext_buf_addr = NULL;
+ rte_free(addr);
+ *freed = true;
printf("External buffer freed via callback\n");
}
@@ -2340,6 +2340,7 @@ test_pktmbuf_ext_shinfo_init_helper(struct rte_mempool *pktmbuf_pool)
void *ext_buf_addr = NULL;
uint16_t buf_len = EXT_BUF_TEST_DATA_LEN +
sizeof(struct rte_mbuf_ext_shared_info);
+ bool freed = false;
/* alloc a mbuf */
m = rte_pktmbuf_alloc(pktmbuf_pool);
@@ -2355,7 +2356,7 @@ test_pktmbuf_ext_shinfo_init_helper(struct rte_mempool *pktmbuf_pool)
GOTO_FAIL("%s: External buffer allocation failed\n", __func__);
ret_shinfo = rte_pktmbuf_ext_shinfo_init_helper(ext_buf_addr, &buf_len,
- ext_buf_free_callback_fn, ext_buf_addr);
+ ext_buf_free_callback_fn, &freed);
if (ret_shinfo == NULL)
GOTO_FAIL("%s: Shared info initialization failed!\n", __func__);
@@ -2388,26 +2389,35 @@ test_pktmbuf_ext_shinfo_init_helper(struct rte_mempool *pktmbuf_pool)
if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 2)
GOTO_FAIL("%s: Invalid ext_buf ref_cnt\n", __func__);
+ if (freed)
+ GOTO_FAIL("%s: extbuf should not be freed\n", __func__);
/* test to manually update ext_buf_ref_cnt from 2 to 3*/
rte_mbuf_ext_refcnt_update(ret_shinfo, 1);
if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 3)
GOTO_FAIL("%s: Update ext_buf ref_cnt failed\n", __func__);
+ if (freed)
+ GOTO_FAIL("%s: extbuf should not be freed\n", __func__);
/* reset the ext_refcnt before freeing the external buffer */
rte_mbuf_ext_refcnt_set(ret_shinfo, 2);
if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 2)
GOTO_FAIL("%s: set ext_buf ref_cnt failed\n", __func__);
+ if (freed)
+ GOTO_FAIL("%s: extbuf should not be freed\n", __func__);
/* detach the external buffer from mbufs */
rte_pktmbuf_detach_extbuf(m);
/* check if ref cnt is decremented */
if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 1)
GOTO_FAIL("%s: Invalid ext_buf ref_cnt\n", __func__);
+ if (freed)
+ GOTO_FAIL("%s: extbuf should not be freed\n", __func__);
rte_pktmbuf_detach_extbuf(clone);
- if (rte_mbuf_ext_refcnt_read(ret_shinfo) != 0)
- GOTO_FAIL("%s: Invalid ext_buf ref_cnt\n", __func__);
+ if (!freed)
+ GOTO_FAIL("%s: extbuf should be freed\n", __func__);
+ freed = false;
rte_pktmbuf_free(m);
m = NULL;