fix possible NULL pointer dereference after call to configASSERT()

[laroche]

Compiling with clang static code analysis, possible NULL pointer dereference are found. Since configASSERT() can possibly return and continue "normal" operation, the code in queue.c and stream_buffer.c can be adjusted to avoid NULL pointer exceptions.

Description

Test Steps

Checklist:

• I have tested my changes. No regression in existing tests.
• I have modified and/or added unit-tests to cover the code changes in this Pull Request.

Related Issue

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[aggarg]

configASSERT is a debugging aid designed to catch programmer errors during development. When an assertion fails, the program execution is not supposed to continue but it is supposed to halt, allowing the developer to address the underlying issue. A typical implementation of configASSERT is:

#define configASSERT( x )   if ( ( x ) == 0 ) { taskDISABLE_INTERRUPTS(); for( ;; ); }

This implementation allows developers to halt execution at the point of failure and use their debugger to examine the call stack, helping identify the root cause of the assertion failure.

[memphis242]

Your changes generally seem to be configASSERT( ptr ) → configASSERT( ptr != NULL ), and although I do personally find that more readable, it's equivalent and not strictly necessary. I do also see that you combine assert conditions into one assert, which I generally don't favor because you can't tell which condition triggered the assertion failure, at least with most standard assert implementations.

The overall affect regardless is nil, however. Are you saying that clang static analysis no longer produces warnings with your changes, because I don't see the difference.

[laroche]

Hello Abdullah Almosalami,

clang assumes that after one assert it is possible to then continue code execution and this
will lead to NULL pointer exceptions then. My changes avoid these NULL pointer exceptions
and thus remove clang warnings (by checking pointers against NULL also for subsequent asserts).

Another option for the compiler would be to change the assert function to be no-return,
then the compiler would also not think that followup NPE are possible.

Regarding "configASSERT( ptr ) → configASSERT( ptr != NULL )". I just thought that the longer
version would be a better match, but this could also stay with the shorter version if you like.
(Wouldn't MISRA favour the longer version?)

best regards,

Florian La Roche

[memphis242]

@laroche,

... My changes avoid these NULL pointer exceptions ...

Hmm... I'm not seeing how your changes avoid null pointer exceptions. There are no additional checks; just combining multiple assert conditions into one and using != NULL. Am I missing something? Maybe your changes clear the clang static analyzer warnings because clang looks for != NULL specifically, or because the check for != NULL is near the dereference locations, but technically, null pointer dereferences can still happen in a production/release build. configASSERT is usually removed for release builds, so a null pointer exception can happen if an application passes one in - a form of "design by contract" if you will.

[aggarg]

Hmm... I'm not seeing how your changes avoid null pointer exceptions.

Consider the following:

configASSERT( pxQueue != NULL );
configASSERT( pxQueue->uxItemSize != 0U );

If the first assertion allows execution to continue when pxQueue is NULL, the second assertion will cause a NULL pointer dereference.

Combining them using a logical AND operator ensures that the second expression is not evaluated if the first one is false:

configASSERT( ( pxQueue != NULL ) && ( pxQueue->uxItemSize != 0U ) );

This works because C's short-circuit evaluation guarantees that if pxQueue is NULL, the second part of the expression won't be evaluated, preventing the NULL pointer dereference. The evaluation stops as soon as the first condition fails, since the overall AND expression would be false regardless of the second condition.

[aggarg]

@laroche What do you think about my comment - #1284 (comment)?

[memphis242]

@aggarg Yup, makes sense. I was assuming the asserts don't return if conditions are false in any application. You mentioning the AND short circuiting reminded me though that that's probably how the clang static analyzer warnings go away with laroch's changes. I get that part now, thank you.

[laroche]

Hello all,

My current thinking is this:

• This is all about debug code. It is certainly not critical.
• I personally like also debug code to be clean without compiler warnings.
• I assume if marking the assert function as no-return, then also the compiler warnings should disappear, but
  I haven't tested this until now.
• I still think my code changes should be reasonable small/save and also remove compiler warnings (even
  if no-return is not used), so could as well be applied as a more general fix.

Anyway, it is your decision and I am fine with either way you decide.
(I know I push quite a few small changes your way.)

greetings and best regards,

Florian La Roche

[aggarg]

I still think my code changes should be reasonable small/save and also remove compiler warnings (even
if no-return is not used), so could as well be applied as a more general fix.

Agree that this changes does not have any drawback.