diff mbox series

test/timer: fix memzone reserve failure check

Message ID 1619083120-57343-1-git-send-email-humin29@huawei.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series test/timer: fix memzone reserve failure check |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

humin (Q) April 22, 2021, 9:18 a.m. UTC 
  Segmentation fault may occur without checking if memzone
reserves succeed or not.

This patch fixed it.

Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
Cc: stable@dpdk.org

Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
---
 app/test/test_timer_secondary.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Carrillo, Erik G May 1, 2021, 8 p.m. UTC | #1 
> -----Original Message-----
> From: Min Hu (Connor) <humin29@huawei.com>
> Sent: Thursday, April 22, 2021 4:19 AM
> To: dev@dpdk.org
> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; rsanford@akamai.com; Carrillo,
> Erik G <erik.g.carrillo@intel.com>
> Subject: [PATCH] test/timer: fix memzone reserve failure check
> 
> Segmentation fault may occur without checking if memzone reserves
> succeed or not.
> 
> This patch fixed it.
> 
> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> ---
>  app/test/test_timer_secondary.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/app/test/test_timer_secondary.c
> b/app/test/test_timer_secondary.c index 1e8f1d4..281f5bd 100644
> --- a/app/test/test_timer_secondary.c
> +++ b/app/test/test_timer_secondary.c
> @@ -125,6 +125,11 @@ test_timer_secondary(void)
> 
>  		mz = rte_memzone_reserve(TEST_INFO_MZ_NAME,
> sizeof(*test_info),
>  					 SOCKET_ID_ANY, 0);
> +		if (mz == NULL) {
> +			printf("Failed to reserve memzone\n");
> +			return TEST_SKIPPED;
> +		}
> +
>  		test_info = mz->addr;
>  		TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate
> memory for "
>  				     "test data");

I think the TEST_ASSERT_NOT_NULL check here should be the area we update -- instead of checking the test_info pointer, we should check "mz", and that will address the issue you have noted.  We can then move the "test_info = mz->addr" assignment below the assert.

> @@ -171,6 +176,11 @@ test_timer_secondary(void)
>  		int i;
> 
>  		mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
> +		if (mz == NULL) {
> +			printf("Failed to lookup memzone\n");
> +			return TEST_SKIPPED;
> +		}
> +
>  		test_info = mz->addr;
>  		TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup
> memzone for "
>  				     "test info");

Same thing here -- we can update the TEST_ASSERT_NOT_NULL call here instead, and move it above the "test_info = mz->addr" assignment.

Thanks,
Erik

> --
> 2.7.4
humin (Q) May 4, 2021, 1:08 a.m. UTC | #2 
在 2021/5/2 4:00, Carrillo, Erik G 写道:
>> -----Original Message-----
>> From: Min Hu (Connor) <humin29@huawei.com>
>> Sent: Thursday, April 22, 2021 4:19 AM
>> To: dev@dpdk.org
>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; rsanford@akamai.com; Carrillo,
>> Erik G <erik.g.carrillo@intel.com>
>> Subject: [PATCH] test/timer: fix memzone reserve failure check
>>
>> Segmentation fault may occur without checking if memzone reserves
>> succeed or not.
>>
>> This patch fixed it.
>>
>> Fixes: 50247fe03fe0 ("test/timer: exercise new APIs in secondary process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>> ---
>>   app/test/test_timer_secondary.c | 10 ++++++++++
>>   1 file changed, 10 insertions(+)
>>
>> diff --git a/app/test/test_timer_secondary.c
>> b/app/test/test_timer_secondary.c index 1e8f1d4..281f5bd 100644
>> --- a/app/test/test_timer_secondary.c
>> +++ b/app/test/test_timer_secondary.c
>> @@ -125,6 +125,11 @@ test_timer_secondary(void)
>>
>>   		mz = rte_memzone_reserve(TEST_INFO_MZ_NAME,
>> sizeof(*test_info),
>>   					 SOCKET_ID_ANY, 0);
>> +		if (mz == NULL) {
>> +			printf("Failed to reserve memzone\n");
>> +			return TEST_SKIPPED;
>> +		}
>> +
>>   		test_info = mz->addr;
>>   		TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate
>> memory for "
>>   				     "test data");
> 
> I think the TEST_ASSERT_NOT_NULL check here should be the area we update -- instead of checking the test_info pointer, we should check "mz", and that will address the issue you have noted.  We can then move the "test_info = mz->addr" assignment below the assert.
> 
Agreed, fixed in v2, thanks.
>> @@ -171,6 +176,11 @@ test_timer_secondary(void)
>>   		int i;
>>
>>   		mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
>> +		if (mz == NULL) {
>> +			printf("Failed to lookup memzone\n");
>> +			return TEST_SKIPPED;
>> +		}
>> +
>>   		test_info = mz->addr;
>>   		TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup
>> memzone for "
>>   				     "test info");
> 
> Same thing here -- we can update the TEST_ASSERT_NOT_NULL call here instead, and move it above the "test_info = mz->addr" assignment.
> 
Fixed in v2, thanks.
> Thanks,
> Erik
> 
>> --
>> 2.7.4
> 
> .
>
diff mbox series

Patch

diff --git a/app/test/test_timer_secondary.c b/app/test/test_timer_secondary.c
index 1e8f1d4..281f5bd 100644
--- a/app/test/test_timer_secondary.c
+++ b/app/test/test_timer_secondary.c
@@ -125,6 +125,11 @@  test_timer_secondary(void)
 
 		mz = rte_memzone_reserve(TEST_INFO_MZ_NAME, sizeof(*test_info),
 					 SOCKET_ID_ANY, 0);
+		if (mz == NULL) {
+			printf("Failed to reserve memzone\n");
+			return TEST_SKIPPED;
+		}
+
 		test_info = mz->addr;
 		TEST_ASSERT_NOT_NULL(test_info, "Couldn't allocate memory for "
 				     "test data");
@@ -171,6 +176,11 @@  test_timer_secondary(void)
 		int i;
 
 		mz = rte_memzone_lookup(TEST_INFO_MZ_NAME);
+		if (mz == NULL) {
+			printf("Failed to lookup memzone\n");
+			return TEST_SKIPPED;
+		}
+
 		test_info = mz->addr;
 		TEST_ASSERT_NOT_NULL(test_info, "Couldn't lookup memzone for "
 				     "test info");