[PATCHv2,2/4] test: odp_timer.c: avoid dereferencing ptr after NULL check

Message ID 1421427959-11751-3-git-send-email-ola.liljedahl@linaro.org
State New
Headers show

Commit Message

Ola Liljedahl Jan. 16, 2015, 5:05 p.m.
Don't dereference pointer after successful check for NULL as this makes Coverity
complain. (Coverity CID 85397, https://bugs.linaro.org/show_bug.cgi?id=1056)

Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
---
(This document/code contribution attached is provided under the terms of
agreement LES-LTM-21309)

 test/validation/odp_timer.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Mike Holmes Jan. 26, 2015, 1:48 p.m. | #1
On 16 January 2015 at 12:05, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> Don't dereference pointer after successful check for NULL as this makes
> Coverity
> complain. (Coverity CID 85397,
> https://bugs.linaro.org/show_bug.cgi?id=1056)
>
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
>
>  test/validation/odp_timer.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
> index dca01ce..5ba1b84 100644
> --- a/test/validation/odp_timer.c
> +++ b/test/validation/odp_timer.c
> @@ -69,23 +69,23 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
> uint64_t prev_tick)
>         if (ttp == NULL)
>                 CU_FAIL("odp_timeout_user_ptr() null user ptr");
>

If you use CU_FAIL_FATAL  or CU_ASSERT_PTR_NOT_NULL_FATAL here, it will
abort the test and you wont need to test for null in the following cases.
http://cunit.sourceforge.net/doc/writing_tests.html#assertions


>
> -       if (ttp->buf2 != buf)
> +       if (ttp != NULL && ttp->buf2 != buf)
>                 CU_FAIL("odp_timeout_user_ptr() wrong user ptr");
> -       if (ttp->tim != tim)
> +       if (ttp != NULL && ttp->tim != tim)
>                 CU_FAIL("odp_timeout_timer() wrong timer");
>         if (stale) {
>                 if (odp_timeout_fresh(tmo))
>                         CU_FAIL("Wrong status (fresh) for stale timeout");
>                 /* Stale timeout => local timer must have invalid tick */
> -               if (ttp->tick != TICK_INVALID)
> +               if (ttp != NULL && ttp->tick != TICK_INVALID)
>                         CU_FAIL("Stale timeout for active timer");
>         } else {
>                 if (!odp_timeout_fresh(tmo))
>                         CU_FAIL("Wrong status (stale) for fresh timeout");
>                 /* Fresh timeout => local timer must have matching tick */
> -               if (ttp->tick != tick) {
> -                       printf("Wrong tick: expected %"PRIu64" actual
> %"PRIu64"\n",
> -                              ttp->tick, tick);
> +               if (ttp != NULL && ttp->tick != tick) {
> +                       LOG_DBG("Wrong tick: expected %"PRIu64" actual
> %"PRIu64"\n",
> +                               ttp->tick, tick);
>                         CU_FAIL("odp_timeout_tick() wrong tick");
>                 }
>                 /* Check that timeout was delivered 'timely' */
> @@ -99,9 +99,11 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
> uint64_t prev_tick)
>                 }
>         }
>
> -       /* Use assert() for correctness check of test program itself */
> -       assert(ttp->buf == ODP_BUFFER_INVALID);
> -       ttp->buf = buf;
> +       if (ttp != NULL) {
> +               /* Internal error */
> +               CU_ASSERT_FATAL(ttp->buf == ODP_BUFFER_INVALID);
> +               ttp->buf = buf;
> +       }
>  }
>
>  /* @private Worker thread entrypoint which performs timer
> alloc/set/cancel/free
> --
> 1.9.1
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>
Ola Liljedahl Jan. 26, 2015, 1:51 p.m. | #2
On 26 January 2015 at 14:48, Mike Holmes <mike.holmes@linaro.org> wrote:
>
>
> On 16 January 2015 at 12:05, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:
>>
>> Don't dereference pointer after successful check for NULL as this makes
>> Coverity
>> complain. (Coverity CID 85397,
>> https://bugs.linaro.org/show_bug.cgi?id=1056)
>>
>> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>> ---
>> (This document/code contribution attached is provided under the terms of
>> agreement LES-LTM-21309)
>>
>>  test/validation/odp_timer.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
>> index dca01ce..5ba1b84 100644
>> --- a/test/validation/odp_timer.c
>> +++ b/test/validation/odp_timer.c
>> @@ -69,23 +69,23 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
>> uint64_t prev_tick)
>>         if (ttp == NULL)
>>                 CU_FAIL("odp_timeout_user_ptr() null user ptr");
>
>
> If you use CU_FAIL_FATAL  or CU_ASSERT_PTR_NOT_NULL_FATAL here, it will
> abort the test and you wont need to test for null in the following cases.
> http://cunit.sourceforge.net/doc/writing_tests.html#assertions
Yes but this error is not a fatal error. I want the validation to
continue even if the timer implementation returns a bogus user
pointer.

>
>>
>>
>> -       if (ttp->buf2 != buf)
>> +       if (ttp != NULL && ttp->buf2 != buf)
>>                 CU_FAIL("odp_timeout_user_ptr() wrong user ptr");
>> -       if (ttp->tim != tim)
>> +       if (ttp != NULL && ttp->tim != tim)
>>                 CU_FAIL("odp_timeout_timer() wrong timer");
>>         if (stale) {
>>                 if (odp_timeout_fresh(tmo))
>>                         CU_FAIL("Wrong status (fresh) for stale timeout");
>>                 /* Stale timeout => local timer must have invalid tick */
>> -               if (ttp->tick != TICK_INVALID)
>> +               if (ttp != NULL && ttp->tick != TICK_INVALID)
>>                         CU_FAIL("Stale timeout for active timer");
>>         } else {
>>                 if (!odp_timeout_fresh(tmo))
>>                         CU_FAIL("Wrong status (stale) for fresh timeout");
>>                 /* Fresh timeout => local timer must have matching tick */
>> -               if (ttp->tick != tick) {
>> -                       printf("Wrong tick: expected %"PRIu64" actual
>> %"PRIu64"\n",
>> -                              ttp->tick, tick);
>> +               if (ttp != NULL && ttp->tick != tick) {
>> +                       LOG_DBG("Wrong tick: expected %"PRIu64" actual
>> %"PRIu64"\n",
>> +                               ttp->tick, tick);
>>                         CU_FAIL("odp_timeout_tick() wrong tick");
>>                 }
>>                 /* Check that timeout was delivered 'timely' */
>> @@ -99,9 +99,11 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
>> uint64_t prev_tick)
>>                 }
>>         }
>>
>> -       /* Use assert() for correctness check of test program itself */
>> -       assert(ttp->buf == ODP_BUFFER_INVALID);
>> -       ttp->buf = buf;
>> +       if (ttp != NULL) {
>> +               /* Internal error */
>> +               CU_ASSERT_FATAL(ttp->buf == ODP_BUFFER_INVALID);
>> +               ttp->buf = buf;
>> +       }
>>  }
>>
>>  /* @private Worker thread entrypoint which performs timer
>> alloc/set/cancel/free
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> lng-odp mailing list
>> lng-odp@lists.linaro.org
>> http://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
> --
> Mike Holmes
> Linaro  Sr Technical Manager
> LNG - ODP
Mike Holmes Jan. 26, 2015, 1:54 p.m. | #3
On 16 January 2015 at 12:05, Ola Liljedahl <ola.liljedahl@linaro.org> wrote:

> Don't dereference pointer after successful check for NULL as this makes
> Coverity
> complain. (Coverity CID 85397,
> https://bugs.linaro.org/show_bug.cgi?id=1056)
>
> Signed-off-by: Ola Liljedahl <ola.liljedahl@linaro.org>
>

Reviewed-by: Mike Holmes <mike.holmes@linaro.org>


> ---
> (This document/code contribution attached is provided under the terms of
> agreement LES-LTM-21309)
>
>  test/validation/odp_timer.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
> index dca01ce..5ba1b84 100644
> --- a/test/validation/odp_timer.c
> +++ b/test/validation/odp_timer.c
> @@ -69,23 +69,23 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
> uint64_t prev_tick)
>         if (ttp == NULL)
>                 CU_FAIL("odp_timeout_user_ptr() null user ptr");
>
> -       if (ttp->buf2 != buf)
> +       if (ttp != NULL && ttp->buf2 != buf)
>                 CU_FAIL("odp_timeout_user_ptr() wrong user ptr");
> -       if (ttp->tim != tim)
> +       if (ttp != NULL && ttp->tim != tim)
>                 CU_FAIL("odp_timeout_timer() wrong timer");
>         if (stale) {
>                 if (odp_timeout_fresh(tmo))
>                         CU_FAIL("Wrong status (fresh) for stale timeout");
>                 /* Stale timeout => local timer must have invalid tick */
> -               if (ttp->tick != TICK_INVALID)
> +               if (ttp != NULL && ttp->tick != TICK_INVALID)
>                         CU_FAIL("Stale timeout for active timer");
>         } else {
>                 if (!odp_timeout_fresh(tmo))
>                         CU_FAIL("Wrong status (stale) for fresh timeout");
>                 /* Fresh timeout => local timer must have matching tick */
> -               if (ttp->tick != tick) {
> -                       printf("Wrong tick: expected %"PRIu64" actual
> %"PRIu64"\n",
> -                              ttp->tick, tick);
> +               if (ttp != NULL && ttp->tick != tick) {
> +                       LOG_DBG("Wrong tick: expected %"PRIu64" actual
> %"PRIu64"\n",
> +                               ttp->tick, tick);
>                         CU_FAIL("odp_timeout_tick() wrong tick");
>                 }
>                 /* Check that timeout was delivered 'timely' */
> @@ -99,9 +99,11 @@ static void handle_tmo(odp_buffer_t buf, bool stale,
> uint64_t prev_tick)
>                 }
>         }
>
> -       /* Use assert() for correctness check of test program itself */
> -       assert(ttp->buf == ODP_BUFFER_INVALID);
> -       ttp->buf = buf;
> +       if (ttp != NULL) {
> +               /* Internal error */
> +               CU_ASSERT_FATAL(ttp->buf == ODP_BUFFER_INVALID);
> +               ttp->buf = buf;
> +       }
>  }
>
>  /* @private Worker thread entrypoint which performs timer
> alloc/set/cancel/free
> --
> 1.9.1
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/lng-odp
>

Patch

diff --git a/test/validation/odp_timer.c b/test/validation/odp_timer.c
index dca01ce..5ba1b84 100644
--- a/test/validation/odp_timer.c
+++ b/test/validation/odp_timer.c
@@ -69,23 +69,23 @@  static void handle_tmo(odp_buffer_t buf, bool stale, uint64_t prev_tick)
 	if (ttp == NULL)
 		CU_FAIL("odp_timeout_user_ptr() null user ptr");
 
-	if (ttp->buf2 != buf)
+	if (ttp != NULL && ttp->buf2 != buf)
 		CU_FAIL("odp_timeout_user_ptr() wrong user ptr");
-	if (ttp->tim != tim)
+	if (ttp != NULL && ttp->tim != tim)
 		CU_FAIL("odp_timeout_timer() wrong timer");
 	if (stale) {
 		if (odp_timeout_fresh(tmo))
 			CU_FAIL("Wrong status (fresh) for stale timeout");
 		/* Stale timeout => local timer must have invalid tick */
