[v2,1/4] raw/skeleton: fix failing test case

Message ID 20190621155659.29297-2-bruce.richardson@intel.com (mailing list archive)
State Not Applicable, archived
Delegated to: Thomas Monjalon
Headers
Series fixes and improvements for rawdev |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Bruce Richardson June 21, 2019, 3:56 p.m. UTC
  Rawdev unit test for setting and getting parameters is failing because
of a pointer value being dereferenced after the memory it pointed to is
freed.

The freeing of the malloced memory is difficult when using asserts to
cause early abort of the test cases, since that can leak memory. The
original placement of the free call caused a memory leak if the test
finished early, while a fix for that leak caused the test to fail at
times due to the memory variable being referenced after free. For a case
like this, using stack rather than heap memory is just easier and avoids
all issues.

Fixes: 55ca1b0f2151 ("raw/skeleton: add test cases")
Fixes: 88d0e47880ec ("raw/skeleton: fix memory leak on test failure")
Cc: shreyansh.jain@nxp.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)
  

Comments

Hemant Agrawal June 27, 2019, 11:50 a.m. UTC | #1
Series-Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>

On 21-Jun-19 9:26 PM, Bruce Richardson wrote:
> Rawdev unit test for setting and getting parameters is failing because
> of a pointer value being dereferenced after the memory it pointed to is
> freed.
>
> The freeing of the malloced memory is difficult when using asserts to
> cause early abort of the test cases, since that can leak memory. The
> original placement of the free call caused a memory leak if the test
> finished early, while a fix for that leak caused the test to fail at
> times due to the memory variable being referenced after free. For a case
> like this, using stack rather than heap memory is just easier and avoids
> all issues.
>
> Fixes: 55ca1b0f2151 ("raw/skeleton: add test cases")
> Fixes: 88d0e47880ec ("raw/skeleton: fix memory leak on test failure")
> Cc: shreyansh.jain@nxp.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>   drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c b/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
> index 359c9e296..a0961c77b 100644
> --- a/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
> +++ b/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
> @@ -274,17 +274,14 @@ static int
>   test_rawdev_attr_set_get(void)
>   {
>   	int ret;
> -	int *dummy_value;
> +	int dummy_value_store;
> +	int *dummy_value = &dummy_value_store;
>   	uint64_t ret_value;
>   
>   	/* Set an attribute and fetch it */
>   	ret = rte_rawdev_set_attr(TEST_DEV_ID, "Test1", 100);
>   	RTE_TEST_ASSERT(!ret, "Unable to set an attribute (Test1)");
>   
> -	dummy_value = malloc(sizeof(int));
> -	if (!dummy_value)
> -		RTE_TEST_ASSERT(1, "Unable to allocate memory (dummy_value)");
> -
>   	*dummy_value = 200;
>   	ret = rte_rawdev_set_attr(TEST_DEV_ID, "Test2", (uintptr_t)dummy_value);
>   
> @@ -294,7 +291,6 @@ test_rawdev_attr_set_get(void)
>   			      "Attribute (Test1) not set correctly (%" PRIu64 ")",
>   			      ret_value);
>   
> -	free(dummy_value);
>   
>   	ret_value = 0;
>   	ret = rte_rawdev_get_attr(TEST_DEV_ID, "Test2", &ret_value);
  
Thomas Monjalon July 1, 2019, 6:03 p.m. UTC | #2
21/06/2019 17:56, Bruce Richardson:
> Rawdev unit test for setting and getting parameters is failing because
> of a pointer value being dereferenced after the memory it pointed to is
> freed.
> 
> The freeing of the malloced memory is difficult when using asserts to
> cause early abort of the test cases, since that can leak memory. The
> original placement of the free call caused a memory leak if the test
> finished early, while a fix for that leak caused the test to fail at
> times due to the memory variable being referenced after free. For a case
> like this, using stack rather than heap memory is just easier and avoids
> all issues.
> 
> Fixes: 55ca1b0f2151 ("raw/skeleton: add test cases")
> Fixes: 88d0e47880ec ("raw/skeleton: fix memory leak on test failure")
> Cc: shreyansh.jain@nxp.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

This test is already fixed by another patch:
http://git.dpdk.org/dpdk/commit/?id=dcb1595956
  

Patch

diff --git a/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c b/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
index 359c9e296..a0961c77b 100644
--- a/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
+++ b/drivers/raw/skeleton_rawdev/skeleton_rawdev_test.c
@@ -274,17 +274,14 @@  static int
 test_rawdev_attr_set_get(void)
 {
 	int ret;
-	int *dummy_value;
+	int dummy_value_store;
+	int *dummy_value = &dummy_value_store;
 	uint64_t ret_value;
 
 	/* Set an attribute and fetch it */
 	ret = rte_rawdev_set_attr(TEST_DEV_ID, "Test1", 100);
 	RTE_TEST_ASSERT(!ret, "Unable to set an attribute (Test1)");
 
-	dummy_value = malloc(sizeof(int));
-	if (!dummy_value)
-		RTE_TEST_ASSERT(1, "Unable to allocate memory (dummy_value)");
-
 	*dummy_value = 200;
 	ret = rte_rawdev_set_attr(TEST_DEV_ID, "Test2", (uintptr_t)dummy_value);
 
@@ -294,7 +291,6 @@  test_rawdev_attr_set_get(void)
 			      "Attribute (Test1) not set correctly (%" PRIu64 ")",
 			      ret_value);
 
-	free(dummy_value);
 
 	ret_value = 0;
 	ret = rte_rawdev_get_attr(TEST_DEV_ID, "Test2", &ret_value);