-		if (ttp->tick != TICK_INVALID)
+		if (ttp != NULL && ttp->tick != TICK_INVALID)
 			CU_FAIL("Stale timeout for active timer");
 	} else {
 		if (!odp_timeout_fresh(tmo))
 			CU_FAIL("Wrong status (stale) for fresh timeout");
 		/* Fresh timeout => local timer must have matching tick */
-		if (ttp->tick != tick) {
-			printf("Wrong tick: expected %"PRIu64" actual %"PRIu64"\n",
-			       ttp->tick, tick);
+		if (ttp != NULL && ttp->tick != tick) {
+			LOG_DBG("Wrong tick: expected %"PRIu64" actual %"PRIu64"\n",
+				ttp->tick, tick);
 			CU_FAIL("odp_timeout_tick() wrong tick");
 		}
 		/* Check that timeout was delivered 'timely' */
@@ -99,9 +99,11 @@  static void handle_tmo(odp_buffer_t buf, bool stale, uint64_t prev_tick)
 		}
 	}
 
-	/* Use assert() for correctness check of test program itself */
-	assert(ttp->buf == ODP_BUFFER_INVALID);
-	ttp->buf = buf;
+	if (ttp != NULL) {
+		/* Internal error */
+		CU_ASSERT_FATAL(ttp->buf == ODP_BUFFER_INVALID);
+		ttp->buf = buf;
+	}
 }
 
 /* @private Worker thread entrypoint which performs timer alloc/set/cancel/